Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/install_upgrade_operators/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
)
from tests.install_upgrade_operators.utils import (
get_network_addon_config,
get_resource_by_name,
get_resource_from_module_name,
)
from tests.utils import get_resource_by_name
from utilities.constants import HOSTPATH_PROVISIONER_CSI, HPP_POOL
from utilities.hco import ResourceEditorValidateHCOReconcile, get_hco_version
from utilities.infra import (
Expand Down
2 changes: 1 addition & 1 deletion tests/install_upgrade_operators/crypto_policy/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
TLS_INTERMEDIATE_CIPHERS_IANA_OPENSSL_SYNTAX,
)
from tests.install_upgrade_operators.utils import (
get_resource_by_name,
get_resource_key_value,
)
from tests.utils import get_resource_by_name
from utilities.constants import (
CLUSTER,
TIMEOUT_2MIN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
RESOURCE_TYPE_STR,
)
from tests.install_upgrade_operators.utils import (
get_resource_by_name,
get_resource_key_value,
)
from tests.utils import get_resource_by_name
from utilities.constants import CDI_KUBEVIRT_HYPERCONVERGED, KUBEVIRT_HCO_NAME

pytestmark = [pytest.mark.post_upgrade, pytest.mark.sno, pytest.mark.s390x, pytest.mark.skip_must_gather_collection]
Expand Down
13 changes: 0 additions & 13 deletions tests/install_upgrade_operators/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,6 @@ def get_resource_from_module_name(related_obj, ocp_resources_submodule_list, adm
)


def get_resource_by_name(
resource_kind: Resource, name: str, admin_client: DynamicClient, namespace: str | None = None
) -> Resource:
kwargs = {"name": name}
if namespace:
kwargs["namespace"] = namespace
kwargs["client"] = admin_client
resource = resource_kind(**kwargs)
if resource.exists:
return resource
raise ResourceNotFoundError(f"{resource_kind} {name} not found.")


def get_resource_key_value(resource: Resource, key_name: str) -> Any:
return benedict(
resource.instance.to_dict()["spec"],
Expand Down
15 changes: 14 additions & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from ocp_resources.datavolume import DataVolume
from ocp_resources.kubevirt import KubeVirt
from ocp_resources.node import Node
from ocp_resources.resource import ResourceEditor
from ocp_resources.resource import Resource, ResourceEditor
from ocp_resources.storage_profile import StorageProfile
from ocp_resources.virtual_machine import VirtualMachine
from ocp_resources.virtual_machine_instance_migration import VirtualMachineInstanceMigration
Expand Down Expand Up @@ -689,3 +689,16 @@ def verify_rwx_default_storage(client: DynamicClient) -> None:
f"Default storage class '{storage_class}' doesn't support RWX mode "
f"(required: RWX, found: {found_mode or 'none'})"
)


def get_resource_by_name(
resource_kind: Resource, name: str, admin_client: DynamicClient, namespace: str | None = None
) -> Resource:
kwargs = {"name": name}
if namespace:
kwargs["namespace"] = namespace
kwargs["client"] = admin_client
resource = resource_kind(**kwargs)
Comment on lines +695 to +701
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the helper signature and constructor-style usage across changed files.
rg -n "def get_resource_by_name|resource_kind\s*:" tests/utils.py tests/install_upgrade_operators/crypto_policy/utils.py
rg -n "get_resource_by_name\(" tests/utils.py tests/install_upgrade_operators

Repository: RedHatQE/openshift-virtualization-tests

Length of output: 675


🏁 Script executed:

#!/bin/bash
# Read the full function implementation including docstring
sed -n '694,710p' tests/utils.py

Repository: RedHatQE/openshift-virtualization-tests

Length of output: 500


🏁 Script executed:

#!/bin/bash
# Check one actual call site to see what's being passed as resource_kind
sed -n '55,60p' tests/install_upgrade_operators/crypto_policy/utils.py

Repository: RedHatQE/openshift-virtualization-tests

Length of output: 294


🏁 Script executed:

#!/bin/bash
# Check what type Resource is and if it's callable
rg -A5 "^class Resource" tests/utils.py | head -15

Repository: RedHatQE/openshift-virtualization-tests

Length of output: 65


Fix type annotation and add required docstring for new helper.

The resource_kind parameter is called as a constructor at line 701 (resource_kind(**kwargs)), so it must be typed as type[Resource], not Resource. Current annotation breaks strict type checking and masks call-site errors.

Additionally, this public helper performs cluster API interactions and raises exceptions—it requires a Google-format docstring explaining the behavior and exception handling per coding guidelines.

Proposed fixes
 def get_resource_by_name(
-    resource_kind: Resource, name: str, admin_client: DynamicClient, namespace: str | None = None
+    resource_kind: type[Resource], name: str, admin_client: DynamicClient, namespace: str | None = None
 ) -> Resource:
+    """Get a resource by name from the cluster.
+
+    Args:
+        resource_kind: The resource class to instantiate.
+        name: Resource name.
+        admin_client: Authenticated cluster client.
+        namespace: Optional namespace; if omitted, cluster-scoped resource is used.
+
+    Returns:
+        Instantiated and verified resource.
+
+    Raises:
+        ResourceNotFoundError: If resource does not exist on cluster.
+    """
     kwargs = {"name": name}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/utils.py` around lines 695 - 701, The parameter annotation for
resource_kind is incorrect: change its type from Resource to type[Resource] so
the helper can call it as a constructor (resource = resource_kind(**kwargs));
then add a Google-style docstring to the helper function that describes its
purpose (creating/returning a Resource via cluster API), lists parameters,
return type, and documents exceptions the function may raise (propagate
client/API exceptions when cluster interactions fail) per coding guidelines.
Ensure the docstring is public, placed immediately under the function signature,
and mentions that callers should handle or expect client/API exceptions.

if resource.exists:
return resource
raise ResourceNotFoundError(f"{resource_kind} {name} not found.")
Comment on lines +694 to +704
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

MEDIUM: Add a Google-style docstring for this new shared helper.

This is a public helper with non-obvious side effects (cluster lookup + ResourceNotFoundError path); document args/returns/raises explicitly.

Proposed fix
 def get_resource_by_name(
     resource_kind: type[Resource], name: str, admin_client: DynamicClient, namespace: str | None = None
 ) -> Resource:
+    """Return an existing cluster resource by kind and name.
+
+    Args:
+        resource_kind (type[Resource]): Resource class to instantiate.
+        name (str): Resource name.
+        admin_client (DynamicClient): Cluster client.
+        namespace (str | None): Namespace for namespaced resources.
+
+    Returns:
+        Resource: The existing resource object.
+
+    Raises:
+        ResourceNotFoundError: If the resource does not exist.
+    """
     kwargs = {"name": name}

As per coding guidelines "Google-format docstrings REQUIRED for all public functions with non-obvious return values OR side effects".

🧰 Tools
🪛 Ruff (0.15.9)

[warning] 704-704: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/utils.py` around lines 694 - 704, Add a Google-style docstring to the
public helper function get_resource_by_name that documents its behavior,
parameters, return value, and exceptions: describe that it performs a cluster
lookup for a resource of type Resource using admin_client (DynamicClient), list
args (resource_kind: Resource class, name: str, admin_client: DynamicClient,
namespace: str | None), state the return type (Resource instance) and side
effect (may perform API call to the cluster), and explicitly document Raises
ResourceNotFoundError when the named resource does not exist; also mention that
kwargs['client'] is set to admin_client and that namespace is optional.

29 changes: 3 additions & 26 deletions tests/virt/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from ocp_resources.deployment import Deployment
from ocp_resources.infrastructure import Infrastructure
from ocp_resources.performance_profile import PerformanceProfile
from timeout_sampler import TimeoutExpiredError, TimeoutSampler

from tests.utils import (
verify_cpumanager_workers,
Expand All @@ -26,6 +25,7 @@
)
from tests.virt.node.gpu.utils import (
apply_node_labels,
assert_mdev_bus_exists_on_nodes,
toggle_vgpu_deploy_labels,
wait_for_ds_ready,
)
Expand All @@ -41,12 +41,10 @@
from utilities.constants import (
AMD,
INTEL,
TIMEOUT_1MIN,
TIMEOUT_5SEC,
NamespacesNames,
)
from utilities.exceptions import ResourceValueError, UnsupportedGPUDeviceError
from utilities.infra import ExecCommandOnPod, get_nodes_with_label, get_resources_by_name_prefix, label_nodes
from utilities.infra import get_nodes_with_label, get_resources_by_name_prefix, label_nodes
from utilities.pytest_utils import exit_pytest_execution
from utilities.virt import get_nodes_gpu_info, vm_instance_from_template

Expand Down Expand Up @@ -306,28 +304,7 @@ def non_existent_mdev_bus_nodes(workers_utility_pods, vgpu_ready_nodes):
If it's not available, this means the nvidia-vgpu-manager-daemonset
Pod might not be in running state in the nvidia-gpu-operator namespace.
"""
desired_bus = "mdev_bus"
non_existent_mdev_bus_nodes = []
for node in vgpu_ready_nodes:
pod_exec = ExecCommandOnPod(utility_pods=workers_utility_pods, node=node)
try:
for sample in TimeoutSampler(
wait_timeout=TIMEOUT_1MIN,
sleep=TIMEOUT_5SEC,
func=pod_exec.exec,
command=f"ls /sys/class | grep {desired_bus} || true",
):
if sample:
return
except TimeoutExpiredError:
non_existent_mdev_bus_nodes.append(node.name)
if non_existent_mdev_bus_nodes:
pytest.fail(
reason=(
f"On these nodes: {non_existent_mdev_bus_nodes} {desired_bus} is not available."
"Ensure that in 'nvidia-gpu-operator' namespace nvidia-vgpu-manager-daemonset Pod is Running."
)
)
assert_mdev_bus_exists_on_nodes(workers_utility_pods=workers_utility_pods, nodes=vgpu_ready_nodes)


@pytest.fixture(scope="session")
Expand Down
14 changes: 14 additions & 0 deletions tests/virt/node/gpu/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,18 @@
MDEV_GRID_AVAILABLE_INSTANCES_STR: "4",
MDEV_GRID_TYPE_STR: "nvidia-746",
},
"10de:20b7": {
DEVICE_ID_STR: "10de:20b7",
GPU_DEVICE_NAME_STR: f"{GPU_DEVICE_MANUFACTURER}/NVIDIA_A30-1-6C",
VGPU_DEVICE_NAME_STR: f"{GPU_DEVICE_MANUFACTURER}/NVIDIA_A30-1-6C",
GPU_PRETTY_NAME_STR: "NVIDIA A30-1-6C",
VGPU_PRETTY_NAME_STR: "NVIDIA A30-1-6C",
MDEV_NAME_STR: "NVIDIA A30-1-6C",
MDEV_AVAILABLE_INSTANCES_STR: "4",
MDEV_TYPE_STR: "nvidia-689",
VGPU_GRID_NAME_STR: f"{GPU_DEVICE_MANUFACTURER}/A30_1_6C",
MDEV_GRID_NAME_STR: "NVIDIA A30-1-6C",
MDEV_GRID_AVAILABLE_INSTANCES_STR: "4",
MDEV_GRID_TYPE_STR: "nvidia-689",
},
}
39 changes: 38 additions & 1 deletion tests/virt/node/gpu/utils.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import logging
import shlex

import pytest
from ocp_resources.resource import ResourceEditor
from pyhelper_utils.shell import run_ssh_commands
from timeout_sampler import TimeoutSampler
from timeout_sampler import TimeoutExpiredError, TimeoutSampler

from tests.virt.node.gpu.constants import (
SANDBOX_VALIDATOR_DEPLOY_LABEL,
Expand All @@ -13,10 +14,12 @@
from tests.virt.utils import fetch_gpu_device_name_from_vm_instance, verify_gpu_device_exists_in_vm
from utilities.constants import (
TCP_TIMEOUT_30SEC,
TIMEOUT_1MIN,
TIMEOUT_2MIN,
TIMEOUT_3MIN,
TIMEOUT_5SEC,
)
from utilities.infra import ExecCommandOnPod
from utilities.virt import restart_vm_wait_for_running_vm, running_vm

LOGGER = logging.getLogger(__name__)
Expand Down Expand Up @@ -111,3 +114,37 @@ def toggle_vgpu_deploy_labels(gpu_nodes, nodes_with_supported_gpus, sandbox_vali
apply_node_labels(nodes=nodes_with_supported_gpus, labels={VGPU_DEVICE_MANAGER_DEPLOY_LABEL: "true"})
wait_for_ds_ready(ds=sandbox_validator_ds, expected=len(gpu_nodes))
wait_for_ds_ready(ds=vgpu_device_manager_ds, expected=len(nodes_with_supported_gpus))


def assert_mdev_bus_exists_on_nodes(workers_utility_pods, nodes):
"""Assert that mdev_bus is present on every node in nodes.

Args:
workers_utility_pods: Utility pods used to run commands on nodes.
nodes: GPU nodes to check.

Raises:
pytest.fail: If mdev_bus is absent on any node after TIMEOUT_1MIN.
"""
desired_bus = "mdev_bus"
missing_nodes = []
for node in nodes:
pod_exec = ExecCommandOnPod(utility_pods=workers_utility_pods, node=node)
try:
for sample in TimeoutSampler(
wait_timeout=TIMEOUT_1MIN,
sleep=TIMEOUT_5SEC,
func=pod_exec.exec,
command=f"ls /sys/class | grep {desired_bus} || true",
):
if sample:
break
except TimeoutExpiredError:
missing_nodes.append(node.name)
if missing_nodes:
pytest.fail(
reason=(
f"On these nodes: {missing_nodes} {desired_bus} is not available."
"Ensure that in 'nvidia-gpu-operator' namespace nvidia-vgpu-manager-daemonset Pod is Running."
)
)
Loading