Improve virtual_network.iface_network.net_pxe to support new test tool#6857
Improve virtual_network.iface_network.net_pxe to support new test tool#6857yanglei-rh wants to merge 1 commit into
Conversation
WalkthroughThe PR adds PXE/VM provisioning parameters to the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/iface_network.py`:
- Around line 104-107: The loop over reference paths [" /usr/sbin/dnsmasq",
"/usr/libexec/libvirt_leaseshelper"] currently calls process.run("chcon -R
--reference %s %s" % (ref, tftp_root), ...) for every existing ref, causing
multiple relabels; change the loop in iface_network.py so that once a matching
ref is found and chcon is executed successfully you break out of the for-loop
(i.e., after the process.run call) to make the list a true fallback and prevent
subsequent overwrites of tftp_root's SELinux context.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4085034f-4681-4ad1-bc16-c3960193c827
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_network.cfglibvirt/tests/src/virtual_network/iface_network.py
1.Setup vms is empty to skip create guest command step 2.Add "only Linux" to limit guest type 3.Add selinux context control for tftp folder when setup netdst 4.Setup netdst to virbr0 becuase this scenario depends on libvirt default network Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22430
branch
from
5047b96 to
fcc62fc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/iface_network.py (1)
66-74: Consider validating that kernel/initrd files exist before copying.The copy operations on lines 73-74 will fail with a non-zero exit status if the source files don't exist, which is appropriate. However, adding an existence check with a clearer error message would improve debuggability when the test data is missing.
Additionally, when
guest_nameis empty (line 69), the path resolves to<data_dir>/images/vmlinuzrather than a guest-specific directory. Verify this is the intended behavior for the test tool.💡 Optional: Add validation for clearer error messages
if params.get("netdst"): root_dir = data_dir.get_data_dir() # Directly use root_dir/images/guest_name, removing export_dir support img_dir = os.path.join(root_dir, "images", params.get("guest_name", "")) kernel = os.path.join(img_dir, params.get("kernel", "")) initrd = os.path.join(img_dir, params.get("initrd", "")) + if not os.path.exists(kernel): + test.error("Kernel file not found: %s" % kernel) + if not os.path.exists(initrd): + test.error("Initrd file not found: %s" % initrd) process.run("cp -f %s %s/vmlinuz" % (kernel, tftp_root), shell=True, verbose=True) process.run("cp -f %s %s/initrd.img" % (initrd, tftp_root), shell=True, verbose=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/iface_network.py` around lines 66 - 74, Check that the kernel and initrd files exist before calling process.run: compute img_dir using data_dir.get_data_dir() and params.get("guest_name"), then validate os.path.exists(kernel) and os.path.exists(initrd) (kernel and initrd are the computed paths) and raise a clear RuntimeError or call processLogger.error with a descriptive message including the full path if either is missing instead of letting cp fail; also add a small guard/warning when params.get("guest_name") is empty (so img_dir resolves to root/images/) and either refuse/raise or log a warning so test authors know this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libvirt/tests/src/virtual_network/iface_network.py`:
- Around line 66-74: Check that the kernel and initrd files exist before calling
process.run: compute img_dir using data_dir.get_data_dir() and
params.get("guest_name"), then validate os.path.exists(kernel) and
os.path.exists(initrd) (kernel and initrd are the computed paths) and raise a
clear RuntimeError or call processLogger.error with a descriptive message
including the full path if either is missing instead of letting cp fail; also
add a small guard/warning when params.get("guest_name") is empty (so img_dir
resolves to root/images/) and either refuse/raise or log a warning so test
authors know this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b010f83a-caff-4e95-b33d-b35fa35c211d
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_network.cfglibvirt/tests/src/virtual_network/iface_network.py
|
Closing this PR due to current team constraints. This is part of a broader effort to triage all in-flight work across our upstream repos. If this work is still needed, please feel free to reopen and it will be picked up. Apologies for any inconvenience. |
2 participants
1.Setup vms is empty to skip create guest command step
2.Add "only Linux" to limit guest type
3.Add selinux context control for tftp folder when setup netdst
4.Setup netdst to virbr0 becuase this scenario depends on libvirt default network
ID: LIBVIRTAT-22430
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit