Skip to content

Conversation

@nvollmar
Copy link
Contributor

@nvollmar nvollmar commented Nov 1, 2025

Change summary

Simplify container run arguments to reduce chance for unintended inconsistencies between host and container networking

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)

Related PR(s)

How to test / Smoketest result

DEBUG - Running Testcase: /usr/libexec/vyos/tests/smoke/cli/test_container.py
DEBUG - test_api_socket (__main__.TestContainer.test_api_socket) ... ok
DEBUG - test_basic (__main__.TestContainer.test_basic) ... ok
DEBUG - test_cpu_limit (__main__.TestContainer.test_cpu_limit) ... ok
DEBUG - test_dual_stack_network (__main__.TestContainer.test_dual_stack_network) ... ok
DEBUG - test_healthcheck (__main__.TestContainer.test_healthcheck) ... ok
DEBUG - test_ipv4_network (__main__.TestContainer.test_ipv4_network) ... ok
DEBUG - test_ipv6_network (__main__.TestContainer.test_ipv6_network) ... ok
DEBUG - test_name_server (__main__.TestContainer.test_name_server) ... ok
DEBUG - test_network_mtu (__main__.TestContainer.test_network_mtu) ... ok
DEBUG - test_network_types (__main__.TestContainer.test_network_types) ... ok
DEBUG - test_no_name_server (__main__.TestContainer.test_no_name_server) ... ok
DEBUG - test_uid_gid (__main__.TestContainer.test_uid_gid) ... ok
DEBUG - test_user_defined_mac (__main__.TestContainer.test_user_defined_mac) ... 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
Copy link

github-actions bot commented Nov 1, 2025

👍
No issues in PR Title / Commit Title

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the network configuration handling in the container run arguments generation by consolidating the network parameter construction into a single variable. Instead of having early returns with duplicated command construction, the code now builds a net variable that contains the appropriate network configuration and uses it in a single return statement.

  • Introduced a net variable to hold network configuration parameters
  • Replaced early return for host networking with conditional assignment to net
  • Unified the final return statement to use the net variable consistently

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nvollmar nvollmar force-pushed the T7982 branch 2 times, most recently from 3a5d42a to 1a8be30 Compare November 2, 2025 12:34
Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I don't see any issues with the logic but I think the commit and PR title should clearly mark it container: and explain what arguments the change is about.

@nvollmar nvollmar changed the title T7982: simplifies run arguments into one T7982: container: generate run arguments once Nov 10, 2025
@nvollmar
Copy link
Contributor Author

I added container: and tried to improve the wording

@github-actions
Copy link

CI integration 👍 passed!

Details

CI logs

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

@nvollmar
Copy link
Contributor Author

@dmbaturin Would be nice to get this in before working on other container related improvements.

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

The logic looks good to me now.

Copy link
Contributor

@hedrok hedrok left a comment

Choose a reason for hiding this comment

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

Clean refactoring - no functional changes.

@sever-sever sever-sever added the bp/circinus Create automatic backport for circinus label Dec 5, 2025
@github-actions github-actions bot added the rebase label Dec 5, 2025
Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

Merge container options to simplify and do not duplicate the code.
Code refactoring.
I have not tested it.

@sever-sever sever-sever merged commit 6795f10 into vyos:current Dec 5, 2025
19 checks passed
@vyosbot vyosbot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Dec 5, 2025
@nvollmar nvollmar deleted the T7982 branch December 5, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bp/circinus Create automatic backport for circinus current mirror-completed rebase

Development

Successfully merging this pull request may close these issues.

5 participants