Add IPv6-only management mode support for running tests#23292
Add IPv6-only management mode support for running tests#23292opcoder0 merged 3 commits intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| @pytest.fixture(name="duthosts", scope="session") | ||
| def fixture_duthosts(enhance_inventory, ansible_adhoc, tbinfo, request): | ||
| def fixture_duthosts(enhance_inventory, ansible_adhoc, tbinfo, request, ipv6_only_mgmt_enabled): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
bf9aafe to
824ee59
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This change adds support for running tests in IPv6-only management mode: - Add --ipv6_only_mgmt pytest option and -6 flag for run_tests.sh - Add ipv6_only_mgmt_enabled fixture in conftest.py - Update tests/common/devices/base.py to use IPv6 address when mode is enabled - Skip IPv4 mgmt sanity check when mgmt_ip is IPv6 in sanity_check/checks.py - Use ping6 for IPv6 management addresses in reboot.py - Add retry logic for chrony service startup in ansible playbook - Update documentation with instructions for running tests with IPv6 Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>
824ee59 to
c752d66
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
StormLiangMS
left a comment
There was a problem hiding this comment.
Review: IPv6-Only Management Mode Support
Approve ✅ — well-structured, clean design. A few items to discuss below.
Architecture (good)
- Class-level flag on
AnsibleHostBaseset by session-scoped fixture, checked in__init__— clean, no global state leaking. - Fixture ordering enforced —
ipv6_only_mgmt_enabledinjected as dependency offixture_duthosts, guaranteeing the flag is set before anySonicHost.__init__calls. - Backward compatible — default is
False, no behavior change unless flag is explicitly passed.
Issues
1. (Medium) ping6 may not exist on all Linux distros
In reboot.py:
ping_cmd = "ping6" if is_ipv6_address(str(duthost.mgmt_ip)) else "ping"On newer distros (Ubuntu 22.04+), ping6 is a symlink to ping and may not exist. The portable alternative is ping -6. This runs on the test server (localhost), not the DUT, so it depends on the sonic-mgmt container's OS.
2. (Low) Missing ansible_hostv6 — silent fallback to IPv4
In base.py:
if self.is_ipv6_only_mgmt() and ansible_hostv6:
self.mgmt_ip = ansible_hostv6
else:
self.mgmt_ip = ansible_host # falls back silentlyIf --ipv6_only_mgmt is set but ansible_hostv6 is not defined in inventory, it silently falls back to IPv4 with no indication. Consider logging a warning: "IPv6-only mode requested but ansible_hostv6 not defined for {hostname}, falling back to IPv4."
3. (Low) _mgmt_ipv4 not used anywhere
self._mgmt_ipv4 = ansible_host # "Keep IPv4 available for reference"Set but never read. Fine if for future use, otherwise dead code.
4. (Question) Other ping calls in the codebase
Only reboot.py::collect_mgmt_config_by_console was updated. Any other test that does localhost.shell(f"ping ... {duthost.mgmt_ip}") will break in IPv6-only mode. May be worth a utility function like ping_mgmt(duthost, localhost) that auto-selects ping/ping6, or a broader grep to confirm no other call sites exist.
Internal PR #20116
Reviewed — config-only (9 new ipv6.yml files for internal labs with IPv6 NTP/DNS/syslog/TACACS addresses). No code logic, straightforward.
Chrony retry
Unrelated to IPv6 flag but reasonable for environments where NTP may be slower to start. No concerns.
Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thank you @StormLiangMS for the comments. I have addressed 1 and 2. For (3) - may be useful for troubleshooting hence left it as-is. For (4) hope to fix them and any other issues as part of IPv6 only tests I am running. |
StormLiangMS
left a comment
There was a problem hiding this comment.
Review: Needs minor fix before merge
Summary: Clean implementation of IPv6-only management mode. Fixture design, run_tests.sh wiring, sanity check skip, ping6 handling, and documentation are all solid. One bug to fix:
🐛 Bug: Spurious warning in normal (non-IPv6) mode (tests/common/devices/base.py)
The else branch fires in two cases:
- IPv6 mode NOT requested (normal operation — the common case)
- IPv6 mode requested BUT
ansible_hostv6is undefined
But the warning is unconditional — so in every normal test run, every DUT init logs "IPv6-only mode requested but ansible_hostv6 not defined". Suggested fix:
if self.is_ipv6_only_mgmt() and ansible_hostv6:
self.mgmt_ip = ansible_hostv6
self.mgmt_ipv6 = ansible_hostv6
self._mgmt_ipv4 = ansible_host
logger.debug(...)
elif self.is_ipv6_only_mgmt():
logger.warning(
"IPv6-only mode requested but ansible_hostv6 not defined for %s, "
"falling back to IPv4.", hostname)
self.mgmt_ip = ansible_host
self.mgmt_ipv6 = None
else:
self.mgmt_ip = ansible_host
self.mgmt_ipv6 = ansible_hostv6Minor observations (non-blocking):
_mgmt_ipv4is only set in the IPv6 branch — consider initializingself._mgmt_ipv4 = Nonebefore the if/else to avoid potentialAttributeErrorin future code.host_vars.get("ansible_host")changes behavior fromKeyErrorto silentNoneifansible_hostis missing — probably fine but worth noting.- Chrony retry fix is useful but tangential to IPv6 — could be a separate PR.
Positives: Fixture ordering via duthosts dependency is clean, ping -6 over ping6 is the right cross-distro choice, docs are thorough, is_ipv6_address is already imported in checks.py ✅.
Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>
Thank you. I have fixed it now. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
) This change adds support for running tests in IPv6-only management mode: - Add --ipv6_only_mgmt pytest option and -6 flag for run_tests.sh - Add ipv6_only_mgmt_enabled fixture in conftest.py - Update tests/common/devices/base.py to use IPv6 address when mode is enabled - Skip IPv4 mgmt sanity check when mgmt_ip is IPv6 in sanity_check/checks.py - Use ping6 for IPv6 management addresses in reboot.py - Add retry logic for chrony service startup in ansible playbook - Update documentation with instructions for running tests with IPv6 Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>
Description of PR
This change adds support for running tests in IPv6-only management mode:
Summary:
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
Support to run tests in IPv6 only management network environment
How did you do it?
Modify deploy mini graph configuration and related scripts.
How did you verify/test it?
On internal pipelines. This is additional work to the infrastructure changes that were made earlier #22310
Any platform specific information?
None
Supported testbed topology if it's a new test case?
NA
Documentation
Updated.