Skip to content

Testsuite: T3871: change testcase to use multiple NIC drivers - #1253

Merged
dmbaturin merged 2 commits into
vyos:rollingfrom
c-po:boot-ifname-race
Aug 14, 2026
Merged

Testsuite: T3871: change testcase to use multiple NIC drivers#1253
dmbaturin merged 2 commits into
vyos:rollingfrom
c-po:boot-ifname-race

Conversation

@c-po

@c-po c-po commented Jul 25, 2026

Copy link
Copy Markdown
Member

Change summary

Extend testcase to use multiple differen virtual NIC drivers to also see if they are always placed in the same order - given my ascending MAC addresses.

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)

Checklist:

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

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved QEMU network interface validation to ensure stable eth0–eth7 MAC address mappings.
    • Updated virtual network device configurations for broader compatibility across virtualization environments.
    • Added checks to detect unexpected interface or MAC address ordering after reboot and configuration changes.
    • Enhanced validation after interface configuration updates and removal operations.
  • Testing

    • Added a dedicated interface-naming test mode for installation verification.
    • Added coverage for preserving network interface and MAC mappings across reboots.

Walkthrough

The QEMU installation checker adds an interface naming persistence test, updates NIC models for networks n4–n7, and verifies eth0–eth7 MAC ordering before and after reboot.

Changes

QEMU NIC mapping

Layer / File(s) Summary
Interface naming test entrypoints
Makefile, scripts/check-qemu-install
The Makefile adds the testifname target. The checker adds and validates --ifnametest.
NIC configuration and MAC base
scripts/check-qemu-install
macbase moves earlier in the script, its duplicate is removed, and networks n4–n7 use e1000e or vmxnet3.
MAC mapping verification
scripts/check-qemu-install
verify_eth_mac_mapping() validates eth0–eth7. The checker runs it after basic CLI tests and after interface deletion, hw-id removal, save, and reboot.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the testcase change to use multiple NIC drivers, which is the main purpose of the pull request.
Description check ✅ Passed The description accurately explains the multiple virtual NIC drivers and ascending MAC address ordering validation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code

Comment @coderabbitai help to get the list of available commands.

@mergify mergify Bot added the rolling label Jul 25, 2026
@mergify mergify Bot assigned c-po Jul 25, 2026
@c-po
c-po requested review from jestabro and natali-rs1985 July 25, 2026 14:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@scripts/check-qemu-install`:
- Around line 1095-1099: Guard the verify_eth_mac_mapping call in the test flow
with the existing --no-interfaces condition, so it is skipped when interface
tests are disabled. Preserve the current check for normal runs and use the
established option/state symbol already used by the surrounding test logic.
🪄 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: bd86c8b3-711e-4768-8d86-e0c3871bd4ee

📥 Commits

Reviewing files that changed from the base of the PR and between 35bfaa5 and 410c60d.

📒 Files selected for processing (1)
  • scripts/check-qemu-install
🔗 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. (3)
  • GitHub Check: build_iso
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🧰 Additional context used
🔍 Remote MCP

Relevant context

  • T3871 is explicitly about “unexpected interface name reordering,” with the bug description saying ethernet interfaces can change their interface name <-> hw-id association after reboot/upgrade; related history also mentions “shift index to accommodate KVM behaviour.” (vyos.dev)
  • VyOS docs describe the same failure mode: a VM with eth0eth3 can come back as eth4eth7 after migration to another host, and the suggested fix is to control MAC addresses. (docs.vyos.io)
  • VyOS testing docs say vyos-build make test launches a QEMU VM, installs the ISO, then runs /usr/bin/vyos-smoketest; interface-based tests can be constrained with TEST_ETH. So this PR affects the normal smoke-test path. (docs.vyos.io)
  • QEMU network docs show NICs are attached with explicit model/MAC pairs, including e1000e and virtio-net-pci with macaddr=. That makes a MAC-to-interface mapping check a reasonable validation point for QEMU-based tests. (qemu.org)
  • systemd-udevd uses predictable interface names by default; net.ifnames=0 disables that behavior. So any test assuming legacy ethX names is sensitive to the guest naming policy. (man7.org)
🔇 Additional comments (1)
scripts/check-qemu-install (1)

85-87: LGTM!

Also applies to: 293-296, 690-714

Comment thread scripts/check-qemu-install
sever-sever
sever-sever previously approved these changes Jul 27, 2026

@sever-sever sever-sever left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Extend tests for multiple NIC types

@mergify

mergify Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
Makefile (1)

74-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicate .ONESHELL declaration.

Line 75 duplicates an existing .ONESHELL declaration. The target has one recipe line and does not need another declaration. This removes the checkmake warning.

Proposed fix
 .PHONY: testifname
-.ONESHELL:
 testifname:
 	scripts/check-qemu-install --debug --ifnametest --iso $(ISO_PATH) $(filter-out $@,$(MAKECMDGOALS))
🤖 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 `@Makefile` around lines 74 - 77, Remove the duplicate .ONESHELL declaration
adjacent to the testifname target, preserving the existing declaration elsewhere
in the Makefile and leaving the testifname recipe unchanged.

Source: Linters/SAST tools

🤖 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 `@scripts/check-qemu-install`:
- Around line 1319-1328: Update the interface mutation flow around the
c.sendline deletion and commit/save commands to assert the selected interface
nodes and hw-id leaves exist before deletion. After applying the changes, assert
both the candidate configuration and /config/config.boot show the expected
interface and hw-id removals before reboot, rather than relying only on
verify_eth_mac_mapping.

---

Nitpick comments:
In `@Makefile`:
- Around line 74-77: Remove the duplicate .ONESHELL declaration adjacent to the
testifname target, preserving the existing declaration elsewhere in the Makefile
and leaving the testifname recipe unchanged.
🪄 Autofix

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: c51974fe-1948-4bca-a3d9-7684041e923e

📥 Commits

Reviewing files that changed from the base of the PR and between 410c60d and 5300258.

📒 Files selected for processing (2)
  • Makefile
  • scripts/check-qemu-install
🔗 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. (4)
  • GitHub Check: build_iso
  • GitHub Check: codeql-analysis-call / Analyze (python)
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🧰 Additional context used
🪛 checkmake (0.3.2)
Makefile

[warning] 76-76: Target ".ONESHELL" defined multiple times (lines 26 and 76).

(uniquetargets)

🔍 Remote MCP vyos.dev

Relevant Phorge context

  • T3871 title: “Resolve unexpected interface name reordering”; status is “Needs testing”, priority Normal, and issue type bug. Its description concerns changed interface name ↔ hw-id associations after reboot or upgrade.
  • The task’s latest comment links this work to vyos-build PR #1253 and vyos-1x PR #5350, confirming the relationship stated in the PR context.
  • Prior testing reported that native kernel ordering can interleave interfaces from different hardware sources. The same discussion says manually specifying hw-id in config.boot was respected after reboot, while post-boot renaming via configuration commands failed in one 1.5 rolling test. This supports testing both initial boot mapping and persistence after configuration changes.
  • Earlier task discussion states that basic VM tests were positive, while config-mode reordering had been artificially disabled in vyos-1x to test the interface-monitor behavior.
🔇 Additional comments (2)
scripts/check-qemu-install (1)

85-87: LGTM!

Also applies to: 143-144, 295-298, 392-405, 694-718, 1099-1103

Makefile (1)

11-11: LGTM!

Comment on lines +1319 to +1328
c.sendline('configure')
c.expect(cfg_mode_prompt)
c.sendline(f'delete interfaces ethernet eth{del_idx}')
c.expect(cfg_mode_prompt)
c.sendline(f'delete interfaces ethernet eth{hwid_idx} hw-id')
c.expect(cfg_mode_prompt)
c.sendline('commit')
c.expect(cfg_mode_prompt)
c.sendline('save')
c.expect(cfg_mode_prompt)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect interface configuration setup and the ifnametest mutation path.
rg -n -C 8 \
  'args\.ifnametest|set interfaces ethernet|delete interfaces ethernet|hw-id|show configuration commands' \
  scripts/check-qemu-install

# Find other image/bootstrap code that creates Ethernet hw-id bindings.
rg -n -C 6 \
  'set interfaces ethernet eth[0-7]|interfaces ethernet.*hw-id' \
  . -g '!scripts/check-qemu-install'

Repository: vyos/vyos-build

Length of output: 5657


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- interface test function and reboot assertions ---'
sed -n '1180,1385p' scripts/check-qemu-install

printf '%s\n' '--- all interface configuration commands in this script ---'
rg -n -C 5 \
  'interfaces ethernet|hw-id|ifname|eth0\.\.eth7|eth0\.\.eth7|random\.sample|reboot' \
  scripts/check-qemu-install || true

printf '%s\n' '--- repository references to interface bootstrap configuration ---'
rg -n -C 4 \
  'set interfaces ethernet|hw-id' . \
  -g '!scripts/check-qemu-install' \
  -g '!*.iso' -g '!*.qcow2' -g '!*.img' || true

Repository: vyos/vyos-build

Length of output: 15033


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- interface test function and reboot assertions ---'
sed -n '1180,1385p' scripts/check-qemu-install

printf '%s\n' '--- all interface configuration commands in this script ---'
rg -n -C 5 \
  'interfaces ethernet|hw-id|ifname|eth0\.\.eth7|random\.sample|reboot' \
  scripts/check-qemu-install || true

printf '%s\n' '--- repository references to interface bootstrap configuration ---'
rg -n -C 4 \
  'set interfaces ethernet|hw-id' . \
  -g '!scripts/check-qemu-install' \
  -g '!*.iso' -g '!*.qcow2' -g '!*.img' || true

Repository: vyos/vyos-build

Length of output: 15033


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- interface and image setup ---'
sed -n '430,760p' scripts/check-qemu-install

printf '%s\n' '--- installation and initial boot flow ---'
sed -n '900,1125p' scripts/check-qemu-install

printf '%s\n' '--- configuration and interface-related symbols ---'
rg -n \
  'macbase|mac[0-9]|get_qemu_cmd|install|config\.boot|verify_eth_mac_mapping|interfaces ethernet|hw-id' \
  scripts/check-qemu-install

printf '%s\n' '--- current change summary ---'
git diff --stat
git status --short

Repository: vyos/vyos-build

Length of output: 26493


🌐 Web query:

VyOS install image automatically creates interfaces ethernet eth0 hw-id config.boot

💡 Result:

In VyOS, the hw-id parameter within the interfaces ethernet configuration is not automatically generated or injected into config.boot by the standard install image process [1]. The hw-id (Hardware ID) serves as a persistent mapping mechanism that ties a specific logical interface (e.g., eth0) to a physical network card identified by its MAC address [2][3]. This is particularly useful in virtualized or dynamic environments to ensure that network interface names remain consistent across reboots, even if the system's hardware discovery order changes [4][3]. Key points regarding hw-id and configuration: * Manual Configuration: Administrators typically define hw-id manually using CLI commands such as set interfaces ethernet eth0 hw-id 'XX:XX:XX:XX:XX:XX' [5][6]. * Automation Scenarios: While the standard installation does not auto-populate this, automated provisioning tools like cloud-init can be configured to inject hw-id settings into the configuration during the initial deployment [7]. * Renaming Consequences: If an hw-id is configured incorrectly or points to a non-existent MAC address, the system may fail to initialize the interface, often leading to interface renumbering or configuration load errors upon reboot [2][3]. * Distinction from MAC Spoofing: It is important to distinguish hw-id from the mac command; hw-id is for interface-to-hardware mapping [3], whereas mac (where supported) is used to change the MAC address presented by the interface [3]. If you find hw-id entries in your config.boot that you did not manually add, they may have been introduced by an automated provisioning script, cloud-init configuration, or a previous system setup that included these parameters in the configuration template [7][8][9].

Citations:


🌐 Web query:

site:github.com/vyos/vyos-1x "install image" "hw-id"

💡 Result:

The term "hw-id" in the context of VyOS is not a command, but rather a configuration node used to associate a physical network interface (Ethernet or wireless) with its specific hardware MAC address [1][2][3]. By defining the hw-id under an interface configuration (e.g., set interfaces ethernet eth0 hw-id 'xx:xx:xx:xx:xx:xx'), you ensure that the system consistently assigns the name (e.g., eth0) to the physical network card with that specific MAC address, regardless of the boot order or the presence of other hardware [3][4]. This mechanism is crucial in VyOS for predictable network interface naming [4]. If an interface is renamed or replaced, updating or deleting the hw-id entry is a common administrative task to allow the system to re-identify and correctly assign the interface name [3][4]. There is no "install image hw-id" command; if you encountered such a phrase, it likely refers to setting the hw-id during the initial installation or configuration of a network interface within a VyOS system image [5][2].

Citations:


🌐 Web query:

site:github.com/vyos/vyos-1x "hw-id" "config.boot"

💡 Result:

In VyOS, the hw-id configuration node is used to associate a specific network interface (such as an ethernet or wireless interface) with a particular Media Access Control (MAC) address [1]. This configuration, found within the config.boot file, ensures that the system assigns the desired interface name (e.g., eth0) to the physical hardware that possesses the specified MAC address, regardless of the order in which the kernel detects the interfaces at boot time [2][3][4]. When you define an interface with an hw-id, the system uses this identifier to perform interface renaming reliably [4]. This prevents issues where interface names might swap on reboot if multiple network interfaces are present or if hardware is added or removed [4]. Example of the hw-id configuration in config.boot: interfaces { ethernet eth0 { hw-id "00:11:22:33:44:55" } } The system processes these entries during the boot sequence—specifically, mechanisms like vyos-net-name-resolve or similar helper scripts read this configuration to map the MAC address to the intended interface name [2][4]. If an interface is not explicitly configured with an hw-id, the system uses other methods for naming, which may be less predictable across different hardware configurations [4].

Citations:


🔍 Additional research query:

Search vyos.dev documentation and issue discussions for whether install imagecreatesinterfaces ethernet ethNnodes orhw-identries inconfig.boot, and whether VyOS delete is a no-op when the requested node or leaf is absent.

💡 Result:

Findings

install image and config.boot

  • VyOS documentation shows installed systems mapping logical interfaces such as eth0, eth1, etc. to MAC addresses, but does not explicitly state that install image writes those entries to config.boot.
  • VyOS issue T577 explicitly reports that a fresh installation discovers Ethernet interfaces and adds them to the configuration. A follow-up states that, with an empty config.boot, each boot added one new Ethernet interface until all were discovered.
  • T1499 states that the interface-renaming logic stores the NIC-to-MAC mapping in hw-id tags immediately before VyOS loads the configuration.

Conclusion: The documented issue evidence indicates that the discovery process can create both:

interfaces {
    ethernet ethN {
        hw-id <MAC>
    }
}

That is, interfaces ethernet ethN nodes and their hw-id leaves—not just one or the other. The evidence is historical and does not guarantee identical behavior in every current release.

delete when the target is absent

  • Current VyOS documentation says REST API strict mode rejects a delete when the target path does not exist; without strict mode, such deletes can succeed silently.
  • The documentation explicitly describes this behavior for the HTTPS /configure API, not necessarily for the interactive CLI.
  • T5991 shows a separate boot-verification issue where missing interfaces caused generated delete operations and entire configuration sections to be omitted. This is not evidence that an interactive CLI delete is inherently destructive or that it behaves identically to REST API deletion.

Conclusion: For the REST API, an absent-path delete is effectively a no-op by default; enable REST strict mode to reject it. The retrieved sources do not conclusively establish the exact absent-target behavior of the interactive CLI.


🏁 Script executed (no clone):

#!/bin/bash
set -euo pipefail

for url in \
  'https://raw.githubusercontent.com/vyos/vyos-1x/f01423cdf32be5ce5344c00d570c80bdbce6eda9/src/helpers/vyos_net_name' \
  'https://raw.githubusercontent.com/vyos/vyos-1x/a5654ac187681f802a356090390d4b66355ee8a5/src/op_mode/image_installer.py'
do
  echo "--- $url ---"
  curl -fsSL "$url" | rg -n -C 6 'hw-id|config\.boot|interfaces|rename|load' || true
done

printf '%s\n' '--- issue references ---'
for url in \
  'https://vyos.dev/T577' \
  'https://vyos.dev/T1499' \
  'https://vyos.dev/T5991'
do
  echo "--- $url ---"
  curl -fsSL "$url" | rg -n -i -C 3 'hw-id|config\.boot|interface|delete|discover' || true
done

Length of output: 50344


🏁 Script executed (no clone):

#!/bin/bash
set -euo pipefail

url='https://raw.githubusercontent.com/vyos/vyos-1x/f01423cdf32be5ce5344c00d570c80bdbce6eda9/src/helpers/vyos_net_name'
echo "--- $url ---"
curl -fsSL "$url" | grep -n -E -C 8 'hw-id|config\.boot|interfaces|rename' | head -n 160

url='https://raw.githubusercontent.com/vyos/vyos-1x/a5654ac187681f802a356090390d4b66355ee8a5/src/op_mode/image_installer.py'
echo "--- $url ---"
curl -fsSL "$url" | grep -n -E -C 8 'hw-id|config\.boot|interfaces|rename|load' | head -n 240

Length of output: 4874


🏁 Script executed (no clone):

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- vyos_net_name assignment and persistence paths ---'
curl -fsSL \
  'https://raw.githubusercontent.com/vyos/vyos-1x/f01423cdf32be5ce5344c00d570c80bdbce6eda9/src/helpers/vyos_net_name' |
  sed -n '70,245p'

printf '%s\n' '--- installer file locations ---'
curl -fsSL \
  'https://api.github.com/repos/vyos/vyos-1x/git/trees/current?recursive=1' |
  jq -r '.tree[]?.path' |
  grep -E 'image_installer|install.*image|installer' |
  head -n 80

Length of output: 5964


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path

path = Path("scripts/check-qemu-install")
lines = path.read_text().splitlines()

start = next(i for i, line in enumerate(lines) if "elif args.ifnametest:" in line)
end = next(i for i in range(start + 1, len(lines))
           if lines[i].startswith("    elif args.") or lines[i].startswith("    else:"))
block = lines[start:end]

commands = [
    line.strip()
    for line in block
    if "c.sendline(" in line
]
assertions = [
    line.strip()
    for line in block
    if any(token in line for token in (
        "show configuration", "compare", "config.boot", "assert", "raise"
    ))
]

print("ifnametest commands:")
print("\n".join(commands))
print("\nconfiguration-state checks:")
print("\n".join(assertions) or "<none>")

assert any("delete interfaces ethernet" in line for line in commands)
assert any("'save'" in line for line in commands)
assert not assertions
assert commands[-1].startswith("c.sendline('reboot now')")
PY

Repository: vyos/vyos-build

Length of output: 435


Assert the interface configuration mutations before reboot. At scripts/check-qemu-install:1321-1327, assert that the selected interface nodes and hw-id leaves exist before deletion, then assert that the candidate and /config/config.boot contain the expected removals. verify_eth_mac_mapping checks only runtime MACs, so no-op deletes can make the reboot test pass without testing persistence.

🤖 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 `@scripts/check-qemu-install` around lines 1319 - 1328, Update the interface
mutation flow around the c.sendline deletion and commit/save commands to assert
the selected interface nodes and hw-id leaves exist before deletion. After
applying the changes, assert both the candidate configuration and
/config/config.boot show the expected interface and hw-id removals before
reboot, rather than relying only on verify_eth_mac_mapping.

Source: MCP tools

c-po added 2 commits August 6, 2026 21:54
Extend testcase to use multiple differen virtual NIC drivers to also see if
they are always placed in the same order - given my ascending MAC addresses.
Testcase removes NIC definition entirely and on a second NIC drops only
the hw-id node. Testcase is there to verify proper ordering which simulates
a NIC replacement.
@c-po
c-po force-pushed the boot-ifname-race branch from 5300258 to 90149ea Compare August 6, 2026 19:55
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
Makefile (1)

74-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicate .ONESHELL declaration.

Makefile already declares .ONESHELL at Line 26. The declaration at Line 75 is redundant and triggers the checkmake warning. Keep the existing declaration and retain .PHONY: testifname.

🤖 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 `@Makefile` around lines 74 - 75, Remove the duplicate .ONESHELL declaration
near the testifname target, while preserving the existing .ONESHELL declaration
and the .PHONY: testifname declaration.

Source: Linters/SAST tools

🤖 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 `@Makefile`:
- Around line 74-75: Remove the duplicate .ONESHELL declaration near the
testifname target, while preserving the existing .ONESHELL declaration and the
.PHONY: testifname declaration.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 9f3b5549-1a95-4d4e-8116-b3a178f57524

📥 Commits

Reviewing files that changed from the base of the PR and between 7633417 and 90149ea.

📒 Files selected for processing (2)
  • Makefile
  • scripts/check-qemu-install
🔗 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)
  • scripts/check-qemu-install
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🧰 Additional context used
🪛 checkmake (0.3.2)
Makefile

[warning] 76-76: Target ".ONESHELL" defined multiple times (lines 26 and 76).

(uniquetargets)

🔍 Remote MCP vyos.dev

Relevant review context

  • T3871 title: “Resolve unexpected interface name reordering”; status: “Needs testing”; priority: Normal; issue type: bug. It tracks changes in interface name ↔ hw-id associations after reboot or upgrade.
  • The task’s latest comment directly links this work to vyos-build PR #1253 and vyos-1x PR #5350.
  • Testing history reports that kernel interface ordering can interleave NICs from different hardware sources. Manually specifying hw-id in config.boot preserved the desired order after reboot, while post-boot renaming via configuration commands failed in one 1.5 rolling test.
  • Earlier testing found basic VM tests positive and noted that config-mode reordering had been artificially disabled in vyos-1x while testing interface-monitor behavior.
🔇 Additional comments (2)
Makefile (2)

11-11: LGTM!


76-77: LGTM!

@github-actions

Copy link
Copy Markdown

CI integration ❌ failed!

Details

CI logs

  • Config tests ❌ failed

@dmbaturin dmbaturin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with the motivation, the implementation looks fine to me as well.

@dmbaturin
dmbaturin merged commit aee889f into vyos:rolling Aug 14, 2026
13 of 18 checks passed
@vyos-bot vyos-bot Bot added mirror-initiated This PR initiated for mirror sync workflow mirror-failed This PR mirror has failed and removed mirror-initiated This PR initiated for mirror sync workflow labels Aug 14, 2026
@c-po
c-po deleted the boot-ifname-race branch August 14, 2026 19:07
@vyos-bot vyos-bot Bot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow mirror-failed This PR mirror has failed labels Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants