CherryPicked: [cnv-4.18] Fix test vm free memory bytes tests #1142
Conversation
* Test kubevirt_vmi_migration_data_total_bytes Test for kubevirt_vmi_migration_data_total_bytes metric. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Changed test_kubevirt_vmi_migration_metrics last_overtime changed the last_overtime time to check the value start of the migration until the current time to avoid failures when there is one failure and it runs must gahter and then and the time passed make the test seem like it's failing but its not. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix vm free_memory_bytes metric based on working set The test for the metric kubevirt_vm_container_free_memory_bytes_based_on_working_set_bytes is flaky, in this PR I am fixing it by adding sampler to vm_memory_working_set_bytes in order to make sure that the value is found. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: vsibirsk <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update refactors and expands the test infrastructure for virtualization and storage, shifting from skip-based to fail/xfail-based test gating for missing cluster features. It introduces new fixtures, test markers, and configuration utilities, updates storage class matrix handling, and adds new or revised tests and helper functions for metrics, migration, and restricted namespace cloning. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
requested review from
EdDev,
sbahar619,
jerry7z,
vsibirsk,
Ahmad-Hafe,
dshchedr,
kgoldbla,
duyanyan,
kbidarkar and
akri3i
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
requested review from
hmeir,
geetikakay,
rnetser,
yossisegev and
RoniKishner
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:running
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}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (22)
utilities/network.py (1)
1119-1133: 🛠️ Refactor suggestion
Add docstring and fix code quality issues.
The function logic is correct, but there are several code quality improvements needed based on static analysis:
+def wait_for_node_marked_by_bridge( + bridge_nad: LinuxBridgeNetworkAttachmentDefinition, + node: Node +) -> None: + """ + Wait for a node to be marked by a bridge network attachment definition. + + Args: + bridge_nad: The bridge network attachment definition + node: The node to check for bridge marking + + Raises: + TimeoutExpiredError: If the node is not marked within the timeout period + """ -def wait_for_node_marked_by_bridge(bridge_nad: LinuxBridgeNetworkAttachmentDefinition, node: Node) -> None: bridge_annotation = bridge_nad.resource_name sampler = TimeoutSampler( wait_timeout=TIMEOUT_3MIN, sleep=5, func=lambda: bridge_annotation in node.instance.status.capacity.keys() and bridge_annotation in node.instance.status.allocatable.keys(), ) try: for sample in sampler: if sample: return except TimeoutExpiredError: - LOGGER.error(f"Node {node.hostname} is not marked by {bridge_nad.bridge_name} bridge") + LOGGER.error("Node %s is not marked by %s bridge", node.hostname, bridge_nad.bridge_name) raise📝 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.def wait_for_node_marked_by_bridge( bridge_nad: LinuxBridgeNetworkAttachmentDefinition, node: Node ) -> None: """ Wait for a node to be marked by a bridge network attachment definition. Args: bridge_nad: The bridge network attachment definition node: The node to check for bridge marking Raises: TimeoutExpiredError: If the node is not marked within the timeout period """ bridge_annotation = bridge_nad.resource_name sampler = TimeoutSampler( wait_timeout=TIMEOUT_3MIN, sleep=5, func=lambda: bridge_annotation in node.instance.status.capacity.keys() and bridge_annotation in node.instance.status.allocatable.keys(), ) try: for sample in sampler: if sample: return except TimeoutExpiredError: LOGGER.error("Node %s is not marked by %s bridge", node.hostname, bridge_nad.bridge_name) raise🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1119-1119: Line too long (107/100)
(C0301)
[convention] 1119-1119: Missing function or method docstring
(C0116)
[warning] 1132-1132: Use lazy % formatting in logging functions
(W1203)
🤖 Prompt for AI Agents
In utilities/network.py around lines 1119 to 1133, add a clear docstring to the function wait_for_node_marked_by_bridge explaining its purpose, parameters, and behavior. Also, improve code quality by replacing the for loop over sampler with a more idiomatic usage if possible, handle exceptions more precisely, and ensure consistent formatting and naming conventions according to project style guidelines.tests/virt/cluster/common_templates/utils.py (1)
36-39: 💡 Verification agent
🧩 Analysis chain
Function behavior changed from skip to expected failure.
The rename from
skip_on_guest_agent_versiontoxfail_old_guest_agent_versionreflects a significant behavioral change from skipping tests to marking them as expected failures. This aligns with the broader shift from skip-based to xfail-based test gating mentioned in the AI summary.Add a docstring to document the function's purpose and behavior:
def xfail_old_guest_agent_version(vm, ga_version): + """ + Mark test as expected failure if guest agent version is older than specified version. + + Args: + vm: Virtual machine instance with ssh_exec capability + ga_version: Minimum required guest agent version string + """ qemu_guest_agent_version = get_linux_guest_agent_version(ssh_exec=vm.ssh_exec)
🏁 Script executed:
#!/bin/bash # Verify all callers have been updated to use the new function name echo "Checking for any remaining references to the old function name:" rg "skip_on_guest_agent_version" --type py echo "Checking usage of the new function name:" rg "xfail_old_guest_agent_version" --type py -A 2 -B 2Length of output: 1720
xfail_old_guest_agent_version callers updated; add missing docstring
All references to the old
skip_on_guest_agent_versionhelper have been removed and replaced withxfail_old_guest_agent_versionin:
tests/virt/cluster/common_templates/utils.pytests/virt/cluster/common_templates/conftest.pyPlease add a docstring to explain the function’s purpose and parameters:
def xfail_old_guest_agent_version(vm, ga_version): + """ + Mark the test as an expected failure if the guest agent version is older than the specified minimum. + + Args: + vm: Virtual machine instance with an ssh_exec attribute for command execution. + ga_version: Minimum required guest agent version (string). + """ qemu_guest_agent_version = get_linux_guest_agent_version(ssh_exec=vm.ssh_exec) if version.parse(qemu_guest_agent_version.split()[0]) < version.parse(ga_version): pytest.xfail(reason=f"Bug in old guest agent version {qemu_guest_agent_version}")📝 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.def xfail_old_guest_agent_version(vm, ga_version): """ Mark the test as an expected failure if the guest agent version is older than the specified minimum. Args: vm: Virtual machine instance with an ssh_exec attribute for command execution. ga_version: Minimum required guest agent version (string). """ qemu_guest_agent_version = get_linux_guest_agent_version(ssh_exec=vm.ssh_exec) if version.parse(qemu_guest_agent_version.split()[0]) < version.parse(ga_version): pytest.xfail(reason=f"Bug in old guest agent version {qemu_guest_agent_version}")🧰 Tools
🪛 Pylint (3.3.7)
[convention] 36-36: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
In tests/virt/cluster/common_templates/utils.py around lines 36 to 39, the function xfail_old_guest_agent_version lacks a docstring. Add a clear docstring that explains the function's purpose, which is to mark tests as expected failures if the guest agent version is older than a specified version, and describe its parameters, including the vm object and the ga_version string.tests/virt/node/node_labeller/cpu_features/test_vm_with_cpu_features.py (1)
15-18:
⚠️ Potential issueFix inconsistency: fixture name suggests fail but implementation skips.
The fixture is renamed to
fail_if_amd_cpu_nodesbut still callspytest.skip(). This creates confusion and inconsistency with the naming convention used elsewhere in the codebase.@pytest.fixture() def fail_if_amd_cpu_nodes(nodes_cpu_vendor): + """Fail test if cluster has AMD CPU nodes (PCID feature unsupported).""" if nodes_cpu_vendor == AMD: - pytest.skip("PCID CPU feature is not supported on AMD CPU") + pytest.fail("PCID CPU feature is not supported on AMD CPU")📝 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.@pytest.fixture() def fail_if_amd_cpu_nodes(nodes_cpu_vendor): """Fail test if cluster has AMD CPU nodes (PCID feature unsupported).""" if nodes_cpu_vendor == AMD: - pytest.skip("PCID CPU feature is not supported on AMD CPU") + pytest.fail("PCID CPU feature is not supported on AMD CPU")🧰 Tools
🪛 Pylint (3.3.7)
[convention] 15-15: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
In tests/virt/node/node_labeller/cpu_features/test_vm_with_cpu_features.py around lines 15 to 18, the function named fail_if_amd_cpu_nodes calls pytest.skip(), which is inconsistent with its name suggesting a failure. To fix this, either rename the function to reflect that it skips the test when AMD CPU nodes are detected or change the implementation to raise an error or fail the test instead of skipping, aligning the behavior with the 'fail' naming convention.utilities/pytest_utils.py (1)
243-265: 🛠️ Refactor suggestion
Address code quality issues in the new function.
The function logic is correct, but there are several code quality improvements needed:
- Typo on line 247:
invald_scshould beinvalid_sc- Missing docstring: The function needs documentation explaining its purpose, parameters, and return value
- Line length: Line 250-253 exceeds 100 characters
- Dictionary iteration: Line 257 can iterate the dictionary directly
Apply this diff to address the issues:
+def update_storage_class_matrix_config(session, pytest_config_matrix): + """ + Update storage class matrix configuration based on command line options. + + Args: + session: pytest session object + pytest_config_matrix: existing storage class matrix from config + + Returns: + list: updated storage class matrix + + Raises: + ValueError: if incompatible storage classes are requested + AssertionError: if invalid storage classes are requested + """ cmdline_storage_class = session.config.getoption(name="storage_class_matrix") matrix_list = pytest_config_matrix matrix_names = [[*sc][0] for sc in pytest_config_matrix] - invald_sc = [] + invalid_sc = [] if cmdline_storage_class: cmdline_storage_class_matrix = cmdline_storage_class.split(",") - if HOSTPATH_CSI in cmdline_storage_class and StorageClassNames.TOPOLVM in cmdline_storage_class: + if (HOSTPATH_CSI in cmdline_storage_class and + StorageClassNames.TOPOLVM in cmdline_storage_class): raise ValueError( - f"{HOSTPATH_CSI} storage classes can't be used with {StorageClassNames.TOPOLVM} " - f": {cmdline_storage_class}" + f"{HOSTPATH_CSI} storage classes can't be used with " + f"{StorageClassNames.TOPOLVM}: {cmdline_storage_class}" ) for sc in cmdline_storage_class_matrix: if sc not in matrix_names: - if sc in HPP_STORAGE_CLASSES.keys(): + if sc in HPP_STORAGE_CLASSES: matrix_list.append({sc: HPP_STORAGE_CLASSES[sc]}) else: - invald_sc.append(sc) - assert not invald_sc, ( - f"Invalid sc requested via --storage-class-matix: {invald_sc}. Valid options: " + invalid_sc.append(sc) + assert not invalid_sc, ( + f"Invalid sc requested via --storage-class-matrix: {invalid_sc}. Valid options: " f"{matrix_names} and {[*HPP_STORAGE_CLASSES]}" ) return matrix_list📝 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.def update_storage_class_matrix_config(session, pytest_config_matrix): """ Update storage class matrix configuration based on command line options. Args: session: pytest session object pytest_config_matrix: existing storage class matrix from config Returns: list: updated storage class matrix Raises: ValueError: if incompatible storage classes are requested AssertionError: if invalid storage classes are requested """ cmdline_storage_class = session.config.getoption(name="storage_class_matrix") matrix_list = pytest_config_matrix matrix_names = [[*sc][0] for sc in pytest_config_matrix] invalid_sc = [] if cmdline_storage_class: cmdline_storage_class_matrix = cmdline_storage_class.split(",") if (HOSTPATH_CSI in cmdline_storage_class and StorageClassNames.TOPOLVM in cmdline_storage_class): raise ValueError( f"{HOSTPATH_CSI} storage classes can't be used with " f"{StorageClassNames.TOPOLVM}: {cmdline_storage_class}" ) for sc in cmdline_storage_class_matrix: if sc not in matrix_names: if sc in HPP_STORAGE_CLASSES: matrix_list.append({sc: HPP_STORAGE_CLASSES[sc]}) else: invalid_sc.append(sc) assert not invalid_sc, ( f"Invalid sc requested via --storage-class-matrix: {invalid_sc}. Valid options: " f"{matrix_names} and {[*HPP_STORAGE_CLASSES]}" ) return matrix_list🧰 Tools
🪛 Pylint (3.3.7)
[convention] 250-250: Line too long (104/100)
(C0301)
[convention] 243-243: Missing function or method docstring
(C0116)
[convention] 257-257: Consider iterating the dictionary directly instead of calling .keys()
(C0201)
🤖 Prompt for AI Agents
In utilities/pytest_utils.py from lines 243 to 265, fix the typo by renaming the variable 'invald_sc' to 'invalid_sc' throughout the function. Add a clear docstring at the start of the function describing its purpose, parameters, and return value. Break the long error message lines (250-253) into shorter lines to keep them under 100 characters. Finally, simplify the dictionary key check on line 257 by iterating directly over the dictionary keys instead of calling .keys().tests/global_config_sno_hpp.py (2)
23-33: 🛠️ Refactor suggestion
Simplify the dynamic configuration building logic.
The current logic has issues with undefined variable checking and could be cleaner.
Apply this diff to improve the logic:
for _dir in dir(): - if not config: # noqa: F821 - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str, int]: continue if _dir in ["encoding", "py_file"]: continue - config[_dir] = locals()[_dir] # noqa: F821 + config[_dir] = valCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 24-24: Using variable 'config' before assignment
(E0601)
🤖 Prompt for AI Agents
In tests/global_config_sno_hpp.py around lines 23 to 33, the current code attempts to build a config dictionary dynamically but has issues with checking if the config variable is defined and uses redundant lookups. Initialize the config dictionary explicitly before the loop. Then iterate over the keys in dir(), filter out unwanted types and keys, and assign the values directly to config without repeated locals() calls or undefined variable checks.
1-10: 🛠️ Refactor suggestion
Add module docstring and fix line length.
The module is missing a docstring and line 5 exceeds the character limit.
Apply this diff to address the issues:
+""" +Global configuration for Single Node OpenShift (SNO) with HostPath Provisioner (HPP). + +This module defines test matrices and configuration parameters specifically for +SNO environments using HPP storage classes. +""" from typing import Any import pytest_testconfig -from utilities.constants import ALL_CNV_DAEMONSETS, ALL_CNV_DEPLOYMENTS, ALL_CNV_PODS, HPP_VOLUME_MODE_ACCESS_MODE +from utilities.constants import ( + ALL_CNV_DAEMONSETS, + ALL_CNV_DEPLOYMENTS, + ALL_CNV_PODS, + HPP_VOLUME_MODE_ACCESS_MODE, +) from utilities.storage import HppCsiStorageClass📝 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.""" Global configuration for Single Node OpenShift (SNO) with HostPath Provisioner (HPP). This module defines test matrices and configuration parameters specifically for SNO environments using HPP storage classes. """ from typing import Any import pytest_testconfig from utilities.constants import ( ALL_CNV_DAEMONSETS, ALL_CNV_DEPLOYMENTS, ALL_CNV_PODS, HPP_VOLUME_MODE_ACCESS_MODE, ) from utilities.storage import HppCsiStorageClass global config global_config = pytest_testconfig.load_python( py_file="tests/global_config.py", encoding="utf-8" )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 5-5: Line too long (114/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'pytest_testconfig'
(E0401)
[warning] 8-8: Using the global statement at the module level
(W0604)
🤖 Prompt for AI Agents
In tests/global_config_sno_hpp.py at lines 1 to 10, add a module-level docstring at the top to describe the purpose of the module. Also, break line 5 into multiple lines or refactor it to ensure it does not exceed the maximum allowed line length, improving readability and compliance with style guidelines.tests/global_config_lvms.py (1)
37-47:
⚠️ Potential issueFix potential NameError in config building loop.
The variable
configis referenced before being defined, which will cause a NameError at runtime.+config: dict[str, Any] = {} + for _dir in dir(): - if not config: # noqa: F821 - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str, int]: continue if _dir in ["encoding", "py_file"]: continue - config[_dir] = locals()[_dir] # noqa: F821 + config[_dir] = locals()[_dir]🧰 Tools
🪛 Pylint (3.3.7)
[error] 38-38: Using variable 'config' before assignment
(E0601)
🤖 Prompt for AI Agents
In tests/global_config_lvms.py around lines 37 to 47, the variable config is used before it is defined, causing a potential NameError. Initialize config as an empty dictionary before the loop starts to ensure it is defined when referenced inside the loop.tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py (1)
157-157: 💡 Verification agent
🧩 Analysis chain
Verify removal of skip fixture is intentional.
The removal of the
skip_if_no_evmcs_supportfixture parameter means these tests will now run unconditionally. Ensure this aligns with the new centralized validation in thevirt_special_infra_sanityfixture mentioned in the AI summary.Also applies to: 190-190
🏁 Script executed:
#!/bin/bash # Description: Verify the virt_special_infra_sanity fixture exists and handles evmcs support validation # Expected: Find the new centralized fixture that replaces the removed skip logic rg -A 10 "virt_special_infra_sanity" tests/virt/conftest.pyLength of output: 816
🏁 Script executed:
#!/bin/bash # Examine virt_special_infra_sanity body for evmcs support check rg -n "def virt_special_infra_sanity" -A200 tests/virt/conftest.py # Search for any evmcs-related validation in the conftest rg -n "evmcs" -A5 tests/virt/conftest.py # Verify removal of skip_if_no_evmcs_support references across tests rg -n "skip_if_no_evmcs_support" -R testsLength of output: 7062
🏁 Script executed:
#!/bin/bash # Search for any remaining references to skip_if_no_evmcs_support across the repo rg -n skip_if_no_evmcs_support . # Show context around the test signature to confirm fixture removal sed -n '140,180p' tests/virt/node/hyperv_support/test_hyperv_features_in_vm.pyLength of output: 1412
🏁 Script executed:
#!/bin/bash # Check if virt_special_infra_sanity is applied to tests via usefixtures or autouse rg -n "usefixtures.*virt_special_infra_sanity" -R tests rg -n "autouse.*virt_special_infra_sanity" -R testsLength of output: 921
Ensure EVMCS tests trigger the centralized VMX support check
The
virt_special_infra_sanityfixture only invokes_verify_evmcs_supportfor tests marked withhigh_resource_vm. However,test_windows_vm_with_evmcs_featureno longer usesskip_if_no_evmcs_supportand isn’t annotated withhigh_resource_vm, so on clusters without VMX support it will now run (and likely fail) instead of being skipped.Please update the test to opt into the central check or restore explicit skipping:
• File: tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py
Line: ~157
Change: Add@pytest.mark.high_resource_vmabovedef test_windows_vm_with_evmcs_feature…Alternatively, if this test shouldn’t rely on the global fixture, re-add
skip_if_no_evmcs_supportto itshyperv_vmparametrization.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 157-157: Missing function or method docstring
(C0116)
[warning] 157-157: Redefining name 'hyperv_vm' from outer scope (line 21)
(W0621)
🤖 Prompt for AI Agents
In tests/virt/node/hyperv_support/test_hyperv_features_in_vm.py at line 157, the test_windows_vm_with_evmcs_feature no longer uses the skip_if_no_evmcs_support fixture and is not marked with @pytest.mark.high_resource_vm, causing it to run unconditionally and potentially fail on clusters without VMX support. To fix this, add the decorator @pytest.mark.high_resource_vm above the test_windows_vm_with_evmcs_feature definition to ensure it triggers the centralized VMX support check in the virt_special_infra_sanity fixture.tests/virt/conftest.py (5)
34-34: 🛠️ Refactor suggestion
Use generator expression instead of list comprehension
Using a list comprehension inside
any()creates an unnecessary intermediate list.-if not any([node.labels.cpumanager == "true" for node in _schedulable_nodes]): +if not any(node.labels.cpumanager == "true" for node in _schedulable_nodes):📝 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.if not any(node.labels.cpumanager == "true" for node in _schedulable_nodes):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 34-34: Use a generator instead 'any(node.labels.cpumanager == 'true' for node in _schedulable_nodes)'
(R1729)
🤖 Prompt for AI Agents
In tests/virt/conftest.py at line 34, replace the list comprehension inside the any() function with a generator expression by removing the square brackets. This avoids creating an unnecessary intermediate list and improves performance.
59-61: 🛠️ Refactor suggestion
Use generator expression for better performance
-if not any([ - label == "cpu-feature.node.kubevirt.io/vmx" and value == "true" for label, value in node.labels.items() -]): +if not any( + label == "cpu-feature.node.kubevirt.io/vmx" and value == "true" for label, value in node.labels.items() +):📝 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.if not any( label == "cpu-feature.node.kubevirt.io/vmx" and value == "true" for label, value in node.labels.items() ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 60-60: Line too long (119/100)
(C0301)
[refactor] 59-61: Use a generator instead 'any(label == 'cpu-feature.node.kubevirt.io/vmx' and value == 'true' for (label, value) in node.labels.items())'
(R1729)
🤖 Prompt for AI Agents
In tests/virt/conftest.py around lines 59 to 61, replace the list comprehension inside the any() function with a generator expression by removing the square brackets. This change improves performance by avoiding the creation of an intermediate list and instead generates items on the fly for the any() check.
66-69: 🛠️ Refactor suggestion
Use generator expression instead of list comprehension
-if not any([ - parse_string_unsafe(worker.instance.status.allocatable["hugepages-1Gi"]) >= parse_string_unsafe("1Gi") - for worker in _workers -]): +if not any( + parse_string_unsafe(worker.instance.status.allocatable["hugepages-1Gi"]) >= parse_string_unsafe("1Gi") + for worker in _workers +):📝 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.if not any( parse_string_unsafe(worker.instance.status.allocatable["hugepages-1Gi"]) >= parse_string_unsafe("1Gi") for worker in _workers ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 67-67: Line too long (114/100)
(C0301)
[refactor] 66-69: Use a generator instead 'any(parse_string_unsafe(worker.instance.status.allocatable['hugepages-1Gi']) >= parse_string_unsafe('1Gi') for worker in _workers)'
(R1729)
🤖 Prompt for AI Agents
In tests/virt/conftest.py around lines 66 to 69, replace the list comprehension inside the any() function with a generator expression by removing the square brackets. This change improves performance by avoiding the creation of an intermediate list.
46-49:
⚠️ Potential issueFix typos in function name and error message
The function name has a typo and the error message contains a spelling error.
-def _verfify_no_dpdk(): +def _verify_no_dpdk(): LOGGER.info("Verify cluster doesn't have DPDK enabled") if PerformanceProfile(name="dpdk").exists: - failed_verifications_list.append("Cluster has DPDK enabled (DPDK is incomatible with NVIDIA GPU)") + failed_verifications_list.append("Cluster has DPDK enabled (DPDK is incompatible with NVIDIA GPU)")📝 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.def _verify_no_dpdk(): LOGGER.info("Verify cluster doesn't have DPDK enabled") if PerformanceProfile(name="dpdk").exists: failed_verifications_list.append("Cluster has DPDK enabled (DPDK is incompatible with NVIDIA GPU)")🧰 Tools
🪛 Pylint (3.3.7)
[convention] 49-49: Line too long (110/100)
(C0301)
🤖 Prompt for AI Agents
In tests/virt/conftest.py around lines 46 to 49, correct the typo in the function name from _verfify_no_dpdk to _verify_no_dpdk and fix the spelling error in the error message by changing "incomatible" to "incompatible".
56-62: 🛠️ Refactor suggestion
Function name doesn't match its purpose
The function is named
_verify_evmcs_supportbut it's actually checking for VMX CPU feature support. Consider renaming for clarity.-def _verify_evmcs_support(_schedulable_nodes): - LOGGER.info("Verify cluster nodes support VMX cpu fixture") +def _verify_vmx_support(_schedulable_nodes): + LOGGER.info("Verify cluster nodes support VMX CPU feature")Also update the function call on line 79:
-_verify_evmcs_support(_schedulable_nodes=schedulable_nodes) +_verify_vmx_support(_schedulable_nodes=schedulable_nodes)📝 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.def _verify_vmx_support(_schedulable_nodes): LOGGER.info("Verify cluster nodes support VMX CPU feature") for node in _schedulable_nodes: if not any([ label == "cpu-feature.node.kubevirt.io/vmx" and value == "true" for label, value in node.labels.items() ]): failed_verifications_list.append("Cluster does not have any node that supports VMX cpu feature") # ... later in the file ... _verify_vmx_support(_schedulable_nodes=schedulable_nodes)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 60-60: Line too long (119/100)
(C0301)
[convention] 62-62: Line too long (112/100)
(C0301)
[refactor] 59-61: Use a generator instead 'any(label == 'cpu-feature.node.kubevirt.io/vmx' and value == 'true' for (label, value) in node.labels.items())'
(R1729)
🤖 Prompt for AI Agents
In tests/virt/conftest.py around lines 56 to 62, the function named _verify_evmcs_support is misleading because it checks for VMX CPU feature support, not EVMCS. Rename the function to _verify_vmx_support to accurately reflect its purpose. Also, update the corresponding function call on line 79 to use the new name _verify_vmx_support.tests/storage/utils.py (1)
250-256: 🛠️ Refactor suggestion
Add docstring to document the function's purpose
The function is missing documentation that explains what it verifies.
def verify_vm_disk_image_permission(vm: VirtualMachine) -> None: + """Verify that the VM's disk image has correct permissions and ownership. + + Args: + vm: The VirtualMachine instance to verify + + Raises: + AssertionError: If disk image is missing or has incorrect permissions/ownership + """ v_pod = vm.vmi.virt_launcher_pod📝 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.def verify_vm_disk_image_permission(vm: VirtualMachine) -> None: """Verify that the VM's disk image has correct permissions and ownership. Args: vm: The VirtualMachine instance to verify Raises: AssertionError: If the disk image is missing or has incorrect permissions/ownership """ v_pod = vm.vmi.virt_launcher_pod LOGGER.debug("Check image exist, permission and ownership") output = v_pod.execute(command=["ls", "-l", "/var/run/kubevirt-private/vmi-disks/dv-disk"]) assert "disk.img" in output assert "-rw-rw----." in output assert "qemu qemu" in output🧰 Tools
🪛 Pylint (3.3.7)
[convention] 250-250: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
In tests/storage/utils.py around lines 250 to 256, the function verify_vm_disk_image_permission lacks a docstring. Add a concise docstring at the beginning of the function that explains its purpose, specifically that it verifies the existence, permissions, and ownership of the VM disk image file.tests/network/conftest.py (1)
63-68: 🛠️ Refactor suggestion
Add docstrings to document fixture purpose
Both fixtures are missing documentation.
@pytest.fixture() def fail_if_not_ipv4_supported_cluster_from_mtx( request, ipv4_supported_cluster, ): + """Fail the test if IPv4 is required by the test matrix but not supported by the cluster.""" if ip_version_data_from_matrix(request=request) == IPV4_STR and not ipv4_supported_cluster: pytest.fail(reason="IPv4 is not supported in this cluster") @pytest.fixture() def fail_if_not_ipv6_supported_cluster_from_mtx( request, ipv6_supported_cluster, ): + """Fail the test if IPv6 is required by the test matrix but not supported by the cluster.""" if ip_version_data_from_matrix(request=request) == IPV6_STR and not ipv6_supported_cluster: pytest.fail(reason="IPv6 is not supported in this cluster")Also applies to: 72-77
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 63-63: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
In tests/network/conftest.py at lines 63 to 68 and 72 to 77, the fixture functions lack docstrings explaining their purpose. Add clear and concise docstrings to each fixture describing what they do, their parameters, and any important behavior or usage notes to improve code readability and maintainability.tests/observability/metrics/test_migration_metrics.py (3)
283-289: 🛠️ Refactor suggestion
Remove unused parameter.
The parameter
vm_migration_metrics_vmim_scope_classis not used in the test.def test_metric_kubevirt_vmi_migration_start_time_seconds( self, prometheus, vm_for_migration_metrics_test, - vm_migration_metrics_vmim_scope_class, vm_migration_state, ):📝 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.def test_metric_kubevirt_vmi_migration_start_time_seconds( self, prometheus, vm_for_migration_metrics_test, vm_migration_state, ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 283-283: Missing function or method docstring
(C0116)
[warning] 286-286: Redefining name 'vm_for_migration_metrics_test' from outer scope (line 110)
(W0621)
[warning] 287-287: Redefining name 'vm_migration_metrics_vmim_scope_class' from outer scope (line 135)
(W0621)
[warning] 287-287: Unused argument 'vm_migration_metrics_vmim_scope_class'
(W0613)
🤖 Prompt for AI Agents
In tests/observability/metrics/test_migration_metrics.py around lines 283 to 289, the parameter vm_migration_metrics_vmim_scope_class is declared in the test function but never used. Remove this unused parameter from the function signature to clean up the code and avoid confusion.
299-305: 🛠️ Refactor suggestion
Remove unused parameters.
The parameters
vm_migration_metrics_vmim_scope_classandmigration_succeeded_scope_classare not used in the test.def test_metric_kubevirt_vmi_migration_end_time_seconds( self, prometheus, vm_for_migration_metrics_test, - vm_migration_metrics_vmim_scope_class, - migration_succeeded_scope_class, vm_migration_state, ):📝 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.def test_metric_kubevirt_vmi_migration_end_time_seconds( self, prometheus, vm_for_migration_metrics_test, vm_migration_state, ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 299-299: Missing function or method docstring
(C0116)
[refactor] 299-299: Too many arguments (6/5)
(R0913)
[refactor] 299-299: Too many positional arguments (6/5)
(R0917)
[warning] 302-302: Redefining name 'vm_for_migration_metrics_test' from outer scope (line 110)
(W0621)
[warning] 303-303: Redefining name 'vm_migration_metrics_vmim_scope_class' from outer scope (line 135)
(W0621)
[warning] 304-304: Redefining name 'migration_succeeded_scope_class' from outer scope (line 175)
(W0621)
[warning] 303-303: Unused argument 'vm_migration_metrics_vmim_scope_class'
(W0613)
[warning] 304-304: Unused argument 'migration_succeeded_scope_class'
(W0613)
🤖 Prompt for AI Agents
In tests/observability/metrics/test_migration_metrics.py around lines 299 to 305, the parameters vm_migration_metrics_vmim_scope_class and migration_succeeded_scope_class are declared in the test function signature but never used. Remove these two parameters from the function definition to clean up the code and avoid confusion.
265-273: 🛠️ Refactor suggestion
Remove unused parameters from test method.
The parameters
namespace,admin_client,migration_policy_with_bandwidth_scope_class, andvm_migration_metrics_vmim_scope_classare not used in the test.def test_kubevirt_vmi_migration_metrics( self, prometheus, - namespace, - admin_client, - migration_policy_with_bandwidth_scope_class, vm_for_migration_metrics_test, - vm_migration_metrics_vmim_scope_class, query, ):📝 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.def test_kubevirt_vmi_migration_metrics( self, prometheus, vm_for_migration_metrics_test, query, ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 265-265: Missing function or method docstring
(C0116)
[refactor] 265-265: Too many arguments (8/5)
(R0913)
[refactor] 265-265: Too many positional arguments (8/5)
(R0917)
[warning] 271-271: Redefining name 'vm_for_migration_metrics_test' from outer scope (line 110)
(W0621)
[warning] 272-272: Redefining name 'vm_migration_metrics_vmim_scope_class' from outer scope (line 135)
(W0621)
[warning] 268-268: Unused argument 'namespace'
(W0613)
[warning] 269-269: Unused argument 'admin_client'
(W0613)
[warning] 270-270: Unused argument 'migration_policy_with_bandwidth_scope_class'
(W0613)
[warning] 272-272: Unused argument 'vm_migration_metrics_vmim_scope_class'
(W0613)
🤖 Prompt for AI Agents
In tests/observability/metrics/test_migration_metrics.py around lines 265 to 273, the test method test_kubevirt_vmi_migration_metrics includes unused parameters: namespace, admin_client, migration_policy_with_bandwidth_scope_class, and vm_migration_metrics_vmim_scope_class. Remove these unused parameters from the method signature to clean up the code and avoid confusion.tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py (4)
190-196: 🛠️ Refactor suggestion
Remove unused parameter.
The parameter
permissions_datavolume_destinationis not used in the test function.def test_user_permissions_only_for_dst_ns_negative( storage_class_name_scope_module, data_volume_multi_storage_scope_module, destination_namespace, unprivileged_client, - permissions_datavolume_destination, ):📝 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.def test_user_permissions_only_for_dst_ns_negative( storage_class_name_scope_module, data_volume_multi_storage_scope_module, destination_namespace, unprivileged_client, ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 190-190: Missing function or method docstring
(C0116)
[warning] 195-195: Unused argument 'permissions_datavolume_destination'
(W0613)
🤖 Prompt for AI Agents
In tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py around lines 190 to 196, the parameter permissions_datavolume_destination is declared but not used in the test_user_permissions_only_for_dst_ns_negative function. Remove this unused parameter from the function signature to clean up the code and avoid confusion.
128-137: 🛠️ Refactor suggestion
Remove unused parameters.
Several parameters are not used in the test function:
namespace,permissions_datavolume_source,permissions_datavolume_destination, anduser_has_get_permissions_in_source_namespace.def test_user_permissions_negative( storage_class_name_scope_module, - namespace, data_volume_multi_storage_scope_module, destination_namespace, unprivileged_client, - permissions_datavolume_source, - permissions_datavolume_destination, - user_has_get_permissions_in_source_namespace, ):📝 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.def test_user_permissions_negative( storage_class_name_scope_module, data_volume_multi_storage_scope_module, destination_namespace, unprivileged_client, ): ...🧰 Tools
🪛 Pylint (3.3.7)
[convention] 128-128: Missing function or method docstring
(C0116)
[refactor] 128-128: Too many arguments (8/5)
(R0913)
[refactor] 128-128: Too many positional arguments (8/5)
(R0917)
[warning] 130-130: Unused argument 'namespace'
(W0613)
[warning] 134-134: Unused argument 'permissions_datavolume_source'
(W0613)
[warning] 135-135: Unused argument 'permissions_datavolume_destination'
(W0613)
[warning] 136-136: Unused argument 'user_has_get_permissions_in_source_namespace'
(W0613)
🤖 Prompt for AI Agents
In tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py around lines 128 to 137, the test_user_permissions_negative function includes unused parameters: namespace, permissions_datavolume_source, permissions_datavolume_destination, and user_has_get_permissions_in_source_namespace. Remove these unused parameters from the function signature to clean up the code and avoid confusion.
86-93: 🛠️ Refactor suggestion
Remove unused parameter.
The parameter
permissions_pvc_destinationis not used in the test function.def test_user_permissions_positive( unprivileged_client, storage_class_matrix__module__, storage_class_name_scope_module, - permissions_pvc_destination, dv_destination_cloned_from_pvc, requested_verify_image_permissions, ):📝 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.def test_user_permissions_positive( unprivileged_client, storage_class_matrix__module__, storage_class_name_scope_module, dv_destination_cloned_from_pvc, requested_verify_image_permissions, ):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 86-86: Missing function or method docstring
(C0116)
[refactor] 86-86: Too many arguments (6/5)
(R0913)
[refactor] 86-86: Too many positional arguments (6/5)
(R0917)
[warning] 90-90: Unused argument 'permissions_pvc_destination'
(W0613)
🤖 Prompt for AI Agents
In tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py around lines 86 to 93, the parameter permissions_pvc_destination is declared but not used in the test_user_permissions_positive function. Remove permissions_pvc_destination from the function's parameter list to clean up the code and avoid confusion.
50-54: 🛠️ Refactor suggestion
Remove unused parameter.
The parameter
permissions_pvc_sourceis not used in the test function.def test_unprivileged_user_clone_dv_same_namespace_positive( - permissions_pvc_source, dv_cloned_by_unprivileged_user_in_the_same_namespace, ):📝 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.def test_unprivileged_user_clone_dv_same_namespace_positive( dv_cloned_by_unprivileged_user_in_the_same_namespace, ): dv_cloned_by_unprivileged_user_in_the_same_namespace.wait_for_dv_success()🧰 Tools
🪛 Pylint (3.3.7)
[convention] 50-50: Missing function or method docstring
(C0116)
[warning] 51-51: Unused argument 'permissions_pvc_source'
(W0613)
🤖 Prompt for AI Agents
In tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning.py around lines 50 to 54, the parameter permissions_pvc_source is declared but not used in the test_unprivileged_user_clone_dv_same_namespace_positive function. Remove the unused permissions_pvc_source parameter from the function signature to clean up the code.
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:passed
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-1142 published |
|
/lgtm |
|
/verified |
|
/retest build-container |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-1142. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:cnv-4.18 published |
Short description:
The test for the metric kubevirt_vm_container_free_memory_bytes_based_on_working_set_bytes is flaky, in this PR I am fixing it by adding sampler to vm_memory_working_set_bytes in order to make sure that the value is found.
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
https://issues.redhat.com/browse/CNV-62980
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores