Updated fedora container-disk and qcow2 to 43#3760
Open
chandramerla wants to merge 3 commits intoRedHatQE:mainfrom
Open
Updated fedora container-disk and qcow2 to 43#3760chandramerla wants to merge 3 commits intoRedHatQE:mainfrom
chandramerla wants to merge 3 commits intoRedHatQE:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughS390X Cirros image refs were replaced with Alpine-based names and defaults adjusted; Fedora S390X image refs moved from Fedora 41→43 and container tag updated; a new script to build Fedora cloud artifacts was added; tests updated to Fedora 43; qemu-guest-agent detection now uses an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
/wip /build-and-push-container |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3760 +/- ##
==========================================
+ Coverage 98.63% 98.65% +0.02%
==========================================
Files 25 25
Lines 2420 2459 +39
==========================================
+ Hits 2387 2426 +39
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:running
openshift-virtualization-qe-bot
previously approved these changes
Feb 7, 2026
Member
Author
|
/build-and-push-container |
Member
Author
|
/wip |
Contributor
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-3
requested review from
dshchedr and
geetikakay
Member
Author
|
/wip /build-and-push-container |
openshift-virtualization-qe-bot-4
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/constants.py (1)
155-168:⚠️ Potential issue | 🟡 MinorMEDIUM: Partial version update creates mixed Fedora versions within the same S390X
Cirrosobject.
QCOW2_IMGnow points to Fedora 43-1.6, whileRAW_IMG,RAW_IMG_GZ,RAW_IMG_XZ,QCOW2_IMG_GZ, andQCOW2_IMG_XZstill reference Fedora 41-1.4. Any test that exercises compressed variants (gz/xz) of either raw or qcow2 on S390X will use a different OS version than the uncompressed qcow2 path.I see the PR description notes a follow-up for
Fedora.*.(raw|qcow2).(xz.gz)— just calling this out because until that lands, S390X image-format tests could produce inconsistent results across formats. Consider either:
- Updating all entries here atomically, or
- Adding a code comment noting the remaining entries are intentionally deferred.
🤖 Fix all issues with AI agents
In `@utilities/constants.py`:
- Line 188: The FEDORA_CONTAINER_IMAGE constant currently points to a personal
quay.io namespace (FEDORA_CONTAINER_IMAGE =
"quay.io/nestor_acuna_blanco/openshift-cnv/fedora:43-s390x"); update it to the
official organization image name matching the existing pattern (e.g.,
"quay.io/openshift-cnv/qe-cnv-tests-fedora:43-s390x") or, if this is a temporary
placeholder, replace or annotate the constant FEDORA_CONTAINER_IMAGE with a
TODO/FIXME comment including a tracking reference (issue/PR ID) indicating
migration to the official quay.io/openshift-cnv image before final release.
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3760 published |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:passed
Member
Author
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3760 published |
chandramerla
force-pushed
the
test-fedora-43
branch
from
bcf398c to
84f2b9e
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
chandramerla
force-pushed
the
test-fedora-43
branch
from
84f2b9e to
9909f7e
Compare
Member
Author
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3760 published |
|
D/S test |
Member
Author
|
/verified I've verified the changes (+ test fixes from #4305 ) on s390x, as mainly changed in constants.py s390x class and global_config_s390x.py and they don't affect tests of x86. Tests run args: |
Contributor
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
Member
Author
|
/wip cancel Please review! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20967 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@containers/fedora_artifacts/build.sh`:
- Around line 49-50: The script currently renames the upstream qcow2 by using mv
"${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}", which removes the original
upstream filename consumers expect; change this so the original ${qcow2_name}
remains in OUT_DIR (e.g., replace the mv with a copy or create a separate
${img_name} while leaving ${qcow2_name} intact). Update the build.sh block
around the echo/mv lines (reference ${qcow2_name}, ${img_name}, OUT_DIR and the
mv invocation) to create the alternate artifact without deleting or overwriting
the upstream qcow2 filename so utilities/constants.py lookups continue to
succeed.
In `@utilities/virt.py`:
- Around line 2552-2554: The check hard-codes "rpm -q qemu-guest-agent" which
breaks on non-RPM guests; replace it with a distro-agnostic flow that queries
ssh_exec.package_manager.info() and uses the detected manager to run the
appropriate query (e.g. use "rpm -q" for RPM families and "dpkg -s" or
equivalent for Debian families), or fall back to a generic package-manager API
if available; specifically update the code that currently calls
ssh_exec.run_command(command=shlex.split("rpm -q qemu-guest-agent")) to inspect
ssh_exec.package_manager.info() and choose the correct check for the
qemu-guest-agent package.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad37198f-3a93-4285-93e5-c27b854a82e4
⛔ Files ignored due to path filters (2)
containers/README.mdis excluded by!**/*.mdcontainers/fedora_artifacts/README.mdis excluded by!**/*.md
📒 Files selected for processing (4)
containers/fedora_artifacts/build.shtests/global_config_s390x.pyutilities/constants.pyutilities/virt.py
Comment on lines
+49
to
+50
| echo "[${arch}] Renaming qcow2 to ${img_name}..." | ||
| mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}" |
Contributor
There was a problem hiding this comment.
HIGH: Keep upstream qcow2 filename; current move breaks filename contract.
Line 50 removes ${qcow2_name} from out/ by moving it to Fedora-qcow2-${arch}.img. Consumers in utilities/constants.py still reference the upstream Fedora filename (for example, Fedora-Cloud-Base-Generic-43-1.6.s390x.qcow2), so artifact lookup can fail.
💡 Suggested fix
- echo "[${arch}] Renaming qcow2 to ${img_name}..."
- mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}"
+ echo "[${arch}] Creating qcow2 alias ${img_name} (keep upstream name too)..."
+ cp "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}"📝 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.
Suggested change
| echo "[${arch}] Renaming qcow2 to ${img_name}..." | |
| mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}" | |
| echo "[${arch}] Creating qcow2 alias ${img_name} (keep upstream name too)..." | |
| cp "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@containers/fedora_artifacts/build.sh` around lines 49 - 50, The script
currently renames the upstream qcow2 by using mv "${OUT_DIR}/${qcow2_name}"
"${OUT_DIR}/${img_name}", which removes the original upstream filename consumers
expect; change this so the original ${qcow2_name} remains in OUT_DIR (e.g.,
replace the mv with a copy or create a separate ${img_name} while leaving
${qcow2_name} intact). Update the build.sh block around the echo/mv lines
(reference ${qcow2_name}, ${img_name}, OUT_DIR and the mv invocation) to create
the alternate artifact without deleting or overwriting the upstream qcow2
filename so utilities/constants.py lookups continue to succeed.
Member
Author
There was a problem hiding this comment.
This artifact referred in the utilities/constants.py are customized qcow2, which also got generated when building qcow2 image quay.io/openshift-cnv/qe-cnv-tests-fedora:43-s390x
Fedora = Fedora(
FEDORA42_IMG="Fedora-Cloud-Base-Generic-42-1.1.s390x.qcow2",
FEDORA43_IMG="Fedora-Cloud-Base-Generic-43-1.6.s390x.qcow2",
But the Fedora-qcow2-${arch}.img which is cloud base image with img extension is referenced in :
Cdi = Cdi(
# TODO: S390X does not support Cirros; this is a workaround until tests are moved to Fedora
QCOW2_IMG="Fedora-qcow2-s390x.img",
Both these files are there in artifactory
Contributor
There was a problem hiding this comment.
@chandramerla, thanks for the clarification! You're right — my concern was based on a false assumption that this build.sh script was the sole source for both artifact names. Now I understand the separation:
Fedora-Cloud-Base-Generic-43-1.6.s390x.qcow2(used byFedora.FEDORA43_IMG) is produced independently during the container image build (quay.io/openshift-cnv/qe-cnv-tests-fedora:43-s390x).Fedora-qcow2-s390x.img(used byCdi.QCOW2_IMG) is the artifact thisbuild.shscript is specifically responsible for, so themvis correct and intentional.
The mv is fine as-is. I'll withdraw my concern.
🧠 Learnings used
Learnt from: stesrn
Repo: RedHatQE/openshift-virtualization-tests PR: 2287
File: utilities/constants.py:139-144
Timestamp: 2025-10-18T16:36:52.365Z
Learning: In utilities/constants.py, the ARM64 Fedora configuration intentionally uses different versions for the disk image (Fedora 42) and container image (Fedora 41) because the Fedora ARM container image based on Fedora 42 is not yet available. This version mismatch is expected and should not be flagged as an issue.
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 1566
File: tests/global_config_x86_64.py:29-29
Timestamp: 2025-07-25T01:59:02.180Z
Learning: In the openshift-virtualization-tests repo, the s390x architecture still relies on Fedora 41 images, while only x86_64 is updated to Fedora 42 in PR `#1566`.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 2838
File: .github/workflows/net-utils-builder-staging.yml:37-37
Timestamp: 2025-11-25T01:56:54.902Z
Learning: In the openshift-virtualization-tests repository, when renaming container images that are built and used by GitHub Actions workflows, the changes must be done sequentially: first merge the workflow files (.github/workflows/) that update the image name in the CI/CD pipelines, then update the code references (like constants.py and manifest files) in a follow-up PR. This prevents the old workflow from running with mismatched image names during the transition.
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 4184
File: tests/infrastructure/golden_images/update_boot_source/test_ssp_common_templates_boot_sources.py:95-99
Timestamp: 2026-03-17T18:44:57.349Z
Learning: In tests/infrastructure/golden_images/update_boot_source/test_ssp_common_templates_boot_sources.py, the `opted_out_fedora_data_source` fixture intentionally compares `fedora_data_source.source.name == OS_FLAVOR_FEDORA` (rather than `fedora_data_source.name`). Each architecture-specific cluster has its own global config that defines the correct value for `OS_FLAVOR_FEDORA`, so the constant is already arch-aware and is the appropriate comparison target.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3755
File: utilities/architecture.py:10-11
Timestamp: 2026-03-15T16:01:27.748Z
Learning: In RedHatQE/openshift-virtualization-tests (PR `#3755`), the `Images` variable in `utilities/constants.py` is initialized at import time using `next(iter(get_cluster_architecture())).upper()` as a transient placeholder only. Before any tests run, `update_cpu_arch_related_config()` in `utilities/pytest_utils.py` (called from `pytest_sessionstart`) re-assigns `constants_module.Images = getattr(constants_module.ArchImages, arch.upper())` with the correct architecture derived from `--cpu-arch` CLI option or cluster detection. Do not flag the import-time `next(iter(...))` call as nondeterministic — the authoritative assignment happens in `update_cpu_arch_related_config` before test collection/execution.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 4196
File: utilities/pytest_utils.py:522-527
Timestamp: 2026-03-22T12:40:15.236Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CentOS Stream VirtualMachineClusterPreferences do NOT have architecture-specific variants for any architecture (including ARM64 and s390x). Preferences use generic names like "centos.stream9" and "centos.stream10" without any architecture suffix. Therefore, in `utilities/pytest_utils.py`, the call to `generate_linux_instance_type_os_matrix` for CentOS Stream should always use `arch_suffix=None` (or omit the parameter entirely) — never passing cpu_arch as arch_suffix.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3883
File: tests/global_config_s390x.py:16-22
Timestamp: 2026-02-23T16:30:33.855Z
Learning: On s390x clusters, Fedora VirtualMachineClusterPreference uses architecture-specific preference names with the s390x suffix (e.g., "fedora.42.s390x"), similar to RHEL 9/10 preferences. This differs from CentOS Stream which uses generic preference names without architecture suffixes on s390x. In tests/global_config_s390x.py and utilities/pytest_utils.py, Fedora instance type matrix generation should include arch_suffix=S390X on s390x clusters.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:13.674Z
Learning: In RedHatQE/openshift-virtualization-tests, maintainer dshchedr prefers CodeRabbit to post targeted inline comments on the Files changed tab for each applicable location rather than aggregating multiple issues into a single discussion thread.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:13.674Z
Learning: In RedHatQE/openshift-virtualization-tests, maintainer dshchedr prefers CodeRabbit to post targeted inline comments on the Files Changed tab at each applicable location rather than aggregating multiple issues into a single PR discussion thread reply.
Learnt from: stesrn
Repo: RedHatQE/openshift-virtualization-tests PR: 2499
File: containers/fedora/fedora-vars:1-12
Timestamp: 2025-11-05T06:06:59.103Z
Learning: Fedora 42 Cloud Base Generic images for s390x architecture are available via fedora-secondary releases at https://download.fedoraproject.org/pub/fedora-secondary/releases/42/Cloud/s390x/images/, using the same version (Fedora 42) as x86_64 and aarch64 architectures.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 983
File: utilities/constants.py:163-168
Timestamp: 2025-05-18T07:36:10.674Z
Learning: In the OpenShift Virtualization tests, architecture constants like X86_64, AMD_64, ARM_64, and S390X need to be included in the get_test_images_arch_class() validation check to be fully supported.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 983
File: tests/virt/node/high_performance_vm/test_high_performance_templates.py:144-144
Timestamp: 2025-05-28T09:59:39.558Z
Learning: In the OpenShift Virtualization tests, f-string image path construction with potential None values is acceptable during test collection because tests are filtered during execution based on architecture and pyconfig settings. Tests that would have None values for architecture-specific images won't actually execute.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3755
File: utilities/constants.py:210-212
Timestamp: 2026-03-09T11:56:08.361Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, `get_cluster_architecture()` (in `utilities/architecture.py`) is called at module import time in `utilities/constants.py` to initialize the `Images` variable. This is intentional: for unit tests and offline tooling, the `OPENSHIFT_VIRTUALIZATION_TEST_IMAGES_ARCH` environment variable must be set, causing `get_cluster_architecture()` to return early with a singleton set from the env var without making any cluster API calls. The `cache` decorator on `get_cluster_architecture()` also ensures the function is only executed once. Do not flag this import-time call as a side-effect issue — the env-var escape hatch is the supported pattern for offline use.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR `#2469`) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:00:59.076Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, CodeRabbit must post ONLY an inline review comment on the Files Changed tab and then stop immediately without generating any follow-up comments in the PR discussion thread. No acknowledgment messages, no confirmation of posting, no explanation - silence after posting the inline review equals success. Additional comments create empty/meaningless reviews that clutter the PR.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Comment on lines
+2552
to
+2554
| """Check if qemu-guest-agent is installed via rpm, bypassing rrmngmnt's `which`-based package manager detection.""" | ||
| rc, _, _ = ssh_exec.run_command(command=shlex.split("rpm -q qemu-guest-agent")) | ||
| return rc == 0 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites of check_qemu_guest_agent_installed =="
rg -nP --type=py -C3 '\bcheck_qemu_guest_agent_installed\s*\('
echo
echo "== Nearby OS flavor / distro context =="
rg -nP --type=py -C3 'OS_FLAVOR_|rhel|fedora|ubuntu|debian|alpine|qemu-guest-agent'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 50396
MEDIUM: Use distro-agnostic package manager abstraction instead of hard-coded rpm.
Line 2553 hard-codes rpm -q, limiting the function to RPM-based guests (RHEL, Fedora, CentOS). While all current call sites are RPM-only, the existing package-manager abstraction (ssh_exec.package_manager.info(), used elsewhere in utilities/infra.py) provides distro-agnostic detection that will prevent errors if this function is reused for non-RPM distributions.
Suggested fix
def check_qemu_guest_agent_installed(ssh_exec: Host) -> bool:
- """Check if qemu-guest-agent is installed via rpm, bypassing rrmngmnt's `which`-based package manager detection."""
- rc, _, _ = ssh_exec.run_command(command=shlex.split("rpm -q qemu-guest-agent"))
- return rc == 0
+ """Check whether qemu-guest-agent is installed on the guest."""
+ return bool(ssh_exec.package_manager.info("qemu-guest-agent"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utilities/virt.py` around lines 2552 - 2554, The check hard-codes "rpm -q
qemu-guest-agent" which breaks on non-RPM guests; replace it with a
distro-agnostic flow that queries ssh_exec.package_manager.info() and uses the
detected manager to run the appropriate query (e.g. use "rpm -q" for RPM
families and "dpkg -s" or equivalent for Debian families), or fall back to a
generic package-manager API if available; specifically update the code that
currently calls ssh_exec.run_command(command=shlex.split("rpm -q
qemu-guest-agent")) to inspect ssh_exec.package_manager.info() and choose the
correct check for the qemu-guest-agent package.
Member
Author
There was a problem hiding this comment.
In Fedora43 onwards which is removed. So, had to use rpm -q
Contributor
There was a problem hiding this comment.
@chandramerla, that's a clear and valid reason — thanks for the explanation! Since which was removed from Fedora 43's default install and rrmngmnt's package_manager abstraction relies on which for detection, using rpm -q qemu-guest-agent directly is the correct approach here. My suggestion doesn't apply. I'll record this for future reviews.
✏️ Learnings added
Learnt from: chandramerla
Repo: RedHatQE/openshift-virtualization-tests PR: 3760
File: utilities/virt.py:2552-2554
Timestamp: 2026-04-03T18:59:29.413Z
Learning: In RedHatQE/openshift-virtualization-tests, `which` was removed from Fedora 43's default installation. As a result, `rrmngmnt`'s `ssh_exec.package_manager.exist()` / `ssh_exec.package_manager.info()` abstraction (which relies on `which` to detect the package manager) is broken on Fedora 43+ guests. For RPM-based guests (Fedora 43+, RHEL), use `rpm -q <package>` directly via `ssh_exec.run_command(command=shlex.split("rpm -q <package>"))` and check `rc == 0` instead of the package_manager abstraction. This applies to `check_qemu_guest_agent_installed` in `utilities/virt.py` and similar functions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 1785
File: tests/virt/cluster/common_templates/utils.py:65-66
Timestamp: 2025-08-14T10:28:22.958Z
Learning: The restart_qemu_guest_agent_service function in tests/virt/cluster/common_templates/utils.py is only called for RHEL7 VMs, with OS version checks handled by the calling test code, not within the function itself. Guest agent functionality is verified by subsequent tests in the test class.
Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 3649
File: tests/network/user_defined_network/ip_specification/conftest.py:32-48
Timestamp: 2026-01-29T15:01:54.833Z
Learning: In tests/network/user_defined_network/ip_specification/conftest.py and similar VM fixtures, calling `vm.wait_for_agent_connected()` after `vm.start()` is the correct and sufficient pattern. There is no need to add `wait_for_ready_status(status=True)` before it, because wait_for_agent_connected() already verifies the guest OS is running (not just powered on), which is a stronger validation than ready status.
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 906
File: tests/observability/metrics/conftest.py:19-19
Timestamp: 2025-06-05T11:36:25.219Z
Learning: In the openshift-virtualization-tests repository, the pyhelper_utils package (specifically pyhelper_utils.shell with run_ssh_commands) is installed and available in the test execution environment where the tests are run.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:02.617Z
Learning: In RedHatQE/openshift-virtualization-tests, when reviewing Python files, post targeted inline comments on the Files changed tab at the exact location (file and line) of the issue rather than opening a single discussion thread for multiple issues. This should be done for each applicable location to improve traceability and clarity. If multiple issues exist in the same file, address them with separate inline comments pointing to the specific lines.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:02.617Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CodeRabbit should post targeted inline comments at each applicable location in the Files Changed tab, rather than aggregating multiple issues into a single PR discussion thread reply. This guideline applies to all Python files (any file ending in .py) changed in a PR; for non-Python files, follow the same inline-comment-at-location principle if relevant.
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 1946
File: tests/infrastructure/instance_types/supported_os/conftest.py:117-131
Timestamp: 2025-09-14T05:39:47.969Z
Learning: In utilities/virt.py, Windows OS detection logic uses substring containment checks (e.g., `OS_FLAVOR_WINDOWS in self.os_flavor`) instead of exact equality, allowing os_flavor values like "win-container-disk" to correctly trigger Windows-specific configurations for memory, CPU, SSH authentication, and cloud-init settings.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4269
File: tests/virt/upgrade/utils.py:32-36
Timestamp: 2026-03-24T01:00:35.942Z
Learning: In tests/virt/upgrade/utils.py (RedHatQE/openshift-virtualization-tests), maintainer dshchedr prefers to omit type annotations on helper functions (e.g., get_virt_launcher_image_from_csv). Do not suggest adding type hints or importing types (e.g., ClusterServiceVersion) for functions in this file.
Learnt from: chandramerla
Repo: RedHatQE/openshift-virtualization-tests PR: 2231
File: tests/install_upgrade_operators/must_gather/test_must_gather_vms.py:183-183
Timestamp: 2025-10-14T04:46:09.569Z
Learning: In OpenShift Virtualization tests, the qemu-kvm binary path `/usr/libexec/qemu-kvm` is consistent across x86, s390x, and ARM64 architectures as verified in test_data_collected_from_virt_launcher[test_domcapabilities]. This is supported by KubeVirt documentation at https://kubevirt.io/user-guide/debug_virt_stack/launch-qemu-strace/ which uses this path without architecture-specific distinctions.
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3785
File: utilities/virt.py:0-0
Timestamp: 2026-02-16T10:34:49.439Z
Learning: In the openshift-virtualization-tests repository, in utilities/virt.py, when configuring SSH on VMs for IPv6 single-stack clusters, use cloud-init-per always <id> <command> in runcmd (not bootcmd) to restart SSH on every boot. This ensures the command runs after network configuration is applied during cloud-init's proper phase, avoiding early-boot timing issues that prevent guest agent connection.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 3571
File: tests/storage/storage_migration/utils.py:158-167
Timestamp: 2026-01-25T13:18:21.675Z
Learning: In reviews of the openshift-virtualization-tests repo (and similar Python code), avoid suggesting minor stylistic changes that require extra verification (e.g., removing dict.keys() checks for membership) unless the change has clear correctness or maintainability impact. Focus on fixes with observable behavior, security, performance, or maintainability benefits; defer low-impact style tweaks that are costly to verify.
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 1566
File: tests/global_config_x86_64.py:29-29
Timestamp: 2025-07-25T01:59:02.180Z
Learning: In the openshift-virtualization-tests repo, the s390x architecture still relies on Fedora 41 images, while only x86_64 is updated to Fedora 42 in PR `#1566`.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR `#2199` depends on PR `#2139` which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR `#2469`) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:00:59.076Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, CodeRabbit must post ONLY an inline review comment on the Files Changed tab and then stop immediately without generating any follow-up comments in the PR discussion thread. No acknowledgment messages, no confirmation of posting, no explanation - silence after posting the inline review equals success. Additional comments create empty/meaningless reviews that clutter the PR.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 2838
File: .github/workflows/net-utils-builder-staging.yml:37-37
Timestamp: 2025-11-25T01:56:54.902Z
Learning: In the openshift-virtualization-tests repository, when renaming container images that are built and used by GitHub Actions workflows, the changes must be done sequentially: first merge the workflow files (.github/workflows/) that update the image name in the CI/CD pipelines, then update the code references (like constants.py and manifest files) in a follow-up PR. This prevents the old workflow from running with mismatched image names during the transition.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 3387
File: tests/network/provider_migration/libprovider.py:50-52
Timestamp: 2026-01-12T11:24:13.825Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, when catching exceptions in Python, use LOGGER.error before re-raising and do not replace it with LOGGER.exception in except blocks. This follows the established pattern across the codebase.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3366
File: tests/storage/cdi_clone/test_clone.py:5-9
Timestamp: 2026-01-12T14:25:05.723Z
Learning: In Python tests and utility code across the repository, bitmath.parse_string_unsafe correctly parses Kubernetes quantities (e.g., '4Gi', '512Mi', PVC storage requests) without supplying system=bitmath.NIST. There are 30+ usages indicating this is the standard behavior. Reviewers should verify that code that builds or compares quantity strings does not pass the NIST parameter, and if a new test relies on quantity parsing, assume no NIST parameter is required unless explicitly documented.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 3387
File: tests/network/provider_migration/libprovider.py:1-8
Timestamp: 2026-01-20T01:03:13.139Z
Learning: In the openshift-virtualization-tests repository, Python imports should consistently use module-level imports for the logging module (i.e., import logging) rather than from logging import ... The established pattern spans 270+ files and should not be flagged for refactoring. Apply this guideline to Python files across the repo (e.g., tests/network/provider_migration/libprovider.py).
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 3559
File: utilities/infra.py:251-254
Timestamp: 2026-01-21T21:26:41.805Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, when reviewing Python code, recognize that with Python 3.14 the syntax 'except ValueError, TypeError:' is valid if there is no 'as' clause, and should not be flagged as Python 2 syntax. If you use an 'as' binding (e.g., 'except (ValueError, TypeError) as e:'), parentheses are required. Ensure this pattern is version-consistent and not flagged as Python 2 syntax when 'as' is absent.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3847
File: tests/virt/cluster/common_templates/utils.py:58-58
Timestamp: 2026-02-18T06:34:38.042Z
Learning: In RedHatQE/openshift-virtualization-tests, treat 'public' functions as those defined in any Python files under libs/ or utilities/ (any depth). Functions inside nested test directories (e.g., tests/virt/cluster/common_templates/, tests/virt/node/, etc.) are test helpers and do not require Google-format docstrings unless explicitly requested. Use this rule during reviews to decide whether to enforce docstrings on public API functions in libs/utilities.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3847
File: utilities/virt.py:2449-2453
Timestamp: 2026-02-18T06:35:39.536Z
Learning: In Python code, a function named clearly and self-descriptively can be deemed not to require a docstring. However, treat this as a context-specific guideline and not a universal rule. For public APIs or functions with side effects, prefer concise docstrings explaining behavior, inputs, outputs, and side effects. This guidance is based on the example in utilities/virt.py from RedHatQE/openshift-virtualization-tests where validate_libvirt_persistent_domain(vm, admin_client) was considered self-documenting.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3883
File: utilities/pytest_utils.py:441-463
Timestamp: 2026-02-23T16:33:22.070Z
Learning: In Python code reviews, the guideline to always use named arguments for multi-argument calls does not apply to built-ins or methods that have positional-only parameters (those defined with a / in their signature). Do not flag or require named arguments for calls like dict.get(key, default=None, /), list.pop(), str.split(sep, maxsplit) and similar built-ins that cannot accept keyword arguments. Apply the named-argument rule only to functions/methods that explicitly accept keyword arguments.
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 4113
File: utilities/virt.py:148-151
Timestamp: 2026-03-09T18:30:09.993Z
Learning: In utilities/virt.py, when reading vmi.instance.status.interfaces, you can assume interfaces only contain guest OS interfaces as reported by the KubeVirt guest agent; pod-level host-network interfaces will not appear. Therefore code like all(iface.get('interfaceName') for iface in interfaces) is safe from external pollution. This guidance is specific to this repository and its VMI status handling.
|
|
||
| ```bash | ||
| # From repo root or from this directory | ||
| FEDORA_VERSION=41-1.4 ./containers/fedora_artifacts/build.sh |
Contributor
There was a problem hiding this comment.
Suggested change
| FEDORA_VERSION=41-1.4 ./containers/fedora_artifacts/build.sh | |
| FEDORA_VERSION=43-1.6 ./containers/fedora_artifacts/build.sh |
Member
Author
There was a problem hiding this comment.
Thanks for the suggestion. included.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
containers/fedora_artifacts/build.sh (1)
49-50:⚠️ Potential issue | 🟠 MajorHIGH: Keep canonical qcow2 filename; create alias instead of renaming.
Line 49-50 uses
mv, which deletes${qcow2_name}fromout/. Consumers still reference upstream names (for example fromutilities/constants.py), so artifact lookup can fail.Proposed fix
- echo "[${arch}] Renaming qcow2 to ${img_name}..." - mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}" + echo "[${arch}] Creating qcow2 alias ${img_name} (preserve canonical filename)..." + cp "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@containers/fedora_artifacts/build.sh` around lines 49 - 50, Don't rename the canonical qcow2 file; replace the mv call with creating an alias so the original ${qcow2_name} remains in ${OUT_DIR}. Specifically, in build.sh where you currently call mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}", create a symbolic link (or hard link if preferred) named "${OUT_DIR}/${img_name}" that points to "${OUT_DIR}/${qcow2_name}" (and handle preexisting "${img_name}" by removing it first), so consumers referencing the upstream ${qcow2_name} continue to work while you still provide ${img_name}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@containers/fedora_artifacts/build.sh`:
- Around line 49-50: Don't rename the canonical qcow2 file; replace the mv call
with creating an alias so the original ${qcow2_name} remains in ${OUT_DIR}.
Specifically, in build.sh where you currently call mv "${OUT_DIR}/${qcow2_name}"
"${OUT_DIR}/${img_name}", create a symbolic link (or hard link if preferred)
named "${OUT_DIR}/${img_name}" that points to "${OUT_DIR}/${qcow2_name}" (and
handle preexisting "${img_name}" by removing it first), so consumers referencing
the upstream ${qcow2_name} continue to work while you still provide ${img_name}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5864dc3b-e780-4030-8887-a8e0483e0aa7
⛔ Files ignored due to path filters (2)
containers/README.mdis excluded by!**/*.mdcontainers/fedora_artifacts/README.mdis excluded by!**/*.md
📒 Files selected for processing (2)
containers/fedora_artifacts/build.shutilities/virt.py
Signed-off-by: chandramerla <Chandra.Merla@ibm.com> Assisted-by: Cursor
The script to build Fedora image related artifacts is added. It downloads Fedora base image for a given version and prepare all artifacts which usually are required for tests in the artifactory: Fedora-Cloud-Base-Generic-43-1.6.s390x.qcow2.gz Fedora-Cloud-Base-Generic-43-1.6.s390x.qcow2.xz Fedora-Cloud-Base-Generic-43-1.6.s390x.raw Fedora-Cloud-Base-Generic-43-1.6.s390x.raw.gz Fedora-Cloud-Base-Generic-43-1.6.s390x.raw.xz Fedora-qcow2-s390x.img Signed-off-by: chandramerla <Chandra.Merla@ibm.com>
Fixes 'which' package missing on Fedora 43 and CentOS Stream 9 s390x images Signed-off-by: chandramerla <Chandra.Merla@ibm.com>
Member
Author
Added a comment here as a reply: #3760 (comment) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
utilities/virt.py (1)
2552-2554:⚠️ Potential issue | 🟠 MajorHIGH:
check_qemu_guest_agent_installedis now RPM-only and can return false negatives on non-RPM guests.Line 2553 hard-codes
rpm -q, so Alpine/Debian-based guests will report “not installed” even whenqemu-guest-agentexists. This helper is generic, so distro-coupling here creates fragile behavior as image mix evolves.Proposed fix
def check_qemu_guest_agent_installed(ssh_exec: Host) -> bool: - """Check if qemu-guest-agent is installed via rpm, bypassing rrmngmnt's `which`-based package manager detection.""" - rc, _, _ = ssh_exec.run_command(command=shlex.split("rpm -q qemu-guest-agent")) - return rc == 0 + """Check whether qemu-guest-agent is installed on the guest.""" + package_check_commands = [ + ["rpm", "-q", "qemu-guest-agent"], + ["dpkg-query", "-W", "-f=${Status}", "qemu-guest-agent"], + ["apk", "info", "-e", "qemu-guest-agent"], + ] + for package_check_command in package_check_commands: + rc, out, _ = ssh_exec.run_command(command=package_check_command) + if rc == 0 and ( + package_check_command[0] != "dpkg-query" or "installed" in out + ): + return True + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/virt.py` around lines 2552 - 2554, The helper check_qemu_guest_agent_installed is RPM-only and may return false negatives on non-RPM guests; update it to detect distro/package-manager or fall back to a distro-agnostic check: try rpm -q qemu-guest-agent first (using ssh_exec.run_command), then if that fails run a fallback like command -v/qemu-guest-agent or check common binary paths (/usr/sbin/qemu-guest-agent) and/or systemctl status/qemu-guest-agent.service; ensure the function (check_qemu_guest_agent_installed) returns true if any of these checks succeed and false only if all checks fail.containers/fedora_artifacts/build.sh (1)
49-50:⚠️ Potential issue | 🟠 MajorHIGH: Keep the upstream qcow2 name and create
Fedora-qcow2-${arch}.imgas an extra artifact.
mvremoves${qcow2_name}fromout/, bututilities/constants.pystill references the upstream Fedora filenames for boot sources. After upload, consumers looking forFedora-Cloud-Base-Generic-*.qcow2will fail. Create the alias without deleting the original file.Proposed fix
- echo "[${arch}] Renaming qcow2 to ${img_name}..." - mv "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}" + echo "[${arch}] Creating qcow2 alias ${img_name}..." + cp "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${img_name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@containers/fedora_artifacts/build.sh` around lines 49 - 50, The script currently renames the upstream qcow2 with mv which deletes the original referenced by utilities/constants.py; instead preserve the original and create the extra artifact by copying or linking "${OUT_DIR}/${qcow2_name}" to "${OUT_DIR}/${img_name}" (use cp or ln as appropriate) so Fedora-Cloud-Base-Generic-*.qcow2 remains in out/ while also producing Fedora-qcow2-${arch}.img; update the line that uses mv in build.sh to perform a copy/link and ensure variables arch, img_name, qcow2_name and OUT_DIR are used exactly as currently defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@containers/fedora_artifacts/build.sh`:
- Around line 35-39: Add a checksum verification step after downloading the
qcow2 and before running qemu-img: download the corresponding CHECKSUM (from the
same url_base using qcow2_name + ".CHECKSUM" or similar), verify it with
sha256sum --check (or the signed verification flow if available), and abort
(non-zero exit) if the check fails; update the wget/verification logic around
the existing variables/commands (qcow2_name, OUT_DIR, url_base, wget, sha256sum,
qemu-img, raw_name) so qemu-img convert only runs when the integrity check
passes.
In `@utilities/constants.py`:
- Around line 170-178: The Cirros image descriptor has a mixed Alpine/Fedora
configuration (Cirros has DISK_DEMO pointing to Fedora while DIR, QCOW2_IMG,
DEFAULT_MEMORY_SIZE, and OS_FLAVOR were changed to Alpine), which breaks
consumers that read multiple fields together (e.g., tests/utils.py and
utilities/storage.py); fix by splitting the workaround into two distinct
descriptors or restoring a consistent Fedora-backed Cirros entry: either create
a new Alpine-only descriptor (e.g., Images.CirrosAlpine or
Images.AlpineSubstitute) with BASE_VERSIONED_ALPINE_NAME, QCOW2_IMG, DIR,
DEFAULT_MEMORY_SIZE, and OS_FLAVOR set to Alpine and leave Cirros.DISk_DEMO,
DISK_DEMO, and any Fedora-specific fields on the original Cirros descriptor, or
revert the modified fields in Cirros back to the Fedora values so tests that
inspect Images.Cirros (DISK_DEMO, DIR, QCOW2_IMG, DEFAULT_MEMORY_SIZE,
OS_FLAVOR) remain consistent; update references in consumers if you introduce
the new descriptor.
---
Duplicate comments:
In `@containers/fedora_artifacts/build.sh`:
- Around line 49-50: The script currently renames the upstream qcow2 with mv
which deletes the original referenced by utilities/constants.py; instead
preserve the original and create the extra artifact by copying or linking
"${OUT_DIR}/${qcow2_name}" to "${OUT_DIR}/${img_name}" (use cp or ln as
appropriate) so Fedora-Cloud-Base-Generic-*.qcow2 remains in out/ while also
producing Fedora-qcow2-${arch}.img; update the line that uses mv in build.sh to
perform a copy/link and ensure variables arch, img_name, qcow2_name and OUT_DIR
are used exactly as currently defined.
In `@utilities/virt.py`:
- Around line 2552-2554: The helper check_qemu_guest_agent_installed is RPM-only
and may return false negatives on non-RPM guests; update it to detect
distro/package-manager or fall back to a distro-agnostic check: try rpm -q
qemu-guest-agent first (using ssh_exec.run_command), then if that fails run a
fallback like command -v/qemu-guest-agent or check common binary paths
(/usr/sbin/qemu-guest-agent) and/or systemctl status/qemu-guest-agent.service;
ensure the function (check_qemu_guest_agent_installed) returns true if any of
these checks succeed and false only if all checks fail.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f945a03e-a7ed-45d3-8581-51355328b132
⛔ Files ignored due to path filters (2)
containers/README.mdis excluded by!**/*.mdcontainers/fedora_artifacts/README.mdis excluded by!**/*.md
📒 Files selected for processing (4)
containers/fedora_artifacts/build.shtests/global_config_s390x.pyutilities/constants.pyutilities/virt.py
Comment on lines
+35
to
+39
| echo "[${arch}] Downloading base qcow2..." | ||
| wget -q -O "${OUT_DIR}/${qcow2_name}" "${url_base}/${qcow2_name}" | ||
|
|
||
| echo "[${arch}] Creating raw..." | ||
| qemu-img convert -O raw "${OUT_DIR}/${qcow2_name}" "${OUT_DIR}/${raw_name}" |
Contributor
There was a problem hiding this comment.
CRITICAL: Verify the downloaded qcow2 before converting or publishing it.
This script promotes a remote Fedora image into Artifactory without any integrity check. Published test images are part of the supply chain, so a corrupted mirror response or partial download will still be converted, compressed, and uploaded as a valid artifact. Download the matching CHECKSUM file and run sha256sum --check (or the signed equivalent) before qemu-img.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@containers/fedora_artifacts/build.sh` around lines 35 - 39, Add a checksum
verification step after downloading the qcow2 and before running qemu-img:
download the corresponding CHECKSUM (from the same url_base using qcow2_name +
".CHECKSUM" or similar), verify it with sha256sum --check (or the signed
verification flow if available), and abort (non-zero exit) if the check fails;
update the wget/verification logic around the existing variables/commands
(qcow2_name, OUT_DIR, url_base, wget, sha256sum, qemu-img, raw_name) so qemu-img
convert only runs when the integrity check passes.
Comment on lines
170
to
+178
| Cirros = Cirros( | ||
| # TODO: S390X does not support Cirros; this is a workaround until tests are moved to Fedora | ||
| RAW_IMG="Fedora-Cloud-Base-Generic-41-1.4.s390x.raw", | ||
| RAW_IMG_GZ="Fedora-Cloud-Base-Generic-41-1.4.s390x.raw.gz", | ||
| RAW_IMG_XZ="Fedora-Cloud-Base-Generic-41-1.4.s390x.raw.xz", | ||
| QCOW2_IMG="Fedora-Cloud-Base-Generic-41-1.4.s390x.qcow2", | ||
| QCOW2_IMG_GZ="Fedora-Cloud-Base-Generic-41-1.4.s390x.qcow2.gz", | ||
| QCOW2_IMG_XZ="Fedora-Cloud-Base-Generic-41-1.4.s390x.qcow2.xz", | ||
| # S390X does not support Cirros; Alpine is used as a lightweight substitute | ||
| RAW_IMG_XZ=f"{BASE_VERSIONED_ALPINE_NAME}.raw.xz", | ||
| QCOW2_IMG=f"{BASE_ALPINE_NAME}.qcow2", | ||
| DISK_DEMO=FEDORA_DISK_DEMO, | ||
| DIR=f"{BASE_IMAGES_DIR}/fedora-images", | ||
| DEFAULT_DV_SIZE="10Gi", | ||
| DEFAULT_MEMORY_SIZE="1Gi", | ||
| OS_FLAVOR=OS_FLAVOR_FEDORA, | ||
| DIR=f"{BASE_IMAGES_DIR}/alpine-images", | ||
| DEFAULT_DV_SIZE="1Gi", | ||
| DEFAULT_MEMORY_SIZE="128M", | ||
| OS_FLAVOR=OS_FLAVOR_ALPINE, |
Contributor
There was a problem hiding this comment.
HIGH: Don’t mix Fedora container-disk metadata with Alpine guest settings.
DISK_DEMO still points to the Fedora demo image, while DIR, QCOW2_IMG, DEFAULT_MEMORY_SIZE, and OS_FLAVOR now describe Alpine. Helpers such as tests/utils.py:551-571 and utilities/storage.py:978-989 already read multiple Images.Cirros fields together, so this descriptor is now internally inconsistent and easy to consume incorrectly. Split the workaround by source type, or keep the Fedora-backed fields together until the container-disk demo also moves. Based on learnings, Images.Cirros.OS_FLAVOR is consumed for conditional logic in tests, so mixing Fedora and Alpine fields on one image descriptor is unsafe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utilities/constants.py` around lines 170 - 178, The Cirros image descriptor
has a mixed Alpine/Fedora configuration (Cirros has DISK_DEMO pointing to Fedora
while DIR, QCOW2_IMG, DEFAULT_MEMORY_SIZE, and OS_FLAVOR were changed to
Alpine), which breaks consumers that read multiple fields together (e.g.,
tests/utils.py and utilities/storage.py); fix by splitting the workaround into
two distinct descriptors or restoring a consistent Fedora-backed Cirros entry:
either create a new Alpine-only descriptor (e.g., Images.CirrosAlpine or
Images.AlpineSubstitute) with BASE_VERSIONED_ALPINE_NAME, QCOW2_IMG, DIR,
DEFAULT_MEMORY_SIZE, and OS_FLAVOR set to Alpine and leave Cirros.DISk_DEMO,
DISK_DEMO, and any Fedora-specific fields on the original Cirros descriptor, or
revert the modified fields in Cirros back to the Fedora values so tests that
inspect Images.Cirros (DISK_DEMO, DIR, QCOW2_IMG, DEFAULT_MEMORY_SIZE,
OS_FLAVOR) remain consistent; update references in consumers if you introduce
the new descriptor.
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4404. Overlapping filesutilities/virt.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4463. Overlapping filesutilities/virt.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description:
As fedora 41 is out of support, moving to newer Fedora. As a first step (less changes) moved cirros workaround from Fedora to alpine, as now with internal-http-server:1.2.0 Fedora/Cirros is replaced with Alpine.
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Chores
Tests
Bug Fixes