Replacing pod_validation tests with deployment/daemonset tests#3580
Open
albarker-rh wants to merge 17 commits intoRedHatQE:mainfrom
Open
Replacing pod_validation tests with deployment/daemonset tests#3580albarker-rh wants to merge 17 commits intoRedHatQE:mainfrom
albarker-rh wants to merge 17 commits intoRedHatQE:mainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRemoved CNV pod-validation tests and their helpers, deleted related pod-validation utilities and constants, migrated resource- and image-validation helpers to a shared tests utils module, updated tests to use the new utilities (daemonset/deployment), removed pod-related label mappings and global ALL_CNV_PODS references, and adjusted an infra exception catch. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Contributor
Author
|
/build-and-push-container |
openshift-virtualization-qe-bot-5
requested review from
OhadRevah,
RoniKishner,
dshchedr and
vsibirsk
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
previously approved these changes
Jan 23, 2026
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3580 published |
![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
🤖 Fix all issues with AI agents
In `@tests/install_upgrade_operators/pod_validation/utils.py`:
- Around line 87-95: The comprehension for
cnv_pods_env_with_upstream_image_reference calls
get_resource_container_env_image_mismatch(container=container) twice for each
container; change it to call the function once by using the walrus operator (:=)
inside the inner comprehension to bind the result (e.g., mismatch :=
get_resource_container_env_image_mismatch(container=container)) and then use
mismatch both in the if condition and as the value, keeping the overall
comprehension structure and keys (pod.name and container["name"]) the same.
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/utils.py`:
- Around line 330-357: Add Google-style docstrings to the two public assertion
helpers assert_cnv_resource_container_env_image_not_in_upstream and
assert_cnv_resource_container_image_not_in_upstream: for each function document
purpose (asserts no upstream image references in container env or image fields),
parameters (resource: Deployment | DaemonSet, describing that
resource.instance.spec.template.spec.containers is inspected), return value
(None) and side effects, and include the Raises section to state that
ResourceMismatch is raised when upstream image references are found; ensure
docstrings are placed immediately above each function definition and follow the
project's Google docstring formatting conventions.
- Around line 306-327: The code is brittle because it indexes nested dicts and
uses cpu_value_pattern.findall(value)[0] without guards; update the validation
in the block around missing_cpu_memory_values, cpu_values, cpu_value_pattern and
invalid_cpus to (1) safely access nested keys using .get and default empty dicts
so missing "resources" or "requests" won't raise, (2) build cpu_values only for
containers where a cpu value is present and a string-like value is available,
(3) use cpu_value_pattern.findall(value) and check that the result is non-empty
before converting to int, and (4) catch ValueError/TypeError when parsing to int
and treat those entries as invalid; finally, if any invalid entries exist raise
ResourceMismatch (same symbol) with a clear map of offending container names and
their problematic cpu values.
In `@utilities/infra.py`:
- Around line 214-216: The except block that handles disappearing pods currently
only catches ResourceNotFoundError, but get_pod_container_error_status() can
raise NotFoundError as well; update the exception handling in the sanity loop
(the try/except surrounding the pod check where pods_not_running is appended) to
catch both ResourceNotFoundError and NotFoundError (e.g., except
(ResourceNotFoundError, NotFoundError):), log the same warning referencing
pod.name, and append the same {pod.name: "Deleted"} entry so disappearing pods
are tracked instead of aborting the check.
🪄 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: 76279748-15be-4d24-900c-3f4e2db45735
📒 Files selected for processing (14)
tests/global_config.pytests/global_config_sno_hpp.pytests/install_upgrade_operators/conftest.pytests/install_upgrade_operators/constants.pytests/install_upgrade_operators/daemonset/test_daemonset_params.pytests/install_upgrade_operators/deployment/test_hco_deployment_params.pytests/install_upgrade_operators/deployment/utils.pytests/install_upgrade_operators/pod_validation/test_pod_spec.pytests/install_upgrade_operators/pod_validation/utils.pytests/install_upgrade_operators/relationship_labels/constants.pytests/install_upgrade_operators/relationship_labels/test_relationship_labels.pytests/install_upgrade_operators/utils.pyutilities/constants.pyutilities/infra.py
💤 Files with no reviewable changes (7)
- utilities/constants.py
- tests/install_upgrade_operators/relationship_labels/test_relationship_labels.py
- tests/global_config.py
- tests/install_upgrade_operators/pod_validation/utils.py
- tests/install_upgrade_operators/pod_validation/test_pod_spec.py
- tests/install_upgrade_operators/relationship_labels/constants.py
- tests/install_upgrade_operators/deployment/utils.py
Contributor
Author
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3580 published |
Contributor
Author
|
/verified on |
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: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2058 🏁 Script executed: Length of output: 1867 🏁 Script executed: Length of output: 61 🧠 Learnings used |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4156. Overlapping filestests/install_upgrade_operators/conftest.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3755. Overlapping filestests/global_config.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4171. Overlapping filesutilities/constants.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4169. Overlapping filesutilities/constants.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4267. Overlapping filesutilities/constants.py |
Contributor
Author
|
/wip |
Contributor
Author
|
/wip cancel |
Contributor
Author
|
/retest dco |
|
No dco configured for this repository |
Contributor
Author
|
/retest all |
rnetser
requested changes
Apr 16, 2026
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4467. Overlapping filestests/install_upgrade_operators/constants.py |
OhadRevah
suggested changes
May 4, 2026
Comment on lines
+41
to
+52
| if cnv_daemonset_by_name.name.startswith(HOSTPATH_PROVISIONER_CSI): | ||
| assert not cnv_daemonset_by_name.instance.spec.template.spec.priorityClassName, ( | ||
| "HPP daemonset shouldn't have priority class name." | ||
| ) | ||
| elif not cnv_daemonset_by_name.instance.spec.template.spec.priorityClassName: | ||
| pytest.fail( | ||
| f"For daemonset {cnv_daemonset_by_name.name}, spec.template.spec.priorityClassName has not been set." | ||
| ) | ||
| elif cnv_daemonset_by_name.instance.spec.template.spec.priorityClassName not in VALID_PRIORITY_CLASS: | ||
| pytest.fail( | ||
| f"For daemonset {cnv_daemonset_by_name.name}, \ | ||
| unexpected priority class found: {cnv_daemonset_by_name.instance.spec.template.spec.priorityClassName}" |
Contributor
There was a problem hiding this comment.
save cnv_daemonset_by_name.instance.spec.template.spec.priorityClassName and reuse to save api calls, there is no need to call it several times.
Comment on lines
+66
to
+69
| elif cnv_deployment_by_name.instance.spec.template.spec.priorityClassName not in VALID_PRIORITY_CLASS: | ||
| pytest.fail( | ||
| f"For cnv deployment {cnv_deployment_by_name.name}, \ | ||
| unexpected priority class found: {cnv_deployment_by_name.instance.spec.template.spec.priorityClassName}" |
Contributor
There was a problem hiding this comment.
cnv_deployment_by_name.instance.spec.template.spec.priorityClassName also here, save and reuse, or if you see that you are using it accross multiple tests, maybe do a fixture for it and reuse, there is no need to do multiple api calls for getting the same result.
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4725. Overlapping filestests/global_config.py |
Co-authored-by: Ohad Revah <orevah@redhat.com>
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
11 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:
pod_validation tests rely on a list of constants which requires manual upkeep to prevent failures. Removing the directory will improve lane success
More details:
Removing these tests requires more coverage from deployment and daemonset tests, which have been added
What this PR does / why we need it:
Part of install_upgrade_operators maintenance work
Which issue(s) this PR fixes:
Future unnecessary failures in iuo lanes
Special notes for reviewer:
jira-ticket:
https://issues.redhat.com/browse/CNV-76662
Summary by CodeRabbit
Tests
New Features
Refactor
Chores