add dedicated cpu instancetype test#3697
Open
RoniKishner wants to merge 1 commit intoRedHatQE:mainfrom
Open
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:
📝 WalkthroughWalkthroughAdds Windows-focused test fixtures (admission policies, registry-backed DataVolume/DataSource, Windows VM fixture), two dedicated-CPU Windows tests, a vCPU-pinning verification utility, and a constant for the Windows container-disk OS flavor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3697 +/- ##
==========================================
+ Coverage 98.38% 98.56% +0.17%
==========================================
Files 25 25
Lines 2166 2298 +132
==========================================
+ Hits 2131 2265 +134
+ Misses 35 33 -2
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-tc-requirement-polarion:running
openshift-virtualization-qe-bot
previously approved these changes
Feb 2, 2026
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/ssp.py (1)
133-149:⚠️ Potential issue | 🟡 MinorMEDIUM: Add Google‑style docstring + avoid list materialization in
any()This helper waits/retries and can raise; a docstring makes side effects and parameters explicit, and a generator avoids building an intermediate list (Ruff C419).
Proposed update
def wait_for_condition_message_value(resource, expected_message): + """Wait until a condition message matches the expected value. + + Args: + resource: Resource with instance.status.conditions + expected_message (str): Expected condition message value + """ LOGGER.info(f"Verify {resource.name} conditions contain expected message: {expected_message}") sample = None try: for sample in TimeoutSampler( wait_timeout=TIMEOUT_5MIN, sleep=TIMEOUT_5SEC, func=lambda: resource.instance.status.conditions, ): - if sample and any([condition["message"] == expected_message for condition in sample]): + if sample and any(condition["message"] == expected_message for condition in sample): returnAs per coding guidelines, Google-format docstrings REQUIRED for all public functions with non-obvious return values OR side effects.
🤖 Fix all issues with AI agents
In `@tests/infrastructure/instance_types/conftest.py`:
- Around line 53-106: Rename the verb-named fixtures
deployed_validating_admission_policy and
deployed_validating_admission_policy_binding to noun-based names (e.g.,
validating_admission_policy and validating_admission_policy_binding) and update
any `@pytest.mark.usefixtures` references in test_common_vm_instancetype.py to the
new names; while renaming, preserve existing parameters, scope="class" and
context-manager usage (ValidatingAdmissionPolicy /
ValidatingAdmissionPolicyBinding) so behavior doesn't change. Add concise
Google-style docstrings above each fixture describing what the fixture provides
and any side effects (e.g., "Creates and yields a ValidatingAdmissionPolicy that
enforces Windows VMI CPU placement" and similarly for the binding). Ensure the
fixture names are public (no leading underscores) and return/yield behavior is
documented.
- Around line 151-166: The fixture windows_vm_for_dedicated_cpu is currently
declared with scope="class" but provisions a mutable VM; change its fixture
scope to "function" to ensure per-test isolation by updating the `@pytest.fixture`
decorator (or removing the scope param to use the default function scope) for
the windows_vm_for_dedicated_cpu fixture; keep the body using
VirtualMachineForTests, VirtualMachineClusterInstancetype,
VirtualMachineClusterPreference, and data_volume_template_with_source_ref_dict
unchanged but verify no tests depend on class-scoped reuse.
- Around line 136-148: The fixture latest_windows_data_source currently accepts
an unused parameter namespace, triggering Ruff ARG001; fix it by either using
the provided namespace when constructing the DataSource (replace
latest_windows_data_volume.namespace with namespace in the DataSource(...) call)
or by removing namespace from the fixture signature and any references to it so
the fixture only uses latest_windows_data_volume, DataSource, and
generate_data_source_dict; choose the option that matches intended test
behavior.
- Around line 109-134: The fixture latest_windows_data_volume is declared with
class scope causing shared DataVolume resources; change the pytest fixture
decorator to use function scope so each test gets an isolated DataVolume
instance. Update the `@pytest.fixture`(scope="class") on
latest_windows_data_volume to `@pytest.fixture`(scope="function"), leaving the
DataVolume(...) context usage, secret/config map retrieval via
get_artifactory_secret and get_artifactory_config_map, and the cleanup call to
cleanup_artifactory_secret_and_config_map unchanged.
In `@tests/infrastructure/instance_types/test_common_vm_instancetype.py`:
- Around line 1-14: The file defines an unused LOGGER variable and imports
logging unnecessarily; remove the logging import and the LOGGER =
logging.getLogger(__name__) line to eliminate dead code (search for the symbol
LOGGER and the logging import at the top of
tests/infrastructure/instance_types/test_common_vm_instancetype.py and delete
both). If you prefer to keep logging, instead use LOGGER in test output or debug
statements within this module, but the simpler fix is to remove the unused
import and LOGGER definition.
In `@tests/infrastructure/instance_types/utils.py`:
- Around line 67-83: The verify_dedicated_cpu_placement function assumes
vm.privileged_vmi.xml_dict["domain"]["cputune"]["vcpupin"] is always a list;
normalize that value to a list (wrap a single dict into [dict]) before building
vcpupin_cpusets, add an assertion that "cputune" and "vcpupin" exist in
vm.privileged_vmi.xml_dict["domain"] to fail fast with a clear message, then
iterate the normalized list and perform the existing isdigit/“-”/“,” checks for
each vcpupin["@cpuset"] to avoid crashes when only one vcpupin is present.
tests/infrastructure/instance_types/test_common_vm_instancetype.py
Outdated
Show resolved
Hide resolved
RoniKishner
force-pushed
the
instancetype-d1
branch
from
77dab83 to
7b5fb47
Compare
RoniKishner
force-pushed
the
instancetype-d1
branch
from
7b93bec to
246931e
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
approved these changes
Feb 11, 2026
Contributor
Author
|
/verified |
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: 1849 🏁 Script executed: Length of output: 4676 🏁 Script executed: Length of output: 6294 🏁 Script executed: Length of output: 1761 🧠 Learnings used |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3592. Overlapping filesutilities/constants.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #3789. Overlapping filesutilities/constants.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
9 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:
add dedicated cpu instancetype test
More details:
Openshift ships with
d1series instance types, which have theDedicatedCPUPlacementfield set.Some instances of OpenShift deployment enforce the user to use
DedicatedCPUPlacementin combination of windows, the added tests aim to replica the validation which occurs on the cluster and verify it is kept across future versions.What this PR does / why we need it:
Add tests coverage for customer use case of d1 instance types
jira-ticket:
https://issues.redhat.com/browse/CNV-76513
Summary by CodeRabbit