Open
Conversation
Signed-off-by: Harel Meir <hmeir@redhat.com>
Contributor
📝 WalkthroughWalkthroughThis PR refactors CNV upgrade testing infrastructure by introducing EUS upgrade stream detection, reorganizing fixture dependencies with production source awareness, and decomposing install plan creation into a separate gated test. It also refactors IDMS mirror application logic to conditionally update existing entries, adds Konflux-related constants, and enhances subscription-scoped fixture wiring throughout the upgrade test suite. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4394 +/- ##
==========================================
+ Coverage 98.63% 98.65% +0.02%
==========================================
Files 25 25
Lines 2420 2460 +40
==========================================
+ Hits 2387 2427 +40
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
requested review from
RoniKishner,
SamAlber,
SiboWang1997,
albarker-rh,
geetikakay,
jerry7z,
kbidarkar,
rnetser and
vsibirsk
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
1788-1793:⚠️ Potential issue | 🔴 CriticalCRITICAL: EUS branch is unreachable because Line 1788 matches first.
At Line 1788, any
target.minor > current.minorreturnsY_STREAM, so the EUS condition at Lines 1792-1793 never executes.
Why this matters: specific routing rules must be evaluated before generic ones, or the specific path is effectively dead code.💡 Proposed fix
- elif current_cnv_version.minor < target_cnv_version.minor: - return UpgradeStreams.Y_STREAM + elif ( + current_cnv_version.major == target_cnv_version.major + and target_cnv_version.minor % 2 == 0 + and target_cnv_version.minor - current_cnv_version.minor == 2 + ): + return UpgradeStreams.EUS + elif current_cnv_version.minor < target_cnv_version.minor: + return UpgradeStreams.Y_STREAM elif current_cnv_version.micro < target_cnv_version.micro: return UpgradeStreams.Z_STREAM - elif target_cnv_version.minor % 2 == 0 and target_cnv_version.minor - 2 == current_cnv_version.minor: - return UpgradeStreams.EUS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 1788 - 1793, The EUS branch (checking target_cnv_version.minor % 2 == 0 and target_cnv_version.minor - 2 == current_cnv_version.minor which returns UpgradeStreams.EUS) is unreachable because the earlier condition "elif current_cnv_version.minor < target_cnv_version.minor: return UpgradeStreams.Y_STREAM" catches any target.minor > current.minor first; fix by evaluating the specific EUS case before the generic minor-increase check (i.e., check the EUS condition involving current_cnv_version and target_cnv_version first, then fall back to the generic current_cnv_version.minor < target_cnv_version.minor -> UpgradeStreams.Y_STREAM), keeping all version symbol names exactly as used (current_cnv_version, target_cnv_version, UpgradeStreams.Y_STREAM, UpgradeStreams.EUS).
🤖 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 136-142: The change reversed the intended logic so
updated_konflux_idms no longer runs for disconnected clusters: ensure the
function runs only when is_disconnected_cluster is True and
iib_build_info.get("pipeline") == KONFLUX_PIPELINE; i.e., remove or invert the
early return that currently exits when is_disconnected_cluster is True and
instead only return when NOT disconnected OR when the pipeline is not
KONFLUX_PIPELINE so updated_konflux_idms is invoked for disconnected Konflux
installs.
In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py`:
- Around line 25-49: The test test_cnv_upgrade_install_plan_creation now
declares shared setup fixtures (disabled_default_sources_in_operatorhub,
updated_konflux_idms, updated_custom_hco_catalog_source_image,
updated_cnv_subscription_source) and is marked `@pytest.mark.order`("first"),
which makes downstream tests (test_cnv_upgrade_process,
test_production_source_cnv_upgrade_process) dependent on its side-effects; to
fix, restore independence by removing those shared fixtures from
test_cnv_upgrade_install_plan_creation and re-attaching them to the actual
upgrade tests (test_cnv_upgrade_process and
test_production_source_cnv_upgrade_process), or explicitly declare a
pytest-dependency on those downstream tests using
`@pytest.mark.dependency`(depends=["test_cnv_upgrade_install_plan_creation"]) on
each dependent test and add a one-line comment explaining WHY the dependency
exists (that these tests require the catalog/operatorhub changes as
side-effects); ensure you keep the `@pytest.mark.order` only if ordering is still
needed.
In `@tests/install_upgrade_operators/utils.py`:
- Around line 99-101: The code dereferences
cnv_subscription.instance.status.installplan before getattr runs causing
AttributeError when status or installplan is None; update the assignment for
install_plan_name_in_subscription inside the install_plan_sampler loop to safely
traverse the nested attributes (e.g., check for None or use nested getattr
calls) so you only access .name when installplan exists on
cnv_subscription.instance.status; target the install_plan_sampler loop and the
variable install_plan_name_in_subscription to implement the guarded lookup.
- Around line 314-321: The code currently treats any mirror that startswith a
base URL as satisfying per-image mirrors; change the presence check to look for
the exact per-image mirror (construct expected = f"{url}/{image_name}") rather
than using startswith. In the block handling entry["source"]/image_name and
mirrors, replace the any(m.startswith(url) for m in mirrors) test with a strict
equality check against the constructed per-image URL (normalizing trailing
slashes if needed) so missing becomes only those exact per-image URLs and then
append those exact URLs to entry["mirrors"] and set has_changes accordingly.
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 1788-1793: The EUS branch (checking target_cnv_version.minor % 2
== 0 and target_cnv_version.minor - 2 == current_cnv_version.minor which returns
UpgradeStreams.EUS) is unreachable because the earlier condition "elif
current_cnv_version.minor < target_cnv_version.minor: return
UpgradeStreams.Y_STREAM" catches any target.minor > current.minor first; fix by
evaluating the specific EUS case before the generic minor-increase check (i.e.,
check the EUS condition involving current_cnv_version and target_cnv_version
first, then fall back to the generic current_cnv_version.minor <
target_cnv_version.minor -> UpgradeStreams.Y_STREAM), keeping all version symbol
names exactly as used (current_cnv_version, target_cnv_version,
UpgradeStreams.Y_STREAM, UpgradeStreams.EUS).
🪄 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: e52462b6-b2ba-47eb-a4db-55659b48c568
📒 Files selected for processing (11)
tests/conftest.pytests/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/product_upgrade/test_upgrade.pytests/install_upgrade_operators/product_upgrade/utils.pytests/install_upgrade_operators/utils.pytests/virt/upgrade/test_upgrade_virt.pyutilities/constants.pyutilities/pytest_utils.py
Comment on lines
136
to
142
| if is_disconnected_cluster: | ||
| LOGGER.warning("Skip applying IDMS in a disconnected setup.") | ||
| return | ||
|
|
||
| if cnv_source == HOTFIX_STR: | ||
| LOGGER.info("IDMS updates skipped as upgrading using production source/upgrade to hotfix") | ||
| if iib_build_info.get("pipeline") != KONFLUX_PIPELINE: | ||
| LOGGER.warning(f"Pipeline is '{iib_build_info.get('pipeline')}', not Konflux. Skipping IDMS.") | ||
| return |
Contributor
There was a problem hiding this comment.
CRITICAL: This reverses the IDMS setup condition.
updated_konflux_idms used to apply mirrors only for disconnected clusters, which is where the IDMS is needed. The new early return on is_disconnected_cluster skips the patch in exactly that environment, so the install-plan precheck and later upgrade flow run without the required mirror configuration.
Proposed fix
- if is_disconnected_cluster:
- LOGGER.warning("Skip applying IDMS in a disconnected setup.")
+ if not is_disconnected_cluster:
+ LOGGER.info("Connected setup; skipping Konflux IDMS update.")
return🧰 Tools
🪛 Ruff (0.15.9)
[warning] 141-141: Logging statement uses f-string
(G004)
🤖 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 136
- 142, The change reversed the intended logic so updated_konflux_idms no longer
runs for disconnected clusters: ensure the function runs only when
is_disconnected_cluster is True and iib_build_info.get("pipeline") ==
KONFLUX_PIPELINE; i.e., remove or invert the early return that currently exits
when is_disconnected_cluster is True and instead only return when NOT
disconnected OR when the pipeline is not KONFLUX_PIPELINE so
updated_konflux_idms is invoked for disconnected Konflux installs.
Comment on lines
+25
to
+49
| @pytest.mark.gating | ||
| @pytest.mark.cnv_upgrade | ||
| @pytest.mark.order("first") | ||
| @pytest.mark.polarion("CNV-12451") | ||
| @pytest.mark.usefixtures( | ||
| "cnv_upgrade_stream", | ||
| "disabled_default_sources_in_operatorhub", | ||
| "updated_konflux_idms", | ||
| "updated_custom_hco_catalog_source_image", | ||
| "updated_cnv_subscription_source", | ||
| ) | ||
| def test_cnv_upgrade_install_plan_creation( | ||
| admin_client, | ||
| hco_namespace, | ||
| hco_target_csv_name, | ||
| is_production_source, | ||
| cnv_subscription_scope_session, | ||
| ): | ||
| wait_for_install_plan( | ||
| client=admin_client, | ||
| hco_namespace=hco_namespace.name, | ||
| hco_target_csv_name=hco_target_csv_name, | ||
| is_production_source=is_production_source, | ||
| cnv_subscription=cnv_subscription_scope_session, | ||
| ) |
Contributor
There was a problem hiding this comment.
HIGH: This precheck is ordered first, but the downstream CNV tests now depend on its side effects.
After moving disabled_default_sources_in_operatorhub, updated_konflux_idms, updated_custom_hco_catalog_source_image, and updated_cnv_subscription_source onto this test, test_cnv_upgrade_process() and test_production_source_cnv_upgrade_process() are no longer independently runnable. @pytest.mark.order("first") only changes order; it does not stop later tests from running without this setup when the precheck is deselected, rerun selectively, or fails. Keep the shared setup fixtures on the upgrade tests too, or add an explicit dependency contract for the test-to-test gating.
As per coding guidelines "Tests MUST be independent; use pytest-dependency ONLY when test B requires side effects from test A; when using @pytest.mark.dependency, a comment explaining WHY the dependency exists is REQUIRED".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py` around lines
25 - 49, The test test_cnv_upgrade_install_plan_creation now declares shared
setup fixtures (disabled_default_sources_in_operatorhub, updated_konflux_idms,
updated_custom_hco_catalog_source_image, updated_cnv_subscription_source) and is
marked `@pytest.mark.order`("first"), which makes downstream tests
(test_cnv_upgrade_process, test_production_source_cnv_upgrade_process) dependent
on its side-effects; to fix, restore independence by removing those shared
fixtures from test_cnv_upgrade_install_plan_creation and re-attaching them to
the actual upgrade tests (test_cnv_upgrade_process and
test_production_source_cnv_upgrade_process), or explicitly declare a
pytest-dependency on those downstream tests using
`@pytest.mark.dependency`(depends=["test_cnv_upgrade_install_plan_creation"]) on
each dependent test and add a one-line comment explaining WHY the dependency
exists (that these tests require the catalog/operatorhub changes as
side-effects); ensure you keep the `@pytest.mark.order` only if ordering is still
needed.
Comment on lines
99
to
+101
| for install_plan_samples in install_plan_sampler: | ||
| # wait for the install plan to be created and updated in the subscription. | ||
| install_plan_name_in_subscription = getattr(subscription.instance.status.installplan, "name", None) | ||
| install_plan_name_in_subscription = getattr(cnv_subscription.instance.status.installplan, "name", None) |
Contributor
There was a problem hiding this comment.
CRITICAL: Guard the subscription InstallPlan lookup while it is still unset.
Line 101 still dereferences .status.installplan before getattr() runs. On the first sampler iterations the Subscription often has no InstallPlan yet, so this raises AttributeError instead of continuing to wait.
Proposed fix
- install_plan_name_in_subscription = getattr(cnv_subscription.instance.status.installplan, "name", None)
+ subscription_status = getattr(cnv_subscription.instance, "status", None)
+ install_plan_ref = getattr(subscription_status, "installplan", None)
+ install_plan_name_in_subscription = getattr(install_plan_ref, "name", None)📝 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
| for install_plan_samples in install_plan_sampler: | |
| # wait for the install plan to be created and updated in the subscription. | |
| install_plan_name_in_subscription = getattr(subscription.instance.status.installplan, "name", None) | |
| install_plan_name_in_subscription = getattr(cnv_subscription.instance.status.installplan, "name", None) | |
| for install_plan_samples in install_plan_sampler: | |
| # wait for the install plan to be created and updated in the subscription. | |
| subscription_status = getattr(cnv_subscription.instance, "status", None) | |
| install_plan_ref = getattr(subscription_status, "installplan", None) | |
| install_plan_name_in_subscription = getattr(install_plan_ref, "name", None) |
🤖 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 99 - 101, The code
dereferences cnv_subscription.instance.status.installplan before getattr runs
causing AttributeError when status or installplan is None; update the assignment
for install_plan_name_in_subscription inside the install_plan_sampler loop to
safely traverse the nested attributes (e.g., check for None or use nested
getattr calls) so you only access .name when installplan exists on
cnv_subscription.instance.status; target the install_plan_sampler loop and the
variable install_plan_name_in_subscription to implement the guarded lookup.
Comment on lines
+314
to
+321
| if entry["source"].startswith(RH_IDMS_SOURCE): | ||
| image_name = entry["source"].removeprefix(f"{RH_IDMS_SOURCE}/") | ||
| mirrors = entry.get("mirrors", []) | ||
| missing = [f"{url}/{image_name}" for url in required_mirrors if not any(m.startswith(url) for m in mirrors)] | ||
| if missing: | ||
| entry["mirrors"] = mirrors + missing | ||
| has_changes = True | ||
| return mirror_entries if has_changes else [] |
Contributor
There was a problem hiding this comment.
HIGH: Base-URL matches hide missing per-image mirrors.
Line 317 treats any mirror that merely starts with the version base URL as “already present”. An existing mirror like .../v4-22 will therefore suppress adding the required per-image mirror .../v4-22/virt-api-rhel9, so older coarse-grained entries never get upgraded to the per-image layout. This is especially easy to hit because the create path at Lines 355-360 seeds those base URLs.
Proposed fix
- missing = [f"{url}/{image_name}" for url in required_mirrors if not any(m.startswith(url) for m in mirrors)]
+ missing = [
+ f"{url}/{image_name}"
+ for url in required_mirrors
+ if f"{url}/{image_name}" not in mirrors
+ ]🤖 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 314 - 321, The code
currently treats any mirror that startswith a base URL as satisfying per-image
mirrors; change the presence check to look for the exact per-image mirror
(construct expected = f"{url}/{image_name}") rather than using startswith. In
the block handling entry["source"]/image_name and mirrors, replace the
any(m.startswith(url) for m in mirrors) test with a strict equality check
against the constructed per-image URL (normalizing trailing slashes if needed)
so missing becomes only those exact per-image URLs and then append those exact
URLs to entry["mirrors"] and set has_changes accordingly.
hmeir
force-pushed
the
install-plan-upgrade-test
branch
from
c2b697a to
6ae3da0
Compare
openshift-virtualization-qe-bot-4
added
size/L
and removed
changes-requested-coderabbitai[bot]
size/XL
labels
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 #4365. Overlapping filestests/conftest.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
5 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 implements an early InstallPlan validation test to catch configuration errors immediately at the start of the execution suite.
More details:
To prevent failing after 1+ hours of runtime due to incorrect channels or images, this PR introduces a "fail early" mechanism. A new InstallPlan validation test is added to the start of the suite to verify that the correct InstallPlan is created according to the source/target versions and channels before proceeding.
New Execution Order:
InstallPlan test -> pre-upgrade tests -> cnv upgrade test -> post upgrade tests
What this PR does / why we need it:
Special notes for reviewer:
Focuses specifically on the new validation logic in the initial test phase.
jira-ticket: