T6686: adds container health checks #4702

nvollmar · 2025-09-07T19:13:49Z

Change summary

Adds config options for container health checks

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

https://vyos.dev/T6686

Related PR(s)

T6686: adds container health check documentation vyos-documentation#1682

How to test / Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_container.py
test_api_socket (__main__.TestContainer.test_api_socket) ... ok
test_basic (__main__.TestContainer.test_basic) ... ok
test_cpu_limit (__main__.TestContainer.test_cpu_limit) ... ok
test_dual_stack_network (__main__.TestContainer.test_dual_stack_network) ... ok
test_healthcheck (__main__.TestContainer.test_healthcheck) ... ok
test_ipv4_network (__main__.TestContainer.test_ipv4_network) ... ok
test_ipv6_network (__main__.TestContainer.test_ipv6_network) ... ok
test_name_server (__main__.TestContainer.test_name_server) ... ok
test_network_mtu (__main__.TestContainer.test_network_mtu) ... ok
test_no_name_server (__main__.TestContainer.test_no_name_server) ... ok
test_uid_gid (__main__.TestContainer.test_uid_gid) ... ok

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

github-actions · 2025-09-07T19:13:58Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

github-actions · 2025-09-07T19:14:04Z

👍
No issues in PR Title / Commit Title

nvollmar · 2025-09-08T06:37:26Z

I have read the CLA Document and I hereby sign the CLA

src/conf_mode/container.py

nvollmar · 2025-09-14T10:14:10Z

@c-po @sever-sever This would be ready for a review

github-actions · 2025-09-15T11:32:49Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

sever-sever · 2025-09-15T11:36:18Z

@nvollmar rebase please and fix conflicts


<<<<<<< T6686
    return f'{container_base_cmd} {healthcheck} --net {networks} {ip_param} {entrypoint} {image} {command} {command_arguments}'.strip()
=======
        addr_info = ''.join(container_config['network'][network]['address'])

    mac_address = f'--mac-address {gen_mac(name, addr_info, host_ident)}'

    return f'{container_base_cmd} --no-healthcheck --net {networks} {ip_param} {mac_address} {entrypoint} {image} {command} {command_arguments}'.strip()
>>>>>>> current

github-actions · 2025-09-15T11:45:46Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

nvollmar · 2025-09-18T12:39:48Z

@sever-sever rebased

sever-sever · 2025-09-22T14:48:00Z

CI cannot build the packet

 dpkg-buildpackage: info: host architecture amd64
dpkg-checkbuilddeps: error: Unmet build dependencies: python3-nose
dpkg-buildpackage: warning: build dependencies/conflicts unsatisfied; aborting
dpkg-buildpackage: warning: (Use -d flag to override.)

Could you rebase, please?

src/conf_mode/container.py

sever-sever · 2025-09-24T14:33:02Z

src/conf_mode/container.py

    mac_address = f'--mac-address {gen_mac(name, addr_info, host_ident)}'

-    return f'{container_base_cmd} --no-healthcheck --net {networks} {ip_param} {mac_address} {entrypoint} {image} {command} {command_arguments}'.strip()
+    return f'{container_base_cmd} {healthcheck} --net {networks} {ip_param} {mac_address} {entrypoint} {image} {command} {command_arguments}'.strip()


The default behaviour is changed,
If we do not have any health-check option, we used --no-healthcheck, but now it will be ''

The issue here is, the --no-healthcheck was only added when using container networking, when host networking was used (see line 491) it wasn't.
So far I never encountered a container which defined health checks in the image by itself. I'd argue changing the default behaviour here to make it consistent regardless of which networking is used would be preferable.

interface-definitions/container.xml.in

github-actions · 2025-09-24T20:14:07Z

CI integration ❌ failed!

Details

CI logs

CLI Smoketests (no interfaces) ❌ failed
CLI Smoketests VPP 👍 passed
CLI Smoketests (interfaces only) 👍 passed
Config tests 👍 passed
Config tests VPP 👍 passed
RAID1 tests 👍 passed
TPM tests 👍 passed

c-po · 2025-09-30T13:48:05Z

interface-definitions/container.xml.in

+              <leafNode name="disable">
+                <properties>
+                  <help>Disable health check if container has one defined</help>
+                  <valueless/>
+                </properties>
+              </leafNode>


Suggested change

<leafNode name="disable">

<properties>

<help>Disable health check if container has one defined</help>

<valueless/>

</properties>

</leafNode>

#include <include/generic-disable-node.xml.i>

We have a generic XML building block for the disable CLI option.

c-po · 2025-09-30T13:49:14Z

interface-definitions/container.xml.in

+                <properties>
+                  <help>Interval for the health checks</help>
+                  <completionHelp>
+                    <list>disable</list>


An interval that is not defined on the CLI should count as disabled

The situation is a bit more complex. Container can define health checks including intervals etc. Podman by default will run them as defined by the image.

So all the config options here are overrides if the image defines them already. And the disable option is there to explicitly also disable pre-defined health checks.

c-po · 2025-09-30T13:50:04Z

interface-definitions/container.xml.in

+                    <description>Run health checks manually</description>
+                  </valueHelp>
+                  <valueHelp>
+                    <format>&lt;time&gt;&lt;suffix&gt;</format>


Not a fan of multiple units. Is there a real reason to run a health-check only once an hour? We should use a seconds base - meaning a value of 3600 would be once an hour.

c-po · 2025-09-30T13:50:32Z

interface-definitions/container.xml.in

+                    <description>Duration with time suffix (ms, s, m, h)</description>
+                  </valueHelp>
+                  <constraint>
+                    <regex>^(?:\d+(?:\.\d+)?(?:ms|s|m|h))+$</regex>


Please change this to seconds only. We should not make the CLI overly complex. Base units are fine.

c-po · 2025-09-30T13:51:53Z

interface-definitions/container.xml.in

+              </leafNode>
+              <leafNode name="retries">
+                <properties>
+                  <help>The number of retries before container is consider unhealthy</help>


Suggested change

<help>The number of retries before container is consider unhealthy</help>

<help>Number of retries before container is consider unhealthy</help>

<valueHelp>

<format>u32:1-255</format>

<description>Update interval in minutes</description>

</valueHelp>

Please add a <valueHelp> definition for the retry cound and allowed range.

A value of 0 should be explicitly stated as "no retry"

github-actions bot added the current label Sep 7, 2025

github-actions bot assigned nvollmar Sep 7, 2025

nvollmar force-pushed the T6686 branch from afe5b30 to 9f27a8f Compare September 8, 2025 05:24

vyosbot added a commit to vyos/vyos-cla-signatures that referenced this pull request Sep 8, 2025

@nvollmar has signed the CLA in vyos/vyos-1x#4702

3210cdf

nvollmar commented Sep 8, 2025

View reviewed changes

src/conf_mode/container.py Outdated Show resolved Hide resolved

nvollmar force-pushed the T6686 branch 3 times, most recently from 0baafcf to ec5e1e3 Compare September 8, 2025 20:34

nvollmar mentioned this pull request Sep 10, 2025

T6686: adds container health check documentation vyos/vyos-documentation#1682

Open

1 task

github-actions bot added the conflicts label Sep 15, 2025

nvollmar force-pushed the T6686 branch from ec5e1e3 to 69da10c Compare September 15, 2025 11:45

github-actions bot removed the conflicts label Sep 15, 2025

nvollmar force-pushed the T6686 branch from 69da10c to 3fce070 Compare September 23, 2025 05:24

sever-sever requested review from c-po and sever-sever September 23, 2025 09:14

sever-sever reviewed Sep 23, 2025

View reviewed changes

src/conf_mode/container.py Show resolved Hide resolved

nvollmar force-pushed the T6686 branch 2 times, most recently from 1c4dc01 to 48df805 Compare September 23, 2025 13:50

sever-sever requested changes Sep 24, 2025

View reviewed changes

nvollmar added 2 commits September 24, 2025 20:13

T6686: adds container health checks

752965b

T6686: adds explicit disable option

424448c

nvollmar force-pushed the T6686 branch from 48df805 to 424448c Compare September 24, 2025 18:14

sever-sever requested a review from dmbaturin September 24, 2025 22:55

nvollmar requested a review from sever-sever September 25, 2025 05:13

c-po requested changes Sep 30, 2025

View reviewed changes

T6686: adds container health checks #4702

Are you sure you want to change the base?

T6686: adds container health checks #4702

Uh oh!

Conversation

nvollmar commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change summary

Types of changes

Related Task(s)

Related PR(s)

How to test / Smoketest result

Checklist:

Uh oh!

github-actions bot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvollmar commented Sep 8, 2025

Uh oh!

Uh oh!

nvollmar commented Sep 14, 2025

Uh oh!

github-actions bot commented Sep 15, 2025

Uh oh!

sever-sever commented Sep 15, 2025

Uh oh!

github-actions bot commented Sep 15, 2025

Uh oh!

nvollmar commented Sep 18, 2025

Uh oh!

sever-sever commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvollmar Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 24, 2025

Details

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nvollmar commented Sep 7, 2025 •

edited

Loading

github-actions bot commented Sep 7, 2025 •

edited

Loading

github-actions bot commented Sep 7, 2025 •

edited

Loading

sever-sever commented Sep 22, 2025 •

edited

Loading

nvollmar Sep 24, 2025 •

edited

Loading