Testsuite: T8602: add QEMU smoketest for add-system-image over HTTP (default + VRF) - #1210
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a test-image-update mode that boots a VM with a second CD-ROM (nested payload ISO) and runs HTTP-based image add/delete cycles; removes Makefile checkiso prerequisite; restructures Docker QEMU package installs and tightens CLI/ISO validation (see scripts/check-qemu-install:57-72,681-790; Makefile:15-62). ChangesNested Installer Payload Image Updates
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🧹 Nitpick comments (1)
docker/Dockerfile (1)
333-336: 💤 Low valueConsider adding checksum verification for syft binary
The binary is downloaded from
cdn.vyos.iowithout integrity verification. While this is a controlled CDN, supply-chain best practice is to verify checksums for downloaded binaries.This is consistent with other downloads in this Dockerfile (open-vmdk, debootstrap patches), so flagging as optional.
🤖 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 `@docker/Dockerfile` around lines 333 - 336, The Dockerfile currently downloads syft.tar.gz and installs the syft binary without integrity checks; update the RUN sequence that fetches syft.tar.gz to also download or embed the expected checksum (e.g., syft_1.44.0_linux_* .sha256 or a known SHA256 string), verify the archive with sha256sum (or sha256sum -c) before extracting, and abort the build on mismatch; ensure the verification step sits between the curl -o syft.tar.gz and tar/mv commands so the RUN that references syft.tar.gz, syft and /usr/local/bin only proceeds if the checksum matches.
🤖 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 `@docker/Dockerfile`:
- Line 31: Fix the typo in the Dockerfile comment: replace the comment text "#
Example bo build natively:" with the correct wording ("# Example to build
natively:" or similar) so the comment reads "to build" instead of "bo build";
update the exact comment line shown in the diff to correct the spelling.
- Line 19: Update the inline comment in the Dockerfile that currently reads "#
This Dockerfile is installable on x86-64 (amd64) and aarch64 (amd64)" to use the
correct architecture name for aarch64 by replacing the second "amd64" with
"arm64" so it reads "...aarch64 (arm64)"; locate the comment string in the
Dockerfile and correct the typo.
In `@scripts/check-qemu-install`:
- Around line 468-482: The variable nested_payload_iso_path is only set inside
the args.test_image_update branch causing NameError when referenced later;
initialize nested_payload_iso_path (e.g., to None or to
os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) if that makes sense) before the if
args.test_image_update: block so it exists regardless of the flag, and ensure
subsequent code that uses nested_payload_iso_path handles the None/default value
appropriately; look for references to nested_payload_iso_path and the
NESTED_INSTALLER_PAYLOAD_ISO/NESTED_ISO_DATA_DIR logic to confirm correct
default behavior.
In `@scripts/image-build/build-vyos-image`:
- Around line 732-748: You shadowed the imported cmd function by assigning cmd =
[[...]] for the Syft/sed commands, which prevents later calls to the utils.cmd
function (e.g., cmd(f"qemu-img convert...")) and causes a TypeError; rename the
local variable (e.g., syft_cmds or syft_commands) used to build/iterate the syft
and sed command lists so the imported cmd function remains callable, and in the
subprocess.Popen loop (the block using p and p.wait()) check p.returncode (or
the result of p.wait()) after the process completes and log/raise on non-zero
exit so failures aren’t silently ignored. Ensure references to the syft list use
the new name and do not modify the imported cmd symbol.
In `@scripts/iso-to-oci`:
- Around line 25-27: The ISO existence check echoes an error but doesn't stop
execution; update the if block that checks [ ! -f "$ISO" ] to exit with a
non-zero status (e.g., exit 1) immediately after the echo so the script
terminates instead of continuing to mount/unsquashfs and producing confusing
downstream errors.
---
Nitpick comments:
In `@docker/Dockerfile`:
- Around line 333-336: The Dockerfile currently downloads syft.tar.gz and
installs the syft binary without integrity checks; update the RUN sequence that
fetches syft.tar.gz to also download or embed the expected checksum (e.g.,
syft_1.44.0_linux_* .sha256 or a known SHA256 string), verify the archive with
sha256sum (or sha256sum -c) before extracting, and abort the build on mismatch;
ensure the verification step sits between the curl -o syft.tar.gz and tar/mv
commands so the RUN that references syft.tar.gz, syft and /usr/local/bin only
proceeds if the checksum matches.
🪄 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 Plus
Run ID: 30b265e3-5d14-423d-a02b-02426a03da97
📒 Files selected for processing (6)
.gitignoreMakefiledocker/Dockerfilescripts/check-qemu-installscripts/image-build/build-vyos-imagescripts/iso-to-oci
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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] 22-22: Target ".ONESHELL" defined multiple times (lines 17 and 22).
(uniquetargets)
[warning] 27-27: Target ".ONESHELL" defined multiple times (lines 17 and 27).
(uniquetargets)
[warning] 32-32: Target ".ONESHELL" defined multiple times (lines 17 and 32).
(uniquetargets)
[warning] 37-37: Target ".ONESHELL" defined multiple times (lines 17 and 37).
(uniquetargets)
[warning] 42-42: Target ".ONESHELL" defined multiple times (lines 17 and 42).
(uniquetargets)
[warning] 47-47: Target ".ONESHELL" defined multiple times (lines 17 and 47).
(uniquetargets)
[warning] 52-52: Target ".ONESHELL" defined multiple times (lines 17 and 52).
(uniquetargets)
[warning] 57-57: Target ".ONESHELL" defined multiple times (lines 17 and 57).
(uniquetargets)
[warning] 62-62: Target ".ONESHELL" defined multiple times (lines 17 and 62).
(uniquetargets)
[warning] 77-77: Target ".ONESHELL" defined multiple times (lines 17 and 77).
(uniquetargets)
[warning] 82-82: Target ".ONESHELL" defined multiple times (lines 17 and 82).
(uniquetargets)
[warning] 87-87: Target ".ONESHELL" defined multiple times (lines 17 and 87).
(uniquetargets)
🪛 Trivy (0.69.3)
docker/Dockerfile
[error] 257-260: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y python3-pexpect qemu-utils qemu-kvm'
Rule: DS-0029
(IaC/Dockerfile)
[error] 263-267: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iamd64; then apt-get update && apt-get install -y ovmf qemu-system-x86; fi'
Rule: DS-0029
(IaC/Dockerfile)
[error] 270-274: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iarm64; then apt-get update && apt-get install -y qemu-system-aarch64 qemu-efi-aarch64; fi'
Rule: DS-0029
(IaC/Dockerfile)
🔍 Remote MCP Context7
Additional Context for PR Review
Syft SBOM Generation Tool
The PR integrates Syft (version 1.44.0) for automated Software Bill of Materials (SBOM) generation. Key points from the documentation:
Syft is a CLI tool for generating SBOMs from container images and filesystems, supporting multiple output formats including CycloneDX JSON and SPDX JSON. The PR's implementation in scripts/image-build/build-vyos-image aligns with standard Syft usage patterns:
- Syft can generate multiple SBOM formats simultaneously using the
-oflag, such as-o spdx-json=sbom.spdx.json -o cyclonedx-json=sbom.cdx.json - The Dockerfile's pinned version (
syft_1.44.0) ensures reproducible builds
QEMU Multi-CD-ROM Configuration
The PR's key feature is adding nested ISO support with a second CD-ROM for image updates. From QEMU documentation:
QEMU supports multiple drives using the -drive option with parameters like media=cdrom to specify CD-ROM drives and index to order them. The script change from transfer_disk to nested_cdrom_iso parameter aligns with this model, allowing QEMU to attach multiple CD-ROM devices (drive-cd1 for the main installer ISO and drive-cd2 for the nested payload ISO).
QEMU also supports remote ISO images via HTTP using -drive media=cdrom,file.driver=http,file.url=..., which is relevant to the PR's HTTP-based image update functionality.
Test Architecture Changes
Key removal noted: The checkiso prerequisite is being dropped from all test targets. This implies:
- ISO existence validation is now handled individually within scripts (e.g.,
scripts/iso-to-ocinow validates the ISO path) - The build flow has shifted away from a centralized dependency check to distributed validation
Docker Architecture Changes
The Dockerfile updates reflect narrowing cross-architecture support:
- Removal of
armhfandi386branches - Architecture-specific QEMU package installation (amd64 gets
ovmfandqemu-system-x86, arm64 getsqemu-aarch64andqemu-efi-aarch64) - Vendor label change from "Sentrium S.L." to "VyOS Networks",
🔇 Additional comments (19)
docker/Dockerfile (1)
256-274: LGTM!The static analysis hints about missing
--no-install-recommendsare false positives—line 51 already configuresAPT::Install-Recommends "0"globally via apt.conf.d.scripts/image-build/build-vyos-image (2)
724-727: LGTM!
33-33: LGTM!.gitignore (1)
14-14: LGTM!Makefile (4)
17-63: LGTM!
75-78: LGTM!
82-88: LGTM!
113-113: LGTM!scripts/check-qemu-install (11)
58-71: LGTM!
209-210: LGTM!Also applies to: 248-269
381-410: LGTM!
435-441: LGTM!
453-453: LGTM!
354-357: LGTM!
649-667: LGTM!
681-786: LGTM!
1441-1463: LGTM!
1502-1510: LGTM!
1530-1530: LGTM!
| # volume mapped in order to easy export images | ||
|
|
||
| # This Dockerfile is installable on both x86, x86-64, armhf and arm64 systems | ||
| # This Dockerfile is installable on x86-64 (amd64) and aarch64 (amd64) systems |
There was a problem hiding this comment.
Typo: "amd64" should be "arm64"
The comment says "aarch64 (amd64)" but should be "aarch64 (arm64)".
-# This Dockerfile is installable on x86-64 (amd64) and aarch64 (amd64) systems
+# This Dockerfile is installable on x86-64 (amd64) and aarch64 (arm64) systems📝 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.
| # This Dockerfile is installable on x86-64 (amd64) and aarch64 (amd64) systems | |
| # This Dockerfile is installable on x86-64 (amd64) and aarch64 (arm64) systems |
🤖 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 `@docker/Dockerfile` at line 19, Update the inline comment in the Dockerfile
that currently reads "# This Dockerfile is installable on x86-64 (amd64) and
aarch64 (amd64)" to use the correct architecture name for aarch64 by replacing
the second "amd64" with "arm64" so it reads "...aarch64 (arm64)"; locate the
comment string in the Dockerfile and correct the typo.
| if args.test_image_update: | ||
| if os.path.isdir(NESTED_ISO_DATA_DIR): | ||
| shutil.rmtree(NESTED_ISO_DATA_DIR) | ||
| os.makedirs(NESTED_ISO_DATA_DIR) | ||
| shutil.copy2(args.iso, os.path.join(NESTED_ISO_DATA_DIR, NESTED_INNER_ISO_NAME)) | ||
| if os.path.isfile(NESTED_INSTALLER_PAYLOAD_ISO): | ||
| os.unlink(NESTED_INSTALLER_PAYLOAD_ISO) | ||
| log.info('Assembling nested installer payload ISO (second CD-ROM, drive-cd2)') | ||
| subprocess.check_call([ | ||
| 'mkisofs', '-joliet', '-rock', '-volid', 'VYOSNESTED', | ||
| '-output', NESTED_INSTALLER_PAYLOAD_ISO, NESTED_ISO_DATA_DIR, | ||
| ]) | ||
| nested_payload_iso_path = os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) | ||
| log.info('Removing nested ISO staging directory %s', NESTED_ISO_DATA_DIR) | ||
| shutil.rmtree(NESTED_ISO_DATA_DIR) |
There was a problem hiding this comment.
Initialize nested_payload_iso_path before conditional block to prevent NameError.
nested_payload_iso_path is defined only when args.test_image_update is True (line 480), but referenced unconditionally at lines 790 and 849. This will raise NameError when --qemu-cmd is used without --test-image-update, or in the main installation path without --test-image-update.
🐛 Proposed fix
gen_disk(args.disk)
+nested_payload_iso_path = None
if args.test_image_update:
if os.path.isdir(NESTED_ISO_DATA_DIR):📝 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.
| if args.test_image_update: | |
| if os.path.isdir(NESTED_ISO_DATA_DIR): | |
| shutil.rmtree(NESTED_ISO_DATA_DIR) | |
| os.makedirs(NESTED_ISO_DATA_DIR) | |
| shutil.copy2(args.iso, os.path.join(NESTED_ISO_DATA_DIR, NESTED_INNER_ISO_NAME)) | |
| if os.path.isfile(NESTED_INSTALLER_PAYLOAD_ISO): | |
| os.unlink(NESTED_INSTALLER_PAYLOAD_ISO) | |
| log.info('Assembling nested installer payload ISO (second CD-ROM, drive-cd2)') | |
| subprocess.check_call([ | |
| 'mkisofs', '-joliet', '-rock', '-volid', 'VYOSNESTED', | |
| '-output', NESTED_INSTALLER_PAYLOAD_ISO, NESTED_ISO_DATA_DIR, | |
| ]) | |
| nested_payload_iso_path = os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) | |
| log.info('Removing nested ISO staging directory %s', NESTED_ISO_DATA_DIR) | |
| shutil.rmtree(NESTED_ISO_DATA_DIR) | |
| nested_payload_iso_path = None | |
| if args.test_image_update: | |
| if os.path.isdir(NESTED_ISO_DATA_DIR): | |
| shutil.rmtree(NESTED_ISO_DATA_DIR) | |
| os.makedirs(NESTED_ISO_DATA_DIR) | |
| shutil.copy2(args.iso, os.path.join(NESTED_ISO_DATA_DIR, NESTED_INNER_ISO_NAME)) | |
| if os.path.isfile(NESTED_INSTALLER_PAYLOAD_ISO): | |
| os.unlink(NESTED_INSTALLER_PAYLOAD_ISO) | |
| log.info('Assembling nested installer payload ISO (second CD-ROM, drive-cd2)') | |
| subprocess.check_call([ | |
| 'mkisofs', '-joliet', '-rock', '-volid', 'VYOSNESTED', | |
| '-output', NESTED_INSTALLER_PAYLOAD_ISO, NESTED_ISO_DATA_DIR, | |
| ]) | |
| nested_payload_iso_path = os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) | |
| log.info('Removing nested ISO staging directory %s', NESTED_ISO_DATA_DIR) | |
| shutil.rmtree(NESTED_ISO_DATA_DIR) |
🤖 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 468 - 482, The variable
nested_payload_iso_path is only set inside the args.test_image_update branch
causing NameError when referenced later; initialize nested_payload_iso_path
(e.g., to None or to os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) if that makes
sense) before the if args.test_image_update: block so it exists regardless of
the flag, and ensure subsequent code that uses nested_payload_iso_path handles
the None/default value appropriately; look for references to
nested_payload_iso_path and the NESTED_INSTALLER_PAYLOAD_ISO/NESTED_ISO_DATA_DIR
logic to confirm correct default behavior.
| cmd = [['syft', syft_target_dir, | ||
| '--source-name', 'VyOS', '--source-version', version, | ||
| '-o', f'cyclonedx-json=vyos-{version}.cdx.json', | ||
| '-o', f'spdx-json=vyos-{version}.spdx.json']] | ||
|
|
||
| # syft bug for CycloneDX https://github.com/anchore/syft/issues/4592#issuecomment-4567247328 | ||
| cmd.append(['sed', '-i', '-e', f's@{syft_base_path}@@g', f'vyos-{version}.cdx.json']) | ||
| cmd.append(['sed', '-i', '-e', f's@{syft_base_path}@//@g', f'vyos-{version}.spdx.json']) | ||
|
|
||
| for c in cmd: | ||
| print(c) | ||
| with subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, | ||
| text=True, bufsize=1) as p: | ||
| for line in p.stdout: | ||
| sys.stdout.write(line) | ||
| sys.stdout.flush() | ||
| p.wait() |
There was a problem hiding this comment.
Critical: Variable cmd shadows imported function, breaking non-ISO builds
Line 732 assigns cmd = [[...]], shadowing the cmd function imported from utils at line 141. After this block executes, line 765 attempts cmd(f"qemu-img convert...") which will raise TypeError: 'list' object is not callable.
This breaks all non-ISO image format builds (raw, qcow2, etc.) when building from scratch rather than reusing an ISO.
Additionally, subprocess failures are silently ignored—p.wait() return code is never checked.
🐛 Proposed fix
# Now create SBOM
syft_target_dir = 'chroot'
syft_base_path = os.getcwd() + f'/{syft_target_dir}'
- cmd = [['syft', syft_target_dir,
+ sbom_cmds = [['syft', syft_target_dir,
'--source-name', 'VyOS', '--source-version', version,
'-o', f'cyclonedx-json=vyos-{version}.cdx.json',
'-o', f'spdx-json=vyos-{version}.spdx.json']]
# syft bug for CycloneDX https://github.com/anchore/syft/issues/4592#issuecomment-4567247328
- cmd.append(['sed', '-i', '-e', f's@{syft_base_path}@@g', f'vyos-{version}.cdx.json'])
- cmd.append(['sed', '-i', '-e', f's@{syft_base_path}`@//`@g', f'vyos-{version}.spdx.json'])
+ sbom_cmds.append(['sed', '-i', '-e', f's@{syft_base_path}@@g', f'vyos-{version}.cdx.json'])
+ sbom_cmds.append(['sed', '-i', '-e', f's@{syft_base_path}`@//`@g', f'vyos-{version}.spdx.json'])
- for c in cmd:
+ for c in sbom_cmds:
print(c)
with subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
text=True, bufsize=1) as p:
for line in p.stdout:
sys.stdout.write(line)
sys.stdout.flush()
- p.wait()
+ ret = p.wait()
+ if ret != 0:
+ print(f'W: SBOM command failed with exit code {ret}: {c}')🤖 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/image-build/build-vyos-image` around lines 732 - 748, You shadowed
the imported cmd function by assigning cmd = [[...]] for the Syft/sed commands,
which prevents later calls to the utils.cmd function (e.g., cmd(f"qemu-img
convert...")) and causes a TypeError; rename the local variable (e.g., syft_cmds
or syft_commands) used to build/iterate the syft and sed command lists so the
imported cmd function remains callable, and in the subprocess.Popen loop (the
block using p and p.wait()) check p.returncode (or the result of p.wait()) after
the process completes and log/raise on non-zero exit so failures aren’t silently
ignored. Ensure references to the syft list use the new name and do not modify
the imported cmd symbol.
There is no need in keeping the old --configd option when starting the smoketests. VyOS heavily relies on vyos-configd and derivates, so there is no need anymore to disable it for our smoketests.
Abusing cats is a prevalent problem in the Linux user community. Users, ranging from know-nothing neophytes to all-knowing professionals, engage in cat abuse on a daily basis, whether in their scripts or simple terminal commands. https://rseragon.github.io/posts/cat-abuse/
Sometimes I do call scripts/check-qemu-install directly without the Makefile wrapper, thus it should be ensured that the ISO image exists also from within the test-case launcher, too.
…default + VRF) Extend check-qemu-install with --test-image-update: build a nested installer ISO as a second CD-ROM, serve the inner image over HTTP, and exercise add/delete system image in the default routing context and in a named VRF. Add make test-image-update and ignore/clean nested_installer_payload.iso artefacts.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker/Dockerfile (1)
333-336: ⚡ Quick winConsider adding checksum verification for syft download.
The binary is downloaded without integrity verification. For a supply-chain security tool used in SBOM generation, consider adding SHA256 validation:
RUN cd /tmp && curl -sSfL -o syft.tar.gz \ - https://cdn.vyos.io/tools/syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz; \ - tar --extract --file=syft.tar.gz syft; mv syft /usr/local/bin/ + https://cdn.vyos.io/tools/syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz && \ + echo "<expected_sha256> syft.tar.gz" | sha256sum -c - && \ + tar --extract --file=syft.tar.gz syft && mv syft /usr/local/bin/This follows the same trust model issue as Go (line 251), but given syft's role in SBOM integrity, pinning its checksum adds defense-in-depth.
🤖 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 `@docker/Dockerfile` around lines 333 - 336, The RUN step that downloads and installs syft currently lacks integrity checks; update the Dockerfile RUN that fetches syft.tar.gz (the syft download block) to also fetch or embed the expected SHA256, verify the downloaded archive before extracting (e.g., use sha256sum -c or echo "<EXPECTED_SHA256> syft.tar.gz" | sha256sum -c -) and abort the build on mismatch, and only proceed to tar --extract and mv syft /usr/local/bin/ if the checksum verification succeeds.
🤖 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 `@docker/Dockerfile`:
- Around line 333-336: The RUN step that downloads and installs syft currently
lacks integrity checks; update the Dockerfile RUN that fetches syft.tar.gz (the
syft download block) to also fetch or embed the expected SHA256, verify the
downloaded archive before extracting (e.g., use sha256sum -c or echo
"<EXPECTED_SHA256> syft.tar.gz" | sha256sum -c -) and abort the build on
mismatch, and only proceed to tar --extract and mv syft /usr/local/bin/ if the
checksum verification succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 847f8193-72fb-4efe-a00d-a55a33ec78e8
📒 Files selected for processing (5)
.gitignoreMakefiledocker/Dockerfilescripts/check-qemu-installscripts/iso-to-oci
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/iso-to-oci
- scripts/check-qemu-install
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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] 22-22: Target ".ONESHELL" defined multiple times (lines 17 and 22).
(uniquetargets)
[warning] 27-27: Target ".ONESHELL" defined multiple times (lines 17 and 27).
(uniquetargets)
[warning] 32-32: Target ".ONESHELL" defined multiple times (lines 17 and 32).
(uniquetargets)
[warning] 37-37: Target ".ONESHELL" defined multiple times (lines 17 and 37).
(uniquetargets)
[warning] 42-42: Target ".ONESHELL" defined multiple times (lines 17 and 42).
(uniquetargets)
[warning] 47-47: Target ".ONESHELL" defined multiple times (lines 17 and 47).
(uniquetargets)
[warning] 52-52: Target ".ONESHELL" defined multiple times (lines 17 and 52).
(uniquetargets)
[warning] 57-57: Target ".ONESHELL" defined multiple times (lines 17 and 57).
(uniquetargets)
[warning] 62-62: Target ".ONESHELL" defined multiple times (lines 17 and 62).
(uniquetargets)
[warning] 77-77: Target ".ONESHELL" defined multiple times (lines 17 and 77).
(uniquetargets)
[warning] 82-82: Target ".ONESHELL" defined multiple times (lines 17 and 82).
(uniquetargets)
[warning] 87-87: Target ".ONESHELL" defined multiple times (lines 17 and 87).
(uniquetargets)
🪛 Trivy (0.69.3)
docker/Dockerfile
[error] 257-260: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y python3-pexpect qemu-utils qemu-kvm'
Rule: DS-0029
(IaC/Dockerfile)
[error] 263-267: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iamd64; then apt-get update && apt-get install -y ovmf qemu-system-x86; fi'
Rule: DS-0029
(IaC/Dockerfile)
[error] 270-274: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iarm64; then apt-get update && apt-get install -y qemu-system-aarch64 qemu-efi-aarch64; fi'
Rule: DS-0029
(IaC/Dockerfile)
🔇 Additional comments (10)
docker/Dockerfile (5)
19-19: Typo: "amd64" should be "arm64"The comment says "aarch64 (amd64)" but should be "aarch64 (arm64)".
-# This Dockerfile is installable on x86-64 (amd64) and aarch64 (amd64) systems +# This Dockerfile is installable on x86-64 (amd64) and aarch64 (arm64) systems
31-31: Typo: "bo build" should be "to build"-# Example bo build natively: +# Example to build natively:
44-44: LGTM!
256-274: LGTM!The static analysis flags about
--no-install-recommendsare false positives since line 51 setsAPT::Install-Recommends "0"globally.
276-283: LGTM!Makefile (5)
17-63: LGTM!Removal of
checkisodependency and--configdflag is consistent across all test targets. ISO validation has been moved intoscripts/check-qemu-installper the PR objectives.
75-78: 💤 Low valueVerify if
--uefiflag should be included.Unlike other smoketest targets (
test,test-no-interfaces, etc.), this target omits--uefi. Is this intentional to test legacy BIOS boot, or should it align with other test targets?test-image-update: - scripts/check-qemu-install --debug --test-image-update --iso $(ISO_PATH) $(filter-out $@,$(MAKECMDGOALS)) + scripts/check-qemu-install --debug --test-image-update --uefi --iso $(ISO_PATH) $(filter-out $@,$(MAKECMDGOALS))
82-83: LGTM!
87-88: LGTM!
113-113: LGTM!Cleanup extended to remove nested ISO test artifacts (
nested_iso_data,nested_installer_payload.iso), consistent with the newtest-image-updateworkflow.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
scripts/check-qemu-install (3)
787-792:⚠️ Potential issue | 🔴 CriticalNameError:
nested_payload_iso_pathreferenced before definition.See critical issue flagged at lines 468-482. This invocation will fail when
--qemu-cmdis used without--test-image-update.🤖 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 787 - 792, The call to get_qemu_cmd inside the args.qemu_cmd branch references nested_payload_iso_path which is only set when --test-image-update is used, causing a NameError; fix by ensuring nested_payload_iso_path is always defined before use (e.g., initialize nested_payload_iso_path = None earlier) or by passing a conditional value into get_qemu_cmd (use nested_payload_iso_path if set else None) so get_qemu_cmd(...) is never given an undefined variable; update the args.qemu_cmd block to use that defined/conditional value and keep the get_qemu_cmd and args.qemu_cmd symbols unchanged.
847-849:⚠️ Potential issue | 🔴 CriticalNameError:
nested_payload_iso_pathreferenced before definition.See critical issue flagged at lines 468-482. This invocation will fail for all primary modes except
--test-image-update.🤖 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 847 - 849, The call to get_qemu_cmd uses nested_payload_iso_path which is undefined, causing a NameError; locate the code paths that set nested_payload_iso_path (see the test-image-update branch around lines 468-482) and ensure nested_payload_iso_path is always defined before the get_qemu_cmd call (e.g., initialize it to None by default or assign the appropriate ISO path for each primary mode), or change the argument to the already-defined variable name if it was misspelled; update the variable initialization used by get_qemu_cmd so every invocation has a valid value.
468-482:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize
nested_payload_iso_pathbefore this block to prevent NameError.
nested_payload_iso_pathis defined only whenargs.test_image_updateis True (line 480), but referenced unconditionally at lines 790 and 849. Running--qemu-cmdor any other primary mode without--test-image-updatewill raise NameError.🐛 Proposed fix
gen_disk(args.disk) +nested_payload_iso_path = None if args.test_image_update: if os.path.isdir(NESTED_ISO_DATA_DIR):🤖 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 468 - 482, The NameError occurs because nested_payload_iso_path is only set inside the if args.test_image_update block; initialize nested_payload_iso_path = None immediately before that block so the variable always exists, then keep the existing assignment to nested_payload_iso_path = os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) inside the block; optionally ensure later uses of nested_payload_iso_path handle the None case if test-image-update wasn’t requested.
🤖 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 `@scripts/check-qemu-install`:
- Around line 787-792: The call to get_qemu_cmd inside the args.qemu_cmd branch
references nested_payload_iso_path which is only set when --test-image-update is
used, causing a NameError; fix by ensuring nested_payload_iso_path is always
defined before use (e.g., initialize nested_payload_iso_path = None earlier) or
by passing a conditional value into get_qemu_cmd (use nested_payload_iso_path if
set else None) so get_qemu_cmd(...) is never given an undefined variable; update
the args.qemu_cmd block to use that defined/conditional value and keep the
get_qemu_cmd and args.qemu_cmd symbols unchanged.
- Around line 847-849: The call to get_qemu_cmd uses nested_payload_iso_path
which is undefined, causing a NameError; locate the code paths that set
nested_payload_iso_path (see the test-image-update branch around lines 468-482)
and ensure nested_payload_iso_path is always defined before the get_qemu_cmd
call (e.g., initialize it to None by default or assign the appropriate ISO path
for each primary mode), or change the argument to the already-defined variable
name if it was misspelled; update the variable initialization used by
get_qemu_cmd so every invocation has a valid value.
- Around line 468-482: The NameError occurs because nested_payload_iso_path is
only set inside the if args.test_image_update block; initialize
nested_payload_iso_path = None immediately before that block so the variable
always exists, then keep the existing assignment to nested_payload_iso_path =
os.path.abspath(NESTED_INSTALLER_PAYLOAD_ISO) inside the block; optionally
ensure later uses of nested_payload_iso_path handle the None case if
test-image-update wasn’t requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b89c6703-7d43-47dc-977a-09d3a38e6316
📒 Files selected for processing (5)
.gitignoreMakefiledocker/Dockerfilescripts/check-qemu-installscripts/iso-to-oci
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
🪛 checkmake (0.3.2)
Makefile
[warning] 22-22: Target ".ONESHELL" defined multiple times (lines 17 and 22).
(uniquetargets)
[warning] 27-27: Target ".ONESHELL" defined multiple times (lines 17 and 27).
(uniquetargets)
[warning] 32-32: Target ".ONESHELL" defined multiple times (lines 17 and 32).
(uniquetargets)
[warning] 37-37: Target ".ONESHELL" defined multiple times (lines 17 and 37).
(uniquetargets)
[warning] 42-42: Target ".ONESHELL" defined multiple times (lines 17 and 42).
(uniquetargets)
[warning] 47-47: Target ".ONESHELL" defined multiple times (lines 17 and 47).
(uniquetargets)
[warning] 52-52: Target ".ONESHELL" defined multiple times (lines 17 and 52).
(uniquetargets)
[warning] 57-57: Target ".ONESHELL" defined multiple times (lines 17 and 57).
(uniquetargets)
[warning] 62-62: Target ".ONESHELL" defined multiple times (lines 17 and 62).
(uniquetargets)
[warning] 77-77: Target ".ONESHELL" defined multiple times (lines 17 and 77).
(uniquetargets)
[warning] 82-82: Target ".ONESHELL" defined multiple times (lines 17 and 82).
(uniquetargets)
[warning] 87-87: Target ".ONESHELL" defined multiple times (lines 17 and 87).
(uniquetargets)
🪛 Trivy (0.69.3)
docker/Dockerfile
[error] 257-260: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y python3-pexpect qemu-utils qemu-kvm'
Rule: DS-0029
(IaC/Dockerfile)
[error] 263-267: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iamd64; then apt-get update && apt-get install -y ovmf qemu-system-x86; fi'
Rule: DS-0029
(IaC/Dockerfile)
[error] 270-274: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'if dpkg-architecture -iarm64; then apt-get update && apt-get install -y qemu-system-aarch64 qemu-efi-aarch64; fi'
Rule: DS-0029
(IaC/Dockerfile)
🔍 Remote MCP
I'll help you gather additional relevant context for reviewing this pull request. Let me search for information about the key features and technologies involved.Based on the search results, I now have good context about VyOS system image management and QEMU testing. Let me search for one more specific query about the task reference.## Summary of Relevant Context for PR Review
Based on my research, here is the additional relevant context for reviewing this pull request:
VyOS System Image Management
The add system image command is used to install a new system image, retrievable from the web (http://, https://) or from your local system. The add system image command supports installing new VyOS versions through an optional VRF, which aligns with this PR's goal of testing the image update flow in both default and VRF routing contexts.
The command extracts the image and prompts you to use the current system configuration and SSH security keys, allowing the new image to boot with your current configuration. This matches the PR's mention of handling "copying active configuration and SSH host keys" during the smoketest.
When fetching a digital signature file fails, the system prompts "Do you want to continue without signature check?" and then checks MD5 checksums of files on the ISO image—directly corresponding to the PR's handling of missing minisig signature prompts mentioned in the description.
QEMU Testing Framework
VyOS uses two independent tests run in parallel by separate QEMU instances via make test and make testc from within the vyos-build repository, where the make test command launches a new QEMU instance and the ISO image is first installed to the virtual harddisk. After its first boot into the newly installed system, the main Smoketest script is executed from /usr/bin/vyos-smoketest.
Architecture Support Changes
Historically, VyOS supported multiple target architectures including amd64, i386, and armhf. However, the current PR removes armhf and i386 support from the build container, limiting QEMU-related packages to amd64 and arm64 architectures, reflecting a strategic narrowing of supported platforms for the rolling release.
OCI Vendor Label Update
The PR updates the OCI image label vendor from "Sentrium S.L." to "VyOS Networks", which represents an organizational identity update for the container image metadata.
[::web_search::]
🔇 Additional comments (20)
docker/Dockerfile (5)
19-19: LGTM!
31-34: LGTM!
44-44: LGTM!
256-274: LGTM!
276-283: LGTM!scripts/iso-to-oci (1)
12-29: LGTM!Makefile (3)
15-78: LGTM!
80-88: LGTM!
111-113: LGTM!scripts/check-qemu-install (11)
57-70: LGTM!
209-210: LGTM!
248-269: LGTM!
381-410: LGTM!
435-441: LGTM!
453-453: LGTM!
354-357: LGTM!
616-674: LGTM!
681-786: LGTM!
1441-1463: LGTM!
1502-1510: LGTM!
|
CI integration ❌ failed! Details
|
natali-rs1985
left a comment
natali-rs1985
left a comment
There was a problem hiding this comment.
Adds testing for the system image upgrade workflow.
Tested locally — both default routing and VRF scenarios work as expected.
jestabro
left a comment
jestabro
left a comment
There was a problem hiding this comment.
Add test for add system image, and clean up of unneeded options. Locally tested.
vyos-bot
Bot
added
mirror-initiated
4 participants
NOTE This must be merged AFTER #1198
Change summary
Change drops
armhfandi386from the build container, updates the OCI vendor label toVyOS Networks, and limits QEMU-related packages toamd64andarm64.The QEMU smoketest harness no longer supports
--configdor the Makefilecheckisohelper; ISO paths are validated inside the scripts, incompatible primary testcase flags are now rejected.It also adds
--test-image-updateandmake test-image-update, which exerciseadd system imageanddelete system imageover HTTP from a nested installer ISO, once in the default routing context and once through a named VRF.Related Task(s)
Related PR(s)
How to test / Smoketest result
sudo make test-image-updateChecklist: