Updated fedora container-disk and qcow2 to 43#3760
Updated fedora container-disk and qcow2 to 43#3760chandramerla wants to merge 3 commits intoRedHatQE:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughS390x image configuration switched from Fedora/Cirros workarounds to Alpine-derived Cirros substitutes, Fedora s390x references updated to Fedora 43, a new Fedora artifact build script was added, tests updated to Fedora 43, and qemu-guest-agent detection now uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
/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.66% +0.03%
==========================================
Files 25 25
Lines 2420 2476 +56
==========================================
+ Hits 2387 2443 +56
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
|
/build-and-push-container |
|
/wip |
|
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
|
/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}
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
chandramerla
force-pushed
the
test-fedora-43
branch
from
84f2b9e to
9909f7e
Compare
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3760 published |
|
D/S test |
|
/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: |
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: |
|
/wip cancel Please review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20967 |
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
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
Added a comment here as a reply: #3760 (comment) |
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
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4404. Overlapping filesutilities/virt.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4463. Overlapping filesutilities/virt.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4567. Overlapping filesutilities/virt.py |
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 45-48: The message printed when expected_hash is empty uses
"WARNING" but this branch is fatal; update the echo in the conditional that
checks expected_hash (the block referencing expected_hash, qcow2_name,
checksum_file) to use "ERROR:" instead of "WARNING:" so the log severity matches
the subsequent exit 1 and aligns with the other mismatch branch.
In `@utilities/constants.py`:
- Around line 171-180: Images.S390X.Cirros.DISK_DEMO is set to ALPINE_DISK_DEMO
which points to an image lacking s390x manifest and causes s390x tests (e.g.,
test_wffc_import_registry_dv marked with `@pytest.mark.s390x`) to fail; update the
constant/assignment so Images.S390X.Cirros.DISK_DEMO uses an image that has a
multi-architecture (includes s390x) manifest or replace it with a known
s390x-capable image (update ALPINE_DISK_DEMO or override the Cirros.DISK_DEMO
assignment), or alternatively remove the `@pytest.mark.s390x` marker from tests
that cannot run on s390x—ensure the change targets the ALPINE_DISK_DEMO constant
and the Images.S390X.Cirros.DISK_DEMO assignment.
🪄 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: 8a681043-9f8c-4b8c-a39e-ab3616323dc1
⛔ 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
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>
|
Clean rebase detected — no code changes compared to previous head ( |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3748. Overlapping filesutilities/constants.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4725. Overlapping filestests/global_config_s390x.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4746. Overlapping filesutilities/virt.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4756. Overlapping filesutilities/virt.py |
8 participants
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
New Features
Tests
Bug Fixes