Skip to content

Conversation

nvollmar
Copy link
Contributor

@nvollmar nvollmar commented Sep 7, 2025

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)

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

Copy link

github-actions bot commented Sep 7, 2025

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

Copy link

github-actions bot commented Sep 7, 2025

👍
No issues in PR Title / Commit Title

@nvollmar
Copy link
Contributor Author

nvollmar commented Sep 8, 2025

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

vyosbot added a commit to vyos/vyos-cla-signatures that referenced this pull request Sep 8, 2025
@nvollmar
Copy link
Contributor Author

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

Copy link

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

@sever-sever
Copy link
Member

@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

Copy link

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

@nvollmar
Copy link
Contributor Author

@sever-sever rebased

@sever-sever
Copy link
Member

sever-sever commented Sep 22, 2025

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?

@nvollmar nvollmar force-pushed the T6686 branch 2 times, most recently from 1c4dc01 to 48df805 Compare September 23, 2025 13:50
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nvollmar nvollmar Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants