guest_os_booting: cover loadparm on interface #6560
Conversation
WalkthroughAdds multi-entry TFTP PXE support for s390x by passing a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Test Runner
participant Test as boot_with_multiple_boot_order.py
participant TFTP as provider.virtual_network.tftpboot
participant HTTP as Install Tree Server
participant FS as Host FS
participant VM as Guest (s390x)
Tester->>Test: load config (install_tree_url, tftp_entries)
Test->>TFTP: create_tftp_content(url, ks, arch="s390x", entries=[linux1,linux2])
activate TFTP
TFTP->>FS: mkdir tftp_dir and per-entry dirs
loop per entry
TFTP->>HTTP: GET {entry}/initrd.img & {entry}/kernel.img
HTTP-->>TFTP: deliver artifacts
TFTP->>FS: write artifacts into entry dir
end
TFTP->>FS: write multi-entry pxelinux.cfg (default=first)
TFTP->>FS: chmod/chown/restorecon
deactivate TFTP
Test->>VM: configure netboot (optional loadparm)
VM->>TFTP: fetch PXE config & boot artifacts
VM-->>Tester: console output (validate Loadparm / test_cmd)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
s390x supports more than one network boot entry since QEMU 10.1.0. The loadparm value can be used to boot into entries directly. Add test case that confirms that the second network entry is booted into. The test only covers that the right entry is accessed, not the actual installation. As such, the test case creates a directory per entry but uses the same bootable data. The message confirms that the right folder is accessed. Signed-off-by: Sebastian Mitterle <[email protected]>
smitterl
force-pushed
the
pxe_with_loadparm
branch
from
98fd3a0 to
6adc5df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
provider/virtual_network/tftpboot.py (2)
129-133: Avoid bare except and log the exception. Optionally clear actions post-cleanup.This improves diagnosability and prevents masking non-trivial errors.
- for action in cleanup_actions: - try: - action() - except: - logging.debug("There were errors during cleanup. Please check the log.") + for action in cleanup_actions: + try: + action() + except Exception: + logging.exception("Error during cleanup") + cleanup_actions.clear()
55-65: Preserve backward compatibility: makeentriesoptional (default None).A 3-arg call exists at virttools/tests/src/virt_install/pxe_installation.py:23 — changing the signature will break callers.
-def create_tftp_content(install_tree_url, kickstart_url, arch, entries): +def create_tftp_content(install_tree_url, kickstart_url, arch, entries=None): @@ - :params entries: list of entry names for the pxe configuration + :param entries: list of entry names for the pxe configuration @@ - if arch != "s390x": + if arch != "s390x": raise NotImplementedError(f"No implementation available for '{arch}'.") - if not entries or len([x for x in entries if len(x) == 0]) > 0: - raise ValueError(f"Expecting list of non-empty strings, got {entries}") + if entries is None: + entries = ["linux"]
🧹 Nitpick comments (5)
libvirt/tests/src/guest_os_booting/boot_order/boot_with_multiple_boot_order.py (2)
39-39: Harden tftp_entries parsing (trim and drop empties).Avoid empty entry names if the param contains spaces or trailing commas.
- tftp_entries = params.get("tftp_entries", "linux").split(",") + entries_raw = params.get("tftp_entries", "linux") + tftp_entries = [e.strip() for e in entries_raw.split(",") if e.strip()]
53-59: Make the repo URL pipeline safer and build boot_img_url with URL utilities.
- Shell pipeline: consider pipefail to fail fast if dnf errors.
- Use urljoin for URLs instead of os.path.join.
- cmd = "dnf repolist -v enabled |awk '/Repo-baseurl.*composes.*BaseOS.*os/ {res=$NF} END{print res}'" - repo_url = process.run(cmd, shell=True).stdout_text.strip() - boot_img_url = os.path.join(repo_url, "images", "boot.iso") + cmd = "bash -o pipefail -c \"dnf repolist -v enabled | awk '/Repo-baseurl.*composes.*BaseOS.*os/ {res=$NF} END{print res}'\"" + repo_url = process.run(cmd, shell=True).stdout_text.strip() + from urllib.parse import urljoin + boot_img_url = urljoin(repo_url.rstrip('/') + '/', 'images/boot.iso')provider/virtual_network/tftpboot.py (3)
69-71: Tighten entries validation (type, whitespace).Catches non-strings and whitespace-only items; clearer error.
- if not entries or len([x for x in entries if len(x) == 0]) > 0: - raise ValueError(f"Expecting list of non-empty strings, got {entries}") + if (not isinstance(entries, (list, tuple)) + or any(not isinstance(e, str) or not e.strip() for e in entries)): + raise ValueError(f"Expected a list of non-empty strings for entries, got {entries!r}") + entries = [e.strip() for e in entries]
93-94: Ensure atomic write of PXE config (nit).Minor: write via a temp file then rename to avoid readers seeing partial content.
- with open(os.path.join(tftp_dir, boot_file), "w") as f: - f.write(pxeconfig_content) + cfg_path = os.path.join(tftp_dir, boot_file) + tmp_path = cfg_path + ".tmp" + with open(tmp_path, "w") as f: + f.write(pxeconfig_content) + os.replace(tmp_path, cfg_path)
96-99: Quote paths and be explicit with chown target.Quote tftp_dir to avoid globbing; use nobody:nobody explicitly.
- cmds.append("chmod -R a+r " + tftp_dir) - cmds.append("chown -R nobody: " + tftp_dir) - cmds.append("chcon -R --reference /usr/sbin/dnsmasq " + tftp_dir) - cmds.append("chcon -R --reference /usr/libexec/libvirt_leaseshelper " + tftp_dir) + cmds.append("chmod -R a+r " + shlex.quote(tftp_dir)) + cmds.append("chown -R nobody:nobody " + shlex.quote(tftp_dir)) + cmds.append("chcon -R --reference /usr/sbin/dnsmasq " + shlex.quote(tftp_dir)) + cmds.append("chcon -R --reference /usr/libexec/libvirt_leaseshelper " + shlex.quote(tftp_dir))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/guest_os_booting/boot_order/boot_with_multiple_boot_order.cfg(1 hunks)libvirt/tests/src/guest_os_booting/boot_order/boot_with_multiple_boot_order.py(3 hunks)provider/virtual_network/tftpboot.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/guest_os_booting/boot_order/boot_with_multiple_boot_order.cfg
🧰 Additional context used
🧬 Code graph analysis (2)
libvirt/tests/src/guest_os_booting/boot_order/boot_with_multiple_boot_order.py (2)
provider/guest_os_booting/guest_os_booting_base.py (2)
prepare_os_xml(59-74)test_file_download(113-121)provider/virtual_network/tftpboot.py (2)
create_tftp_content(55-102)create_tftp_network(105-122)
provider/virtual_network/tftpboot.py (1)
virttools/tests/src/virt_install/pxe_installation.py (1)
run(12-56)
🪛 Ruff (0.12.2)
libvirt/tests/src/guest_os_booting/boot_order/boot_with_multiple_boot_order.py
54-54: Function call with shell=True parameter identified, security issue
(S604)
provider/virtual_network/tftpboot.py
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Function call with shell=True parameter identified, security issue
(S604)
80-80: Function call with shell=True parameter identified, security issue
(S604)
87-87: Function call with shell=True parameter identified, security issue
(S604)
⏰ 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: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (4)
libvirt/tests/src/guest_os_booting/boot_order/boot_with_multiple_boot_order.py (2)
67-69: Good: wires new tftp_entries into tftp content creation.Call site updated to pass entries; matches the new API.
83-86: Manual verification required: confirm interface 'loadparm' is appliedmodify_vm_device implementation was not found in this repo and virsh is unavailable in the sandbox — cannot verify automatically. Confirm libvirt_vmxml.modify_vm_device can set the s390x "loadparm" on and that the VM XML contains it.
Run on a host with virsh and paste the output:
# show any <interface> blocks that include a loadparm attribute virsh dumpxml "${VM:-$vm_name}" | sed -n '/<interface/,/<\/interface>/p' | rg "loadparm=" -n -C2provider/virtual_network/tftpboot.py (2)
30-53: Nice extraction of PXE config builder.Simple and readable; defaulting to the first entry as PXE default makes sense for the test.
111-118: Network XML creation looks good.Parameters are clear and the cleanup action to destroy the network is registered.
| process.run("mkdir " + tftp_dir, ignore_status=False, shell=True, verbose=True) | ||
| cleanup_actions.insert( | ||
| 0, | ||
| lambda: process.run( | ||
| "rm -rf " + tftp_dir, ignore_status=False, shell=True, verbose=True | ||
| ), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use os.makedirs/shutil.rmtree instead of shell mkdir/rm (idempotent, safer).
Prevents failures if the directory exists, avoids shell=True, and improves quoting.
- process.run("mkdir " + tftp_dir, ignore_status=False, shell=True, verbose=True)
- cleanup_actions.insert(
- 0,
- lambda: process.run(
- "rm -rf " + tftp_dir, ignore_status=False, shell=True, verbose=True
- ),
- )
+ os.makedirs(tftp_dir, exist_ok=True)
+ cleanup_actions.insert(0, lambda: shutil.rmtree(tftp_dir, ignore_errors=True))Add at the top of the module:
import shutil
import shlex🧰 Tools
🪛 Ruff (0.12.2)
77-77: Function call with shell=True parameter identified, security issue
(S604)
80-80: Function call with shell=True parameter identified, security issue
(S604)
| for entry in entries: | ||
| entry_dir = os.path.join(tftp_dir, entry) | ||
| process.run("mkdir " + entry_dir, ignore_status=False, shell=True, verbose=True) | ||
| cmds.append("curl %s -o %s/initrd.img" % (initrd_img_url, entry_dir)) | ||
| cmds.append("curl %s -o %s/kernel.img" % (kernel_img_url, entry_dir)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create per-entry dirs idempotently and harden downloads.
- Avoid shell mkdir; use os.makedirs.
- Use curl with fail/redirect/retry flags and proper quoting; or switch to avocado download.get_file for consistency.
- for entry in entries:
- entry_dir = os.path.join(tftp_dir, entry)
- process.run("mkdir " + entry_dir, ignore_status=False, shell=True, verbose=True)
- cmds.append("curl %s -o %s/initrd.img" % (initrd_img_url, entry_dir))
- cmds.append("curl %s -o %s/kernel.img" % (kernel_img_url, entry_dir))
+ for entry in entries:
+ entry_dir = os.path.join(tftp_dir, entry)
+ os.makedirs(entry_dir, exist_ok=True)
+ cmds.append(
+ "curl --fail -L --retry 5 --retry-delay 2 {src} -o {dst}".format(
+ src=shlex.quote(initrd_img_url),
+ dst=shlex.quote(os.path.join(entry_dir, "initrd.img")),
+ )
+ )
+ cmds.append(
+ "curl --fail -L --retry 5 --retry-delay 2 {src} -o {dst}".format(
+ src=shlex.quote(kernel_img_url),
+ dst=shlex.quote(os.path.join(entry_dir, "kernel.img")),
+ )
+ )Alternative (preferred): replace the two curl cmds with avocado.utils.download.get_file calls executed directly (no shell).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
87-87: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
In provider/virtual_network/tftpboot.py around lines 85 to 90, the code uses
shell mkdir and bare curl commands; replace them to be idempotent and hardened:
use os.makedirs(entry_dir, exist_ok=True) instead of process.run("mkdir ...",
shell=True), and remove the two curl command strings — preferably call
avocado.utils.download.get_file(url, target_path) directly for initrd.img and
kernel.img (no shell), or if keeping curl use properly quoted paths and flags
like --fail --location --retry 3 to harden downloads; ensure the download calls
run in-process so errors propagate instead of silent shell failures.
provider/virtual_network/tftpboot.py
Outdated
| return "\n".join(contents) | ||
|
|
||
|
|
||
| def create_tftp_content(install_tree_url, kickstart_url, arch, entries): |
There was a problem hiding this comment.
I found a script tp-libvirt/virttools/tests/src/virt_install/pxe_installation.py also refer to this function, I think this file also should be modified accordingly.
liang-cong-red-hat
left a comment
liang-cong-red-hat
left a comment
There was a problem hiding this comment.
Just one comments, others LGTM
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
@liang-cong-red-hat spotted a backward compatibility issue that I need to fix so putting this in Draft shortly so it won't be merged. But given the already existing approvals, I think I'll merge once I confirmed I addressed it.
The newly introduced parameter 'entries' needs a default parameter to work with existing usage, e.g. in virttools' pxe installation test. Signed-off-by: Sebastian Mitterle <[email protected]>
The network selection was missing from the interface definition update. This is a new test case and presumably it passed because the interface definition already had that value configured from previous manual testing. Select now the right value, same as in other with_loadparm variants. Signed-off-by: Sebastian Mitterle <[email protected]>
Additionally, I identified a test config issue which I also fixed. With the two additional commits: |
|
As discussed, the other changes were already reviewed and this seems minor enough (added default value) with executed tests. With this, I will merge it now. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libvirt/tests/cfg/guest_os_booting/boot_order/boot_with_multiple_boot_order.cfg (1)
42-50: Good addition for loadparm-driven PXE selection; consider small robustness improvements.
- tftp_entries parsing: ensure whitespace is trimmed and empty items are ignored (e.g., "linux1, linux2").
- Coverage: optionally mirror this with_loadparm variant under the other network_bootable blocks for consistency across first_dev combinations.
provider/virtual_network/tftpboot.py (1)
67-71: Validate entry names to avoid path/shell surprises.Restrict entries to a safe charset (e.g., [A-Za-z0-9._-]) and deduplicate to prevent malformed PXE labels or odd dirs.
You can add:
+ import re + safe = re.compile(r'^[A-Za-z0-9._-]+$') + if any(not safe.match(e) for e in entries): + raise ValueError(f"Invalid entry name(s) {entries}; allowed: [A-Za-z0-9._-]") + entries = list(dict.fromkeys(entries)) # preserve order, drop dups
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/guest_os_booting/boot_order/boot_with_multiple_boot_order.cfg(1 hunks)provider/virtual_network/tftpboot.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/virtual_network/tftpboot.py (1)
virttools/tests/src/virt_install/pxe_installation.py (1)
run(12-56)
🪛 Ruff (0.13.1)
provider/virtual_network/tftpboot.py
55-55: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Function call with shell=True parameter identified, security issue
(S604)
80-80: Function call with shell=True parameter identified, security issue
(S604)
87-87: Function call with shell=True parameter identified, security issue
(S604)
🔇 Additional comments (6)
provider/virtual_network/tftpboot.py (6)
30-52: PXE config generator looks correct.Default entry selection and per-entry kernel/initrd paths are clear.
91-94: LGTM on writing pxelinux.cfg.File creation via context manager is fine.
111-118: LGTM on network params reformatting.No behavior change; keeps previous semantics.
77-83: Stop using shell=True for mkdir/rm; use os/shutil (prevents injection, idempotent).Current code builds shell commands; replace with Python APIs.
Apply this diff:
- process.run("mkdir " + tftp_dir, ignore_status=False, shell=True, verbose=True) - cleanup_actions.insert( - 0, - lambda: process.run( - "rm -rf " + tftp_dir, ignore_status=False, shell=True, verbose=True - ), - ) + os.makedirs(tftp_dir, exist_ok=True) + cleanup_actions.insert(0, lambda: shutil.rmtree(tftp_dir, ignore_errors=True))Add at top (imports):
import shutil
85-90: Eliminate shell mkdir and bare curl; download safely without shell.The composed curl commands allow command injection via install_tree_url and write paths; switch to in-process downloads and os.makedirs.
Apply this diff:
- for entry in entries: - entry_dir = os.path.join(tftp_dir, entry) - process.run("mkdir " + entry_dir, ignore_status=False, shell=True, verbose=True) - cmds.append("curl %s -o %s/initrd.img" % (initrd_img_url, entry_dir)) - cmds.append("curl %s -o %s/kernel.img" % (kernel_img_url, entry_dir)) + for entry in entries: + entry_dir = os.path.join(tftp_dir, entry) + os.makedirs(entry_dir, exist_ok=True) + initrd_dst = os.path.join(entry_dir, "initrd.img") + kernel_dst = os.path.join(entry_dir, "kernel.img") + # Prefer Avocado downloader if available: + try: + from avocado.utils import download + download.get_file(initrd_img_url, initrd_dst) + download.get_file(kernel_img_url, kernel_dst) + except Exception: + # Fallback to urllib if download module is unavailable + import urllib.request + urllib.request.urlretrieve(initrd_img_url, initrd_dst) + urllib.request.urlretrieve(kernel_img_url, kernel_dst)
55-55: All create_tftp_content call sites remain compatible with the updated signature.
| return "\n".join(contents) | ||
|
|
||
|
|
||
| def create_tftp_content(install_tree_url, kickstart_url, arch, entries=["linux"]): |
There was a problem hiding this comment.
Fix mutable default argument for entries (B006) and initialize safely.
Use None default and set the fallback inside the function.
Apply this diff:
-def create_tftp_content(install_tree_url, kickstart_url, arch, entries=["linux"]):
+def create_tftp_content(install_tree_url, kickstart_url, arch, entries=None):
@@
- if arch != "s390x":
+ if arch != "s390x":
raise NotImplementedError(f"No implementation available for '{arch}'.")
- if not entries or len([x for x in entries if len(x) == 0]) > 0:
+ if entries is None:
+ entries = ["linux"]
+ if not entries or any(len(x) == 0 for x in entries):
raise ValueError(f"Expecting list of non-empty strings, got {entries}")As per static analysis hints
Also applies to: 67-71
🧰 Tools
🪛 Ruff (0.13.1)
55-55: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In provider/virtual_network/tftpboot.py around lines 55 and 67-71, the function
create_tftp_content uses a mutable default for entries (entries=["linux"]),
which can lead to unexpected shared-state bugs; change the signature to use
entries=None and inside the function set entries = ["linux"] if entries is None,
and ensure any code that mutates entries operates on this new local list (or a
copy) so defaults are not shared across calls.
3 participants
Depends on avocado-framework/avocado-vt#4239
s390x supports more than one network boot entry since QEMU 10.1.0. The loadparm value can be used to boot into entries directly.
Add test case that confirms that the second network entry is booted into.
The test only covers that the right entry is accessed, not the actual installation. As such, the test case creates a directory per entry but uses the same bootable data. The message confirms that the right folder is accessed.
Summary by CodeRabbit
New Features
Tests
Chores