[IUO] Apply new Lazy IDMS strategy for upgrade and install lanes#4453
Open
hmeir wants to merge 4 commits intoRedHatQE:mainfrom
Open
[IUO] Apply new Lazy IDMS strategy for upgrade and install lanes#4453hmeir wants to merge 4 commits intoRedHatQE:mainfrom
hmeir wants to merge 4 commits intoRedHatQE:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdded Konflux/IDMS constants, introduced a session fixture that queries Version Explorer (iib_build_info), and refactored IDMS mirror handling: new helper to merge per-image mirrors, changed apply_konflux_idms behavior, and updated fixtures to gate/skip IDMS updates based on pipeline and cluster state. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4453 +/- ##
==========================================
+ Coverage 98.63% 98.66% +0.02%
==========================================
Files 25 25
Lines 2420 2469 +49
==========================================
+ Hits 2387 2436 +49
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:
|
|
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
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-5
requested review from
OhadRevah,
dshchedr,
geetikakay,
rlobillo and
rnetser
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/install_upgrade_operators/product_upgrade/conftest.py`:
- Around line 125-140: The disconnected-cluster guard in updated_konflux_idms
runs too late because pytest resolves the iib_build_info fixture eagerly
(triggering Version Explorer lookups); change updated_konflux_idms to accept a
lazy provider (e.g., iib_build_info_getter callable/ lambda) or defer accessing
iib_build_info until after the is_disconnected_cluster check so the disconnected
check short-circuits before any Version Explorer logic; update callers/fixtures
to pass a zero-arg function (or move the disconnected check into the
iib_build_info fixture itself) and ensure all references in updated_konflux_idms
use the lazily obtained value when comparing to KONFLUX_PIPELINE.
In `@tests/install_upgrade_operators/utils.py`:
- Around line 347-352: The fallback IDMS created in the create path uses an
aggregate source (RH_IDMS_SOURCE) which is incompatible with the patch helper
_get_entries_with_missing_mirrors() that expects per-image sources of the form
registry.redhat.io/container-native-virtualization/<image>; fix by either (A)
changing the creation logic around ImageDigestMirrorSet (where
image_digest_mirrors is set and ImageDigestMirrorSet(...) is constructed) to
emit per-image entries (expand required_mirrors into entries like
RH_IDMS_SOURCE/<image> for each image name) or (B) updating
_get_entries_with_missing_mirrors() to detect the aggregate fallback (source
equals RH_IDMS_SOURCE with no path component) and handle it as a special case
(skip per-image parsing and apply mirrors appropriately) so it doesn’t derive
malformed image_name values.
- Around line 309-312: The current check uses any(m.startswith(url) for m in
mirrors) which matches any image under the same version prefix; instead compute
the full candidate mirror string (f"{url}/{image_name}") and check existence
per-image (e.g., any(m == candidate for m in mirrors) or
any(m.startswith(candidate) if tags/digests may follow) ) when building missing
from required_mirrors; then set entry["mirrors"] = mirrors + missing and
has_changes = True as before. Ensure you reference the local variables
required_mirrors, image_name, mirrors, missing, and entry["mirrors"] when making
the change.
🪄 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: 8b1962de-bc7f-41fb-871e-cb115b317c18
📒 Files selected for processing (5)
tests/install_upgrade_operators/constants.pytests/install_upgrade_operators/product_install/conftest.pytests/install_upgrade_operators/product_upgrade/conftest.pytests/install_upgrade_operators/utils.pyutilities/pytest_utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4467. Overlapping filestests/install_upgrade_operators/constants.py |
hmeir
force-pushed
the
idms-modification
branch
from
244c596 to
d0a45ce
Compare
hmeir
changed the title
openshift-virtualization-qe-bot-3
removed
the
changes-requested-coderabbitai[bot]
label
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utilities/pytest_utils.py (1)
269-275: 🛠️ Refactor suggestion | 🟠 MajorMEDIUM: Add the required public-helper docstring.
This public utility now has non-obvious behavior: it may return a URL, return
None, or raise based on pytest flags andCNV_VERSION_EXPLORER_URL. Documenting that contract prevents callers from treating CNV upgrade runs like the non-upgrade path.Proposed docstring
def get_cnv_version_explorer_url(pytest_config): + """Return the CNV Version Explorer URL for install and selected upgrade flows. + + Args: + pytest_config: Pytest config object. + + Returns: + CNV Version Explorer URL from `CNV_VERSION_EXPLORER_URL` when `--install` + is used or `--upgrade` is `eus` or `cnv`; otherwise `None`. + + Raises: + MissingEnvironmentVariableError: If the URL is required but the environment + variable is unset. + """ if pytest_config.getoption("install") or pytest_config.getoption("upgrade") in ("eus", "cnv"): LOGGER.info("Checking for cnv version explorer url:") version_explorer_url = os.environ.get("CNV_VERSION_EXPLORER_URL")Based on learnings, public functions are those defined under
libs/orutilities/; As per coding guidelines, “Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/pytest_utils.py` around lines 269 - 275, The function get_cnv_version_explorer_url lacks a Google-style docstring describing its non-obvious contract: depending on pytest flags it may return a URL string, return None, or raise MissingEnvironmentVariableError if CNV_VERSION_EXPLORER_URL is unset during CNV-related install/upgrade; add a Google-format docstring immediately above get_cnv_version_explorer_url that documents parameters (pytest_config), the return value (str or None), the conditions under which it raises MissingEnvironmentVariableError, and any side effects (reads CNV_VERSION_EXPLORER_URL and calls LOGGER.info) so callers understand the different behaviors.tests/install_upgrade_operators/product_upgrade/conftest.py (1)
426-428:⚠️ Potential issue | 🟠 MajorHIGH: include the brew fallback mirror for new EUS IDMS resources.
The non-EUS path adds
BREW_MIRROR_BASE_URLwhen the IDMS does not exist, but the EUS path creates a fresh fallback IDMS with only Konflux version mirrors. Fresh EUS runs can miss the brew fallback mirror and diverge from the non-EUS behavior.Proposed fix
apply_konflux_idms( idms=idms, - required_mirrors=required_mirrors, + required_mirrors=required_mirrors if idms.exists else required_mirrors + [BREW_MIRROR_BASE_URL], machine_config_pools=machine_config_pools, mcp_conditions=machine_config_pools_conditions_scope_module, nodes=nodes,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install_upgrade_operators/product_upgrade/conftest.py` around lines 426 - 428, The EUS branch that creates a fresh fallback IDMS is missing the brew fallback mirror; update the logic in apply_konflux_idms (and its callers that build required_mirrors) to ensure BREW_MIRROR_BASE_URL is appended to the mirror list when creating new EUS IDMS resources just like the non-EUS path does; locate the code paths that build required_mirrors and the EUS-specific conditional and add the same fallback mirror insertion so fresh EUS runs include the brew fallback.
♻️ Duplicate comments (1)
tests/install_upgrade_operators/utils.py (1)
293-315:⚠️ Potential issue | 🟠 MajorHIGH: update the aggregate fallback entry instead of treating it as complete.
Line 349 creates a fallback entry with
source == RH_IDMS_SOURCE, but lines 305-314 only updateRH_IDMS_SOURCE/<image>entries. On the next run, that fallback IDMS is ignored and line 340 returns early, so new target-version mirrors are never added.Proposed fix
- for entry in mirror_entries: - if entry["source"].startswith(f"{RH_IDMS_SOURCE}/"): - image_name = entry["source"].removeprefix(f"{RH_IDMS_SOURCE}/") - mirrors = entry.get("mirrors", []) - missing = [ - expected_mirror for url in required_mirrors if (expected_mirror := f"{url}/{image_name}") not in mirrors - ] - if missing: - entry["mirrors"] = mirrors + missing - has_changes = True + for entry in mirror_entries: + mirrors = entry.get("mirrors", []) + if entry["source"] == RH_IDMS_SOURCE: + missing = [url for url in required_mirrors if url not in mirrors] + elif entry["source"].startswith(f"{RH_IDMS_SOURCE}/"): + image_name = entry["source"].removeprefix(f"{RH_IDMS_SOURCE}/") + missing = [ + expected_mirror for url in required_mirrors if (expected_mirror := f"{url}/{image_name}") not in mirrors + ] + else: + continue + + if missing: + entry["mirrors"] = mirrors + missing + has_changes = TrueAlso applies to: 349-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install_upgrade_operators/utils.py` around lines 293 - 315, _in _get_entries_with_missing_mirrors_, the code only updates entries whose source startswith RH_IDMS_SOURCE + "/", so the aggregate fallback entry whose source == RH_IDMS_SOURCE is left unchanged and future runs skip adding new target-version mirrors; fix by handling the exact-match fallback: inside the loop over mirror_entries (refer to variables entry, entry["source"], mirrors, missing, has_changes) add a branch for entry["source"] == RH_IDMS_SOURCE that computes missing mirrors from required_mirrors (use the base URLs themselves, not per-image paths) and appends them to entry["mirrors"], otherwise keep the existing logic for RH_IDMS_SOURCE/<image> where you build expected_mirror as f"{url}/{image_name}"; set has_changes when you append mirrors and return mirror_entries if has_changes else [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/install_upgrade_operators/product_upgrade/conftest.py`:
- Around line 426-428: The EUS branch that creates a fresh fallback IDMS is
missing the brew fallback mirror; update the logic in apply_konflux_idms (and
its callers that build required_mirrors) to ensure BREW_MIRROR_BASE_URL is
appended to the mirror list when creating new EUS IDMS resources just like the
non-EUS path does; locate the code paths that build required_mirrors and the
EUS-specific conditional and add the same fallback mirror insertion so fresh EUS
runs include the brew fallback.
In `@utilities/pytest_utils.py`:
- Around line 269-275: The function get_cnv_version_explorer_url lacks a
Google-style docstring describing its non-obvious contract: depending on pytest
flags it may return a URL string, return None, or raise
MissingEnvironmentVariableError if CNV_VERSION_EXPLORER_URL is unset during
CNV-related install/upgrade; add a Google-format docstring immediately above
get_cnv_version_explorer_url that documents parameters (pytest_config), the
return value (str or None), the conditions under which it raises
MissingEnvironmentVariableError, and any side effects (reads
CNV_VERSION_EXPLORER_URL and calls LOGGER.info) so callers understand the
different behaviors.
---
Duplicate comments:
In `@tests/install_upgrade_operators/utils.py`:
- Around line 293-315: _in _get_entries_with_missing_mirrors_, the code only
updates entries whose source startswith RH_IDMS_SOURCE + "/", so the aggregate
fallback entry whose source == RH_IDMS_SOURCE is left unchanged and future runs
skip adding new target-version mirrors; fix by handling the exact-match
fallback: inside the loop over mirror_entries (refer to variables entry,
entry["source"], mirrors, missing, has_changes) add a branch for entry["source"]
== RH_IDMS_SOURCE that computes missing mirrors from required_mirrors (use the
base URLs themselves, not per-image paths) and appends them to entry["mirrors"],
otherwise keep the existing logic for RH_IDMS_SOURCE/<image> where you build
expected_mirror as f"{url}/{image_name}"; set has_changes when you append
mirrors and return mirror_entries if has_changes else [].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6217507-43c8-4f0b-a58e-88badbf3f11d
📒 Files selected for processing (6)
tests/install_upgrade_operators/conftest.pytests/install_upgrade_operators/constants.pytests/install_upgrade_operators/product_install/conftest.pytests/install_upgrade_operators/product_upgrade/conftest.pytests/install_upgrade_operators/utils.pyutilities/pytest_utils.py
💤 Files with no reviewable changes (1)
- tests/install_upgrade_operators/product_install/conftest.py
hmeir
force-pushed
the
idms-modification
branch
from
d0a45ce to
6588b17
Compare
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
Signed-off-by: Harel Meir <hmeir@redhat.com>
EUS: assert ImageDigestMirrorSet has at least the two expected mirror entries for the upgrade path. Install: skip applying Konflux IDMS in tests; deployment infra already provides IDMS entries aligned with the installed version. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Code (claude-opus-4-6) <noreply@anthropic.com>
hmeir
force-pushed
the
idms-modification
branch
from
6588b17 to
d447442
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
- _get_entries_with_missing_mirrors now handles both aggregate source entries (source == RH_IDMS_SOURCE) and per-image entries, so an IDMS created via the fallback path can be patched on subsequent runs. - eus_updated_konflux_idms adds BREW_MIRROR_BASE_URL on create, matching the existing behavior in updated_konflux_idms. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Code (claude-opus-4-6) <noreply@anthropic.com>
Moves duplicated logic from both upgrade fixtures into the utility function: IDMS object creation, existence check for the brew mirror fallback, and the KONFLUX_IDMS_NAME constant usage. Callers now pass admin_client instead of a pre-built IDMS object. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Code (claude-opus-4-6) <noreply@anthropic.com>
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
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
6 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:
This PR modifies the IDMS patching logic to follow a per-image strategy, aligning with Konflux build requirements.
More details:
This change aligns the code with the DevOps per-image IDMS strategy to reduce execution time and avoid unnecessary VM migrations that cause PSI execution failures.
What this PR does / why we need it:
Special notes for reviewer:
Review the logic in the IDMS patching helper functions to ensure the image version check is robust.
jira-ticket:
🤖 Assisted with Claude Code