boot: T3871: rework interface renaming and ordering - #5350
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe pull request adds post-udev hardware-based interface naming, integrates the resolver into boot, adds regression coverage, and makes flow-control checks driver-aware. Interface Naming Resolution
Flow-Control Capability Detection
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/cli/test_interfaces_ethernet.py`:
- Around line 290-302: After the expected ConfigSessionError in the
check_flow_control() failure branch, reset the candidate configuration before
the next interface iteration by deleting the disable-flow-control leaf or
discarding the session. Keep the existing failure assertion and ensure no stale
candidate state remains.
In `@src/system/vyos-net-name-resolve.py`:
- Around line 200-219: Update find_available() in
src/system/vyos-net-name-resolve.py (lines 200-219) to search missing indices
from 0 so it always returns the lowest free index; this authoritative copy
affects compute_rename_plan(). Apply the same range-start change in
src/udev/vyos_net_name (lines 55-72) for consistency.
- Around line 259-277: Update the relocation logic in the plan-building block to
include all configured target names, including targets whose hardware IDs are
absent from current. Build taken from the existing occupied/planned names plus
configured.values() before calling find_available, while preserving normal
relocation and unchanged-name behavior.
- Around line 328-361: Update safe_bulk_rename so phase-2 failures from
rename_interface(tmp, target) are handled: remove the corresponding
old-to-target entry from applied, restore the interface under its scratch name,
and bring that scratch-named interface back up. Keep successful renames in
applied and preserve the existing phase-2 target bring-up behavior.
In `@src/systemd/vyos-net-name-resolve.service`:
- Around line 5-10: Harden syslog handling for the DefaultDependencies=no
service: in src/systemd/vyos-net-name-resolve.service lines 5-10, either add
explicit ordering after the syslog socket/service or rely on the script guard;
additionally, in src/system/vyos-net-name-resolve.py lines 434-441, wrap
SysLogHandler construction in try/except so a syslog failure does not abort the
rename pass.
In `@src/tests/test_net_name_resolve.py`:
- Around line 124-130: Update the comment in
test_missing_hardware_no_crash_no_entry to reword “mis-assign” without the “mis”
fragment, while preserving the intended meaning that another interface must not
receive the wrong assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 101784c1-de93-4286-918a-c27b682f1789
📒 Files selected for processing (9)
python/vyos/ethtool.pysmoketest/scripts/cli/test_interfaces_ethernet.pysrc/helpers/vyos-interface-rescan.pysrc/init/vyos-routersrc/system/vyos-net-name-resolve.pysrc/systemd/vyos-net-name-resolve.servicesrc/tests/helper.pysrc/tests/test_net_name_resolve.pysrc/udev/vyos_net_name
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (8)
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Typos / typos: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-23T18%3A18%3A18Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-23T17%3A17%3A34Z&ske=2026-07-23T18%3A18%3A18Z&sks=b&skv=2018-11-09&sig=12b7SD4%2F76t3bnoJB8wnCucJE3Zu4Q4JNiTClrPNQeg%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-23T18%3A18%3A18Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-23T17%3A17%3A34Z&ske=2026-07-23T18%3A18%3A18Z&sks=b&skv=2018-11-09&sig=12b7SD4%2F76t3bnoJB8wnCucJE3Zu4Q4JNiTClrPNQeg%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.110.133, 185.199.111.133, 185.199.108.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Typos / 0_typos.txt: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-23T18%3A18%3A18Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-23T17%3A17%3A34Z&ske=2026-07-23T18%3A18%3A18Z&sks=b&skv=2018-11-09&sig=12b7SD4%2F76t3bnoJB8wnCucJE3Zu4Q4JNiTClrPNQeg%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-23T18%3A18%3A18Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-23T17%3A17%3A34Z&ske=2026-07-23T18%3A18%3A18Z&sks=b&skv=2018-11-09&sig=12b7SD4%2F76t3bnoJB8wnCucJE3Zu4Q4JNiTClrPNQeg%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.110.133, 185.199.111.133, 185.199.108.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
GitHub Actions: VyOS ISO Integration Test / build_iso: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: packages/vyos-1x
fetch-depth: 0
ref: 951c88449d87c9bc7e4bd367460833c055fe67b5
repository: c-po/vyos-1x
***REDACTED***
submodules: true
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-tags: false
show-progress: true
lfs: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / set_config: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: T3871: rework boot interface renaming and interface ordering
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: packages/vyos-1x
fetch-depth: 0
ref: 951c88449d87c9bc7e4bd367460833c055fe67b5
repository: c-po/vyos-1x
***REDACTED***
submodules: true
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-tags: false
show-progress: true
lfs: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/helpers/vyos-interface-rescan.pysrc/tests/helper.pysmoketest/scripts/cli/test_interfaces_ethernet.pypython/vyos/ethtool.pysrc/system/vyos-net-name-resolve.pysrc/tests/test_net_name_resolve.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/cli/test_interfaces_ethernet.py
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/ethtool.py
🧠 Learnings (3)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_interfaces_ethernet.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_interfaces_ethernet.py
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
smoketest/scripts/cli/test_interfaces_ethernet.py
🪛 ast-grep (0.44.1)
src/system/vyos-net-name-resolve.py
[info] 398-398: use jsonify instead of json.dumps for JSON output
Context: json.dumps(status, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/tests/test_net_name_resolve.py
[warning] 373-373: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(path, 'address'), 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 410-410: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(hint)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 421-421: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(stale, 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 478-478: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth0'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 480-480: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth1'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 496-496: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 522-522: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 GitHub Actions: Typos / 0_typos.txt
src/tests/test_net_name_resolve.py
[error] 126-126: typos validation failed: mis should be miss or mist.
🪛 GitHub Actions: Typos / typos
src/tests/test_net_name_resolve.py
[error] 126-126: typos formatting/lint error: mis should be miss, mist.
🪛 GitHub Check: typos
src/tests/test_net_name_resolve.py
[warning] 126-126:
"mis" should be "miss" or "mist".
🔍 Remote MCP Context7
Additional review context
DefaultDependencies=noremoves systemd’s usual implicit dependencies, so ordering must come from explicitAfter=/Before=relations.Type=oneshotis appropriate for a one-time initialization job; withRemainAfterExit=yes, the unit stays active afterExecStartexits instead of immediately going inactive.WantedBy=affects enablement:systemctl enablecreates a.wants/symlink under the target, which causes the target to pull in the unit via aWants=-style dependency.Wants=is a soft dependency;Requires=is hard.Before=/After=are ordering only, not enablement.
These points are directly relevant to reviewing the new vyos-net-name-resolve.service and its boot-order assumptions.
🔇 Additional comments (13)
src/tests/helper.py (1)
17-33: LGTM!src/tests/test_net_name_resolve.py (1)
41-121: LGTM!Also applies to: 132-161, 164-236, 239-427, 429-527
python/vyos/ethtool.py (1)
218-222: LGTM!src/udev/vyos_net_name (4)
32-43: LGTM!
108-111: LGTM!
156-178: LGTM!
194-196: LGTM!Also applies to: 208-210
src/helpers/vyos-interface-rescan.py (1)
99-102: LGTM!src/system/vyos-net-name-resolve.py (2)
67-401: LGTM!
404-432: LGTM!src/systemd/vyos-net-name-resolve.service (1)
12-18: LGTM!src/init/vyos-router (2)
370-382: LGTM!
627-640: LGTM!
c-po
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/test_net_name_resolve.py (1)
429-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak coverage: test can't actually detect cross-group distance leakage.
ifaceBandradio0are the only members of their respective type groups, so their assigned distances (5 and 0) are never compared against a same-group peer. The test would pass identically even if bootstrap ranking computed PCIe distance globally before splitting by ethernet/wireless (the kind of bug this test's docstring intends to catch), since group prefixes already guaranteeeth0/wlan0regardless.Add a second candidate to at least one group with a distance value that would reorder results if groups leaked into each other, to give this test real detection power.
🧪 Suggested strengthening
- self.is_wireless.side_effect = lambda name: name == 'radio0' - current = {'ifaceB': 'bb', 'radio0': 'aa'} - self.pcie_distance.side_effect = lambda name: { - 'ifaceB': 5, 'radio0': 0, - }[name] - plan = resolver.compute_bootstrap_plan({}, current, {}) - self.assertEqual(plan.get('radio0'), 'wlan0') - self.assertEqual(plan.get('ifaceB'), 'eth0') + self.is_wireless.side_effect = lambda name: name in ('radio0', 'radio1') + current = {'ifaceB': 'bb', 'radio0': 'aa', 'radio1': 'cc'} + self.pcie_distance.side_effect = lambda name: { + 'ifaceB': 5, 'radio0': 3, 'radio1': 0, + }[name] + plan = resolver.compute_bootstrap_plan({}, current, {}) + # radio1 has the lowest distance overall but must only outrank radio0 + # (its own group), never bump ifaceB's eth0 numbering. + self.assertEqual(plan.get('radio1'), 'wlan0') + self.assertEqual(plan.get('radio0'), 'wlan1') + self.assertEqual(plan.get('ifaceB'), 'eth0')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/test_net_name_resolve.py` around lines 429 - 440, Strengthen test_pcie_distance_ordering_independent_of_ethernet_wireless_split by adding a second candidate to at least one interface type group and assigning distances so same-group ordering is observable while cross-group distance leakage would change the result. Update the expected bootstrap plan assertions to verify both candidates retain the correct eth/wlan numbering independently of the other group’s distances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tests/test_net_name_resolve.py`:
- Around line 429-440: Strengthen
test_pcie_distance_ordering_independent_of_ethernet_wireless_split by adding a
second candidate to at least one interface type group and assigning distances so
same-group ordering is observable while cross-group distance leakage would
change the result. Update the expected bootstrap plan assertions to verify both
candidates retain the correct eth/wlan numbering independently of the other
group’s distances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 87f2efa5-681a-4bd5-8027-48ced2e7ad16
📒 Files selected for processing (2)
src/system/vyos-net-name-resolve.pysrc/tests/test_net_name_resolve.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/system/vyos-net-name-resolve.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: result
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/tests/test_net_name_resolve.py
🔍 Remote MCP Context7
Relevant systemd context
Type=oneshotservices have a disabled start timeout by default; this PR’s explicitTimeoutStartSec=90stherefore provides the resolver’s startup bound.DefaultDependencies=noremoves implicit ordering/dependency setup, so the resolver’s correctness depends on explicit ordering invyos-router.- Ordering directives such as
After=/Before=establish sequencing but do not themselves pull a unit into the transaction; the directvyos-routerstart invocation remains essential. Requires=does not necessarily propagate a required unit’s start failure back to the requiring unit, consistent with the PR’s explicitoverall_statushandling.
🔇 Additional comments (1)
src/tests/test_net_name_resolve.py (1)
252-317: LGTM!Also applies to: 334-337, 405-427
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/tests/test_net_name_resolve.py (2)
699-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative-only assertions can pass vacuously. If
src/systemd/vyos-net-name-resolve.servicewere gutted or its content changed shape, all threeassertNotInchecks still pass. Add a positive anchor (e.g.Type=oneshot/ theExecStartpath) so the test fails loudly when the unit stops doing what it claims.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/test_net_name_resolve.py` around lines 699 - 705, Add a positive content assertion to test_service_unit_not_independently_ordered_before_vyos_router that verifies the unit retains its expected behavior, such as Type=oneshot or the intended ExecStart path, while preserving the existing negative ordering assertions.
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gap: no test for a free index below the lowest used one.
{'eth2','eth5'}still passes with the currentrange(index_list[0], ...)bound. Addfind_available({'eth5'}, 'eth')→eth0to pin the docstring's "lowest free index" contract (see the open issue onsrc/system/vyos-net-name-resolve.pyLine 247).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/test_net_name_resolve.py` around lines 54 - 62, Add a test alongside the existing resolver tests for find_available using {'eth5'} and the 'eth' prefix, asserting that it returns 'eth0'. This covers the lowest-free-index behavior below the smallest used index without changing the existing contiguous, hole-filling, or append cases.src/system/vyos-net-name-resolve.py (1)
462-478: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider recomputing the hw-id plan after
wait_for_settle().planis built from the pre-settle snapshot (Line 462), but Line 471 replacescurrentwith a newer one. Any interface that appeared/was renamed during the settle window makes a plan source stale, and that entry then fails insafe_bulk_rename(). Recomputingcompute_rename_plan(configured, current)after the settle would keep both plans consistent.Also minor:
write_status()receives the pre-renamecurrent(Line 478) while hints usefinal_current, and thefoundkey actually contains MACs, not names — consider renaming the key for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/vyos-net-name-resolve.py` around lines 462 - 478, Recompute the rename plan from the settled interface snapshot in the bootstrap branch before calling safe_bulk_rename, using compute_rename_plan(configured, current) and then applying any required bootstrap entries so plan matches current. Update write_status to use the post-rename final_current snapshot, and rename its found field to clearly indicate it contains MAC addresses, preserving all existing status data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/system/vyos-net-name-resolve.py`:
- Around line 370-397: Update rename_interface so a failed rename restores the
interface to its original administrative-up state after it was brought down,
while preserving the existing error return and logging. Ensure safe_bulk_rename
continues to treat the operation as failed and does not record the interface in
applied when recovery is performed.
---
Nitpick comments:
In `@src/system/vyos-net-name-resolve.py`:
- Around line 462-478: Recompute the rename plan from the settled interface
snapshot in the bootstrap branch before calling safe_bulk_rename, using
compute_rename_plan(configured, current) and then applying any required
bootstrap entries so plan matches current. Update write_status to use the
post-rename final_current snapshot, and rename its found field to clearly
indicate it contains MAC addresses, preserving all existing status data.
In `@src/tests/test_net_name_resolve.py`:
- Around line 699-705: Add a positive content assertion to
test_service_unit_not_independently_ordered_before_vyos_router that verifies the
unit retains its expected behavior, such as Type=oneshot or the intended
ExecStart path, while preserving the existing negative ordering assertions.
- Around line 54-62: Add a test alongside the existing resolver tests for
find_available using {'eth5'} and the 'eth' prefix, asserting that it returns
'eth0'. This covers the lowest-free-index behavior below the smallest used index
without changing the existing contiguous, hole-filling, or append cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c8276778-27f6-4835-8163-5c371326a110
📒 Files selected for processing (9)
python/vyos/ethtool.pysmoketest/scripts/cli/test_interfaces_ethernet.pysrc/helpers/vyos-interface-rescan.pysrc/init/vyos-routersrc/system/vyos-net-name-resolve.pysrc/systemd/vyos-net-name-resolve.servicesrc/tests/helper.pysrc/tests/test_net_name_resolve.pysrc/udev/vyos_net_name
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/systemd/vyos-net-name-resolve.service
- src/tests/helper.py
- src/init/vyos-router
- smoketest/scripts/cli/test_interfaces_ethernet.py
- src/udev/vyos_net_name
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (6)
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: VyOS ISO Integration Test / set_config: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run if [[ "pull_request" == "pull_request" ]]; then
�[36;1mif [[ "pull_request" == "pull_request" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="5350/merge"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / build_iso: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 314c2beabac82d600b5ba0ae10c4ca67eaab8e25
##[error]fatal: remote error: upload-pack: not our ref 314c2beabac82d600b5ba0ae10c4ca67eaab8e25
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Run if [[ "pull_request" == "pull_request" ]]; then
�[36;1mif [[ "pull_request" == "pull_request" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="5350/merge"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: boot: T3871: rework interface renaming and ordering
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 314c2beabac82d600b5ba0ae10c4ca67eaab8e25
##[error]fatal: remote error: upload-pack: not our ref 314c2beabac82d600b5ba0ae10c4ca67eaab8e25
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/helpers/vyos-interface-rescan.pypython/vyos/ethtool.pysrc/system/vyos-net-name-resolve.pysrc/tests/test_net_name_resolve.py
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/ethtool.py
🪛 ast-grep (0.44.1)
src/system/vyos-net-name-resolve.py
[info] 445-445: use jsonify instead of json.dumps for JSON output
Context: json.dumps(status, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/tests/test_net_name_resolve.py
[warning] 551-551: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(path, 'address'), 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 588-588: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(hint)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 599-599: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(stale, 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 656-656: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth0'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 658-658: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth1'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 674-674: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 700-700: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🔍 Remote MCP vyos.dev
Additional relevant context
-
T3871 — “Resolve unexpected interface name reordering” is currently “Needs testing”. It identifies persistent interface-name ↔
hw-idreassociation after reboot/upgrade as a recurring issue and serves as a meta-task for investigating the naming problem addressed by this PR. -
T9127 — “Bug: Slow-initializing NIC … loses VyOS interface-naming race” is Open with “Requires assessment” priority. It documents a concrete failure mode involving slow Intel
iceinitialization, incorrecthw-idbinding, duplicate MACs, and possible management lockout. Its expected behavior—matching configuredhw-idto permanent MAC regardless of driver initialization order—directly aligns with this PR’s resolver design. -
T2158 — “Commit fails if ethernet interface doesn't support flow control (pause)” is Resolved with “Urgent!” priority. It records that unsupported drivers such as
xen_netfrontcaused commits to fail when VyOS attempted to configure flow control, supporting the rationale for honoring the known-unsupported-driver list incheck_flow_control().
🔇 Additional comments (19)
src/helpers/vyos-interface-rescan.py (1)
100-101: LGTM!python/vyos/ethtool.py (1)
220-222: LGTM!src/system/vyos-net-name-resolve.py (10)
232-251: 🎯 Functional Correctness | ⚡ Quick win
find_available()still skips free indices below the lowest used one. Line 247 bounds the gap search atindex_list[0], so{'eth5'}yieldseth6, noteth0, contradicting the docstring. Range start should be0.
32-64: LGTM!
67-118: LGTM!
121-159: LGTM!
162-190: LGTM!
193-229: LGTM!
254-279: LGTM!
282-314: LGTM!
317-360: LGTM!
411-448: LGTM!src/tests/test_net_name_resolve.py (7)
1-38: LGTM!
86-174: LGTM!
177-216: LGTM!
219-315: LGTM!
318-440: LGTM!
443-494: LGTM!
497-604: LGTM!
| def rename_interface(old: str, new: str) -> bool: | ||
| run(f'ip link set dev {old} down') | ||
| code = run(f'ip link set dev {old} name {new}') | ||
| if code != 0: | ||
| logger.error(f"failed to rename '{old}' -> '{new}' (exit {code})") | ||
| return False | ||
| logger.info(f"renamed '{old}' -> '{new}'") | ||
| return True | ||
|
|
||
|
|
||
| def safe_bulk_rename(plan: dict) -> dict: | ||
| """Two-phase rename: stage every interface via a unique scratch name | ||
| (derived from its ifindex, which is always unique) before assigning | ||
| final names. This makes the whole batch collision-proof regardless of | ||
| permutations/cycles between current and target names - a straight | ||
| from->to rename can fail if the target name is still held by another | ||
| interface earlier/later in the same plan. | ||
| """ | ||
| if not plan: | ||
| return {} | ||
|
|
||
| applied = {} | ||
| scratch = {} | ||
| for old, target in plan.items(): | ||
| tmp = f'vyeth{get_ifindex(old)}' | ||
| if rename_interface(old, tmp): | ||
| scratch[tmp] = (old, target) | ||
| applied[old] = target |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Phase-1 rename failure leaves the interface down with no recovery. rename_interface() downs the link (Line 371) before renaming; if the rename fails the caller records nothing and never re-ups it, so the NIC stays admin-down under its original name. Unconfigured/bootstrap interfaces are not rescued by the later config apply.
🛡️ Proposed fix
for old, target in plan.items():
tmp = f'vyeth{get_ifindex(old)}'
if rename_interface(old, tmp):
scratch[tmp] = (old, target)
applied[old] = target
+ else:
+ # rename failed - don't leave the link administratively down
+ run(f'ip link set dev {old} up')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def rename_interface(old: str, new: str) -> bool: | |
| run(f'ip link set dev {old} down') | |
| code = run(f'ip link set dev {old} name {new}') | |
| if code != 0: | |
| logger.error(f"failed to rename '{old}' -> '{new}' (exit {code})") | |
| return False | |
| logger.info(f"renamed '{old}' -> '{new}'") | |
| return True | |
| def safe_bulk_rename(plan: dict) -> dict: | |
| """Two-phase rename: stage every interface via a unique scratch name | |
| (derived from its ifindex, which is always unique) before assigning | |
| final names. This makes the whole batch collision-proof regardless of | |
| permutations/cycles between current and target names - a straight | |
| from->to rename can fail if the target name is still held by another | |
| interface earlier/later in the same plan. | |
| """ | |
| if not plan: | |
| return {} | |
| applied = {} | |
| scratch = {} | |
| for old, target in plan.items(): | |
| tmp = f'vyeth{get_ifindex(old)}' | |
| if rename_interface(old, tmp): | |
| scratch[tmp] = (old, target) | |
| applied[old] = target | |
| def rename_interface(old: str, new: str) -> bool: | |
| run(f'ip link set dev {old} down') | |
| code = run(f'ip link set dev {old} name {new}') | |
| if code != 0: | |
| logger.error(f"failed to rename '{old}' -> '{new}' (exit {code})") | |
| return False | |
| logger.info(f"renamed '{old}' -> '{new}'") | |
| return True | |
| def safe_bulk_rename(plan: dict) -> dict: | |
| """Two-phase rename: stage every interface via a unique scratch name | |
| (derived from its ifindex, which is always unique) before assigning | |
| final names. This makes the whole batch collision-proof regardless of | |
| permutations/cycles between current and target names - a straight | |
| from->to rename can fail if the target name is still held by another | |
| interface earlier/later in the same plan. | |
| """ | |
| if not plan: | |
| return {} | |
| applied = {} | |
| scratch = {} | |
| for old, target in plan.items(): | |
| tmp = f'vyeth{get_ifindex(old)}' | |
| if rename_interface(old, tmp): | |
| scratch[tmp] = (old, target) | |
| applied[old] = target | |
| else: | |
| # rename failed - don't leave the link administratively down | |
| run(f'ip link set dev {old} up') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/system/vyos-net-name-resolve.py` around lines 370 - 397, Update
rename_interface so a failed rename restores the interface to its original
administrative-up state after it was brought down, while preserving the existing
error return and logging. Ensure safe_bulk_rename continues to treat the
operation as failed and does not record the interface in applied when recovery
is performed.
|
@c-po pls check the regression case: vyos-net-name-resolve: interface with config but no hw-id is given a different name than the one its configuration is written for, silently losing its configuration Tested on VM with >=3 NICs
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/system/vyos-net-name-resolve.py (2)
458-465: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPhase-1 rename failure leaves the link administratively down.
Line 459 downs the link before the rename. If Line 460 fails,
safe_bulk_rename()records nothing and never brings the interface back up, so the NIC stays down under its original name. A bootstrap/unconfigured interface is not rescued by the later config apply.🛡️ Proposed fix
def rename_interface(old: str, new: str) -> bool: run(f'ip link set dev {old} down') code = run(f'ip link set dev {old} name {new}') if code != 0: logger.error(f"failed to rename '{old}' -> '{new}' (exit {code})") + # don't leave the link down under its original name + run(f'ip link set dev {old} up') return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/vyos-net-name-resolve.py` around lines 458 - 465, Update rename_interface so a failed ip link rename restores the original interface’s administrative state after it was brought down. When the rename command returns nonzero, run the corresponding link-up command for old before logging and returning False; preserve the existing success behavior.
268-287: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicated
find_available()never returns an index below the lowest used one. Both copies bound the gap scan withrange(index_list[0], index_list[-1]), so a free slot under the smallest used index is unreachable. With onlyeth5present, both returneth6instead ofeth0, contradicting each docstring.
src/system/vyos-net-name-resolve.py#L268-L287: change the range start to0. This copy is authoritative and drivescompute_rename_plan()squatter relocation at Line 343.src/udev/vyos_net_name#L55-L72: apply the same range-start change. Impact here is provisional only, because the resolver renames unconfigured interfaces afterwards.🐛 Proposed fix (both files)
- missing = sorted(set(range(index_list[0], index_list[-1])) - set(index_list)) + missing = sorted(set(range(0, index_list[-1])) - set(index_list))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/vyos-net-name-resolve.py` around lines 268 - 287, Update find_available() in src/system/vyos-net-name-resolve.py:268-287 to scan missing indices from 0 so free slots below the smallest used suffix are selected; this authoritative copy must return eth0 when only eth5 exists. Apply the same range-start change to the duplicate find_available() logic in src/udev/vyos_net_name:55-72.
🧹 Nitpick comments (1)
src/tests/test_net_name_resolve.py (1)
130-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a free index below the lowest used index.
The current cases only cover a hole between two used indices and a contiguous run. Neither reaches the
range(index_list[0], ...)defect infind_available(). Add a case that pins the documented "lowest free index" contract, so the fix insrc/system/vyos-net-name-resolve.pyLine 283 is guarded.💚 Proposed test
def test_resolver_fills_hole(self): self.assertEqual(resolver.find_available({'eth2', 'eth5'}, 'eth'), 'eth3') + def test_resolver_returns_index_below_lowest_used(self): + # only 'eth5' is taken - 'eth0' is free and is the lowest free index + self.assertEqual(resolver.find_available({'eth5'}, 'eth'), 'eth0') + def test_resolver_no_hole_appends(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/test_net_name_resolve.py` around lines 130 - 134, Add a test alongside test_resolver_fills_hole and test_resolver_no_hole_appends that passes used names whose lowest index is greater than the expected result, and assert resolver.find_available returns the lowest free index below that range, preserving the documented lowest-free-index contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/system/vyos-net-name-resolve.py`:
- Around line 458-465: Update rename_interface so a failed ip link rename
restores the original interface’s administrative state after it was brought
down. When the rename command returns nonzero, run the corresponding link-up
command for old before logging and returning False; preserve the existing
success behavior.
- Around line 268-287: Update find_available() in
src/system/vyos-net-name-resolve.py:268-287 to scan missing indices from 0 so
free slots below the smallest used suffix are selected; this authoritative copy
must return eth0 when only eth5 exists. Apply the same range-start change to the
duplicate find_available() logic in src/udev/vyos_net_name:55-72.
---
Nitpick comments:
In `@src/tests/test_net_name_resolve.py`:
- Around line 130-134: Add a test alongside test_resolver_fills_hole and
test_resolver_no_hole_appends that passes used names whose lowest index is
greater than the expected result, and assert resolver.find_available returns the
lowest free index below that range, preserving the documented lowest-free-index
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 739def41-a52d-484e-8b30-3e06dfb3cac7
📒 Files selected for processing (9)
python/vyos/ethtool.pysmoketest/scripts/cli/test_interfaces_ethernet.pysrc/helpers/vyos-interface-rescan.pysrc/init/vyos-routersrc/system/vyos-net-name-resolve.pysrc/systemd/vyos-net-name-resolve.servicesrc/tests/helper.pysrc/tests/test_net_name_resolve.pysrc/udev/vyos_net_name
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (6)
- python/vyos/ethtool.py
- src/systemd/vyos-net-name-resolve.service
- src/helpers/vyos-interface-rescan.py
- smoketest/scripts/cli/test_interfaces_ethernet.py
- src/tests/helper.py
- src/init/vyos-router
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (1)
GitHub Check: Mergify Merge Protections: 1 applicable rule, 0 validating requirements
Conclusion: failure
# Merge Protections
🔴 **1 of 1 protections blocking** · waiting on 🙋 you
| | Protection | Waiting on |
|:--:|:--|:--:|
| 🔴 | **invalid-task-id label must be absent to merge** | 🙋 you |
## 🔴 invalid-task-id label must be absent to merge
**Waiting for**
- [ ] `label != invalid-task-id`
<details><summary>This rule is failing.</summary>
Block merge while the invalid-task-id label is present. Set by the per-repo product T-ID rule (product repos only); dormant where the label is never applied.
- [ ] `label != invalid-task-id`
</details>
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/tests/test_net_name_resolve.pysrc/system/vyos-net-name-resolve.py
🪛 ast-grep (0.45.0)
src/tests/test_net_name_resolve.py
[warning] 59-59: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self.config_path, 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 764-764: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(path, 'address'), 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 801-801: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(hint)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 812-812: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(stale, 'w')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 872-872: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth0'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 874-874: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth1'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 977-977: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(self.udev_dir, 'eth1'))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1499-1499: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1525-1525: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
src/system/vyos-net-name-resolve.py
[info] 550-550: use jsonify instead of json.dumps for JSON output
Context: json.dumps(status, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔍 Remote MCP vyos.dev
Additional relevant context
-
T9127 — “Bug: Slow-initializing NIC … loses VyOS interface-naming race …” is Open, priority “High”. It documents slow
iceNIC initialization causing incorrectethNassignment, duplicate MACs, nondeterministic naming, and potential management lockout. Its expected behavior is permanent-MAC matching independent of driver initialization order, directly validating this PR’s resolver approach. `` -
T3871 — “Resolve unexpected interface name reordering” remains “Needs testing” and describes recurring interface-name/
hw-idreassociation after reboot or upgrade. `` -
T2158 — “Commit fails if ethernet interface doesn't support flow control (pause)” is Resolved, priority “Urgent!”. Its discussion confirms Xen and
r8169boot/configuration failures, while noting that maintaining a driver blacklist is only a workaround; dynamicethtoolcapability detection was considered preferable. -
T2158 comments specifically state that unsupported flow-control handling should not make systems unbootable and that many unsupported drivers may exist beyond a static blacklist. This is relevant when reviewing whether the new
Ethtool.check_flow_control()behavior sufficiently addresses the broader failure mode. ``
🔇 Additional comments (4)
src/udev/vyos_net_name (1)
33-43: LGTM!Also applies to: 108-111, 155-178, 208-208
src/system/vyos-net-name-resolve.py (1)
101-154: LGTM!Also applies to: 290-347, 468-496, 499-553, 556-629
src/tests/test_net_name_resolve.py (2)
42-128: LGTM!Also applies to: 158-289, 291-431, 432-654, 656-708, 710-818, 821-1424, 1426-1486, 1524-1530
1509-1522: 📐 Maintainability & Code QualityNo change required. All four asserted literals exist in
src/init/vyos-router, so_first_index()does not fail because of a string mismatch.> Likely an incorrect or invalid review comment.
Fixed |
|
@c-po thanks, the reported case is fixed at f052754: the freed NIC returns to its own node, address intact, hw-id regenerated in place The replacement fill introduces a new case though: when freed slots' MAC order doesn't match their old name order, a configured node is bound to the wrong physical NIC. Tested on VM with 4 NICs:
"recovering its original hardware" only holds when existing names already follow the (pcie_distance, MAC) sort, for boxes named by the old udev order that's roughly a coin flip per freed pair. |
…l too Flow-control commits failed intermittently on virtio_net and similar paravirtualized drivers: they support reading pause parameters but not setting them, and that mismatch varies across kernel/QEMU versions. The existing denylist for this already covered speed/duplex changes and its name implied flow-control too, but nothing actually consulted it there - the check relied solely on a live probe. The paired smoketest branched on that same raw probe instead of the fixed capability check, so it missed the same drivers. Assisted-by: Claude:claude-sonnet-5
…lure Two ethtool test cases intentionally trigger a commit failure for an unsupported driver, then move on to the next interface in the same loop. Commit does not auto-rollback on failure and the session is shared across the loop, so the failed change stayed staged and could silently carry into the next interface's commit - failing an otherwise passing interface if it followed an unsupported one. Discard the candidate configuration right after each expected failure, matching the pattern already used elsewhere in the suite. Assisted-by: Claude:claude-sonnet-5
Multi-vendor PCIe NIC systems could lose or rename Ethernet/wireless interfaces on boot, because naming was decided from a single per-device udev event before all hardware had a chance to enumerate. Replace that with one authoritative pass, run once configuration is available during router startup: wait for configured hardware, apply every hw-id binding, and name whatever has none yet by PCIe distance from the root complex and MAC address - the same result every boot. A node with its hw-id deleted (NIC replacement) or its whole configuration removed is treated as an ordinary open slot, filled the same way - recovering its original hardware whenever the interfaces that vacated slots this boot also show up as candidates. Boot now reports any interface still unresolved, and the pass is covered by a new test suite in vyos-build named: make testifname Assisted-by: Claude:claude-sonnet-5
|
CI integration ❌ failed! Details
|
There was a problem hiding this comment.
This is a comprehensive solution to the problem of misnaming of NICs from different vendors, by (1) identifying the underlying logic error (2) updating and improving the udev detection/renaming framework.
This has already been tested by @c-po , @alexk37 , with some corner cases identified by @alexk37 addressed.
Note that the failing smoketests are known issues corrected by the later fix to the container smoketests.
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
I could not see obvious issues and I trust the review and testing of other participants.
vyos-bot
Bot
added
mirror-initiated
4 participants
Change summary
Fixes a long-standing boot-time race where Ethernet/wireless interfaces could disappear or be renamed on systems with multiple PCIe NICs from different vendors, because naming was previously decided from a single per-device udev event before all hardware had enumerated.
Replaces that with one authoritative pass, run once configuration is available during router startup: wait for configured hardware, apply every hw-id binding, and deterministically name whatever has none yet by PCIe distance from the root complex and MAC address. An interface whose
hw-idwas deleted (NIC replacement) or whose whole configuration was removed is treated as an ordinary open slot and filled the same way, recovering its original hardware whenever the interfaces that vacated slots the same boot also show up as candidates. Boot now reports any interface still unresolved instead of failing silently, and the whole pass is covered by a new test suite.Two unrelated fixes found via code review while touching adjacent code:
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Embedded smoketests
Checklist: