[virt] tests for mass machine type transition #1210
Conversation
|
""" WalkthroughA new test module for KubeVirt virtual machine lifecycle automation has been introduced. It adds multiple pytest fixtures and parameterized test cases to validate machine type transitions, scheduling behavior, and lifecycle automation jobs for VMs in a Kubernetes cluster. The module covers resource setup, job execution, and VM state validation. Additionally, a new class defining machine type name constants was added, an old constants file was deleted, and import paths for this class were corrected in existing test files. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/verified |
![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
🧹 Nitpick comments (5)
tests/virt/cluster/general/test_mass_machine_type_transition.py (5)
25-34: Reduce fixture signature complexity
run_kubevirt_api_lifecycle_automation_jobtakes eight positional arguments, breaching Pylint’s R0913/R0917 limits and making the fixture hard to read/maintain.
Consider wrapping the optional values (name,service_account,delete_after) into a small dataclass or a simple**kwargsdict so the signature only exposes the injected fixtures.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 25-25: Too many arguments (8/5)
(R0913)
[refactor] 25-25: Too many positional arguments (8/5)
(R0917)
41-44: NormaliseRESTART_REQUIREDto a predictable value
restart_requiredarrives from the parametrisation as free-form text.
A mix of"TRUE","True","true"(or even bools) would silently break the equality check at line 191.- {"name": "RESTART_REQUIRED", "value": params.get("restart_required")}, + {"name": "RESTART_REQUIRED", "value": str(params.get("restart_required")).lower()},and compare with
"true"/"false"everywhere.
80-84: Use the resolved namespace instead of the constant
add_scc_to_service_account()should use the actual namespace name coming from the fixture for clarity and to avoid accidental drift if the constant ever changes.- add_scc_to_service_account( - namespace=KUBEVIRT_API_LIFECYCLE_AUTOMATION, + add_scc_to_service_account( + namespace=kubevirt_api_lifecycle_namespace.name,
200-202: Fix typo in assertion message
smae→same.- f"VM {vm_with_schedulable_machine_type.name} should have smae machine type as before transition" + f"VM {vm_with_schedulable_machine_type.name} should have same machine type as before transition"
13-13: Constant should be upper-case
pc_q35_rhel8_4represents a constant value and should follow all-caps naming (PC_Q35_RHEL8_4) to stay in line with PEP-8.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/virt/cluster/general/test_mass_machine_type_transition.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/virt/cluster/general/test_mass_machine_type_transition.py
[refactor] 25-25: Too many arguments (8/5)
(R0913)
[refactor] 25-25: Too many positional arguments (8/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: build-container
🔇 Additional comments (1)
tests/virt/cluster/general/test_mass_machine_type_transition.py (1)
151-153: Guard against missing VMI when VM is unschedulableIf the VM never spawns a VMI,
vm_with_unschedulable_machine_type.instancemay beNone, raisingAttributeError.
Consider:vmi = vm_with_unschedulable_machine_type.instance assert vmi is not None and vmi.status.get("printableStatus") == "ErrorUnschedulable", ...
tests/virt/cluster/general/test_mass_machine_type_transition.py
Outdated
Show resolved
Hide resolved
tests/virt/cluster/general/test_mass_machine_type_transition.py
Outdated
Show resolved
Hide resolved
akri3i
dismissed stale reviews from vsibirsk and openshift-virtualization-qe-bot
via
3eda52f
akri3i
force-pushed
the
test_machine_type_transition
branch
from
5d3b661 to
3eda52f
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
| @@ -0,0 +1,260 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
please follow the readme and split into different files, contest, test file and if needed utils.
should be placed under a dir with the feature name
| yield job | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
why is this scoped session?
| yield from create_ns(name=KUBEVIRT_API_LIFECYCLE_AUTOMATION, admin_client=admin_client) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
|
|
||
| @pytest.fixture(scope="session") | ||
| def kubevirt_api_lifecycle_service_account(kubevirt_api_lifecycle_namespace): | ||
| with ServiceAccount(name=KUBEVIRT_API_LIFECYCLE_AUTOMATION, namespace=kubevirt_api_lifecycle_namespace.name) as sa: |
There was a problem hiding this comment.
please add client in all resources
| if not any(label.startswith("machine-type.node.kubevirt") for label in node.labels.keys()) | ||
| ] | ||
| assert not nodes_without_machine_type_label, ( | ||
| f"Node {nodes_without_machine_type_label} does not have 'machine-type' label" |
There was a problem hiding this comment.
| f"Node {nodes_without_machine_type_label} does not have 'machine-type' label" | |
| f"Nodes {nodes_without_machine_type_label} do not have 'machine-type' label" |
|
|
||
| @pytest.mark.polarion("CNV-12003") | ||
| def test_vm_with_unschedulable_machine_type_fails_to_schedule(vm_with_unschedulable_machine_type): | ||
| vm_with_unschedulable_machine_type.wait_for_specific_status(status=VirtualMachine.Status.ERROR_UNSCHEDULABLE) |
There was a problem hiding this comment.
ERROR_UNSCHEDULABLE is a generic error, i recommend checking the actual reason to make sure it is the expected one (and not, for example, failed cluster)
| class TestMachineTypeTransition: | ||
| @pytest.mark.dependency(name=f"{TESTS_CLASS_NAME}::vm_running_with_schedulable_machine_type") | ||
| @pytest.mark.polarion("CNV-11989") | ||
| def test_vm_running_with_schedulable_machine_type( |
There was a problem hiding this comment.
based on the class name, this should be in setup, no?
|
|
||
| KUBEVIRT_API_LIFECYCLE_AUTOMATION = "kubevirt-api-lifecycle-automation" | ||
| TESTS_CLASS_NAME = "TestMachineTypeTransition" | ||
| ERROR_MESSAGE = "VM {} should have machine type {}, current machine type: {}" |
There was a problem hiding this comment.
please add arg names so it is clear what is expected, e.g "VM {vm_name} ..."
Short description:
tests for mass machine type transition
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