Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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