address review comments

updates! 50ec24b

updates! f3a6c3e

updates! 792156f

updates! 399aa10

updates! 5080e3b

updates! c34f4e7

Author: dbasunag

conftest.py

@@ -226,9 +226,11 @@ def _add_upgrade_test(_item: Item, _upgrade_deployment_modes: list[str]) -> bool
 def pytest_sessionstart(session: Session) -> None:
     log_file = session.config.getoption("log_file") or "pytest-tests.log"
     tests_log_file = os.path.join(get_base_dir(), log_file)
+    LOGGER.info(f"Writing tests log to {tests_log_file}")
     if os.path.exists(tests_log_file):
         pathlib.Path(tests_log_file).unlink()
-
+    if session.config.getoption("--collect-must-gather"):
+        session.config.option.must_gather_db = Database()
     session.config.option.log_listener = setup_logging(
         log_file=tests_log_file,
         log_level=session.config.getoption("log_cli_level") or logging.INFO,
@@ -261,10 +263,10 @@ def pytest_runtest_setup(item: Item) -> None:
         # start time
 
         try:
-            db = Database()
+            db = item.config.option.must_gather_db
             db.insert_test_start_time(
                 test_name=f"{item.fspath}::{item.name}",
-                start_time=int(datetime.datetime.now().strftime("%s")),
+                start_time=int(datetime.datetime.now().timestamp()),
             )
         except Exception as db_exception:
             LOGGER.error(f"Database error: {db_exception}. Must-gather collection may not be accurate")
@@ -320,10 +322,11 @@ def pytest_sessionfinish(session: Session, exitstatus: int) -> None:
     if session.config.option.setupplan or session.config.option.collectonly:
         return
     if session.config.getoption("--collect-must-gather"):
-        db = Database()
+        db = session.config.option.must_gather_db
         file_path = db.database_file_path
         LOGGER.info(f"Removing database file path {file_path}")
-        os.remove(file_path)
+        if os.path.exists(file_path):
+            os.remove(file_path)
         # clean up the empty folders
     collector_directory = py_config["must_gather_collector"]["must_gather_base_directory"]
     if os.path.exists(collector_directory):
@@ -357,7 +360,7 @@ def pytest_exception_interact(node: Item | Collector, call: CallInfo[Any], repor
         LOGGER.info(f"Must-gather collection is enabled for {test_name}.")
 
         try:
-            db = Database()
+            db = node.config.option.must_gather_db
             test_start_time = db.get_test_start_time(test_name=test_name)
         except Exception as db_exception:
             test_start_time = 0

utilities/constants.py

@@ -273,4 +273,3 @@ class RunTimeConfig:
 }
 
 RHOAI_OPERATOR_NAMESPACE = "redhat-ods-operator"
-RHOAI_SUBSCRIPTION_NAME = "rhoai-operator-dev"

utilities/database.py

@@ -1,4 +1,5 @@
 import logging
+import os
 
 from sqlalchemy import Integer, String, create_engine
 from sqlalchemy.orm import Mapped, Session, mapped_column
@@ -24,7 +25,7 @@ class OpenDataHubTestTable(Base):
 
 class Database:
     def __init__(self, database_file_name: str = TEST_DB, verbose: bool = True) -> None:
-        self.database_file_path = f"{get_base_dir()}{database_file_name}"
+        self.database_file_path = os.path.join(get_base_dir(), database_file_name)
         self.connection_string = f"sqlite:///{self.database_file_path}"
         self.verbose = verbose
         self.engine = create_engine(url=self.connection_string, echo=self.verbose)
@@ -38,9 +39,15 @@ def insert_test_start_time(self, test_name: str, start_time: int) -> None:
 
     def get_test_start_time(self, test_name: str) -> int:
         with Session(bind=self.engine) as db_session:
-            return (
+            result_row = (
                 db_session.query(OpenDataHubTestTable)
                 .with_entities(OpenDataHubTestTable.start_time)
                 .filter_by(test_name=test_name)
-                .one()[0]
+                .first()
             )
+            if result_row:
+                start_time_value = result_row[0]
+            else:
+                start_time_value = 0
+                LOGGER.warning(f"No test found with name: {test_name}")
+            return start_time_value

utilities/exceptions.py

@@ -96,7 +96,9 @@ def __str__(self) -> str:
         return f"Failed to log in as user {self.user}."
 
 
-class InvalidArguments(Exception):
+class InvalidArgumentsError(Exception):
+    """Raised when mutually exclusive or invalid argument combinations are passed."""
+
     pass
 
 

utilities/infra.py

@@ -46,11 +46,11 @@
 )
 from pyhelper_utils.shell import run_command
 from pytest_testconfig import config as py_config
-from packaging.version import parse, Version
+from semver import Version
 from simple_logger.logger import get_logger
 
 from ocp_resources.subscription import Subscription
-from utilities.constants import ApiGroups, Labels, Timeout, RHOAI_OPERATOR_NAMESPACE, RHOAI_SUBSCRIPTION_NAME
+from utilities.constants import ApiGroups, Labels, Timeout, RHOAI_OPERATOR_NAMESPACE
 from utilities.constants import KServeDeploymentType
 from utilities.constants import Annotations
 from utilities.exceptions import (
@@ -856,19 +856,34 @@ def wait_for_isvc_pods(client: DynamicClient, isvc: InferenceService, runtime_na
     return get_pods_by_isvc_label(client=client, isvc=isvc, runtime_name=runtime_name)
 
 
-def get_rhods_subscription() -> Subscription:
-    return Subscription(name=RHOAI_SUBSCRIPTION_NAME, namespace=RHOAI_OPERATOR_NAMESPACE, ensure_exists=True)
+def get_rhods_subscription() -> Subscription | None:
+    subscriptions = Subscription.get(dyn_client=get_client(), namespace=RHOAI_OPERATOR_NAMESPACE)
+    if subscriptions:
+        for subscription in subscriptions:
+            LOGGER.info(f"Checking subscription {subscription.name}")
+            if subscription.name.startswith(tuple(["rhods-operator", "rhoai-operator"])):
+                return subscription
 
+    LOGGER.warning("No RHOAI subscription found. Potentially ODH cluster")
+    return None
 
-def get_rhods_operator_installed_csv() -> ClusterServiceVersion:
-    subscription = get_rhods_subscription()
-    return ClusterServiceVersion(
-        name=subscription.instance.status.installedCSV, namespace=RHOAI_OPERATOR_NAMESPACE, ensure_exists=True
-    )
 
-
-def get_rhods_csv_version() -> Version:
-    return parse(version=get_rhods_operator_installed_csv().instance.spec.version)
+def get_rhods_operator_installed_csv() -> ClusterServiceVersion | None:
+    subscription = get_rhods_subscription()
+    if subscription:
+        csv_name = subscription.instance.status.installedCSV
+        LOGGER.info(f"Expected CSV: {csv_name}")
+        return ClusterServiceVersion(name=csv_name, namespace=RHOAI_OPERATOR_NAMESPACE, ensure_exists=True)
+    return None
+
+
+def get_rhods_csv_version() -> Version | None:
+    rhoai_csv = get_rhods_operator_installed_csv()
+    if rhoai_csv:
+        LOGGER.info(f"RHOAI CSV version: {rhoai_csv.instance.spec.version}")
+        return Version.parse(version=rhoai_csv.instance.spec.version)
+    LOGGER.warning("No RHOAI CSV found. Potentially ODH cluster")
+    return None
 
 
 @retry(

utilities/must_gather_collector.py

@@ -4,25 +4,27 @@
 from pytest_testconfig import config as py_config
 from pytest import Item
 from pyhelper_utils.shell import run_command
-
-from utilities.exceptions import InvalidArguments
+from simple_logger.logger import get_logger
+from utilities.exceptions import InvalidArgumentsError
 from utilities.infra import get_rhods_csv_version, get_oc_image_info, generate_openshift_pull_secret_file
 
 BASE_DIRECTORY_NAME = "must-gather-collected"
+BASE_RESULTS_DIR = "/home/odh/opendatahub-tests/"
+LOGGER = get_logger(name=__name__)
 
 
 def get_base_dir() -> str:
-    if os.path.exists("/home/odh/opendatahub-tests/"):
+    if os.path.exists(BASE_RESULTS_DIR):
         # we are running from jenkins.
-        return "/home/odh/opendatahub-tests/results"
+        return os.path.join(BASE_RESULTS_DIR, "results")
     else:
         # this is local run
         return ""
 
 
 def set_must_gather_collector_values() -> dict[str, str]:
     py_config["must_gather_collector"] = {
-        "must_gather_base_directory": f"{get_base_dir()}{BASE_DIRECTORY_NAME}",
+        "must_gather_base_directory": os.path.join(get_base_dir(), BASE_DIRECTORY_NAME),
     }
     return py_config["must_gather_collector"]
 
@@ -81,8 +83,22 @@ def run_must_gather(
     component_name: str = "",
     namespaces_dict: dict[str, str] | None = None,
 ) -> str:
+    """
+    Process the arguments to build must-gather command and run the same
+
+    Args:
+         image_url (str): must-gather image url
+         target_dir (str): must-gather target directory
+         since (str): duration in seconds for must-gather log collection
+         component_name (str): must-gather component name
+         namespaces_dict (dict[str, str] | None): namespaces dict for extra data collection from different component
+            namespaces
+
+    Returns:
+        str: must-gather output
+    """
     if component_name and namespaces_dict:
-        raise InvalidArguments("component name and namespaces can't be passed together")
+        raise InvalidArgumentsError("component name and namespaces can't be passed together")
 
     must_gather_command = "oc adm must-gather"
     if target_dir:
@@ -96,41 +112,56 @@ def run_must_gather(
     elif namespaces_dict:
         namespace_str = ""
         if namespaces_dict.get("operator"):
-            namespace_str += f"export OPERATOR_NAMESPACE={namespaces_dict['operator']};"
+            namespace_str += f"export OPERATOR_NAMESPACE={shlex.quote(namespaces_dict['operator'])};"
         if namespaces_dict.get("notebooks"):
-            namespace_str += f"export NOTEBOOKS_NAMESPACE={namespaces_dict['notebooks']};"
+            namespace_str += f"export NOTEBOOKS_NAMESPACE={shlex.quote(namespaces_dict['notebooks'])};"
         if namespaces_dict.get("monitoring"):
-            namespace_str += f"export MONITORING_NAMESPACE={namespaces_dict['monitoring']};"
+            namespace_str += f"export MONITORING_NAMESPACE={shlex.quote(namespaces_dict['monitoring'])};"
         if namespaces_dict.get("application"):
-            namespace_str += f"export APPLICATIONS_NAMESPACE={namespaces_dict['application']};"
+            namespace_str += f"export APPLICATIONS_NAMESPACE={shlex.quote(namespaces_dict['application'])};"
         if namespaces_dict.get("model_registries"):
-            namespace_str += f"export MODEL_REGISTRIES_NAMESPACE={namespaces_dict['model_registries']};"
+            namespace_str += f"export MODEL_REGISTRIES_NAMESPACE={shlex.quote(namespaces_dict['model_registries'])};"
         if namespaces_dict.get("ossm"):
-            namespace_str += f"export OSSM_NS={namespaces_dict['ossm']};"
+            namespace_str += f"export OSSM_NS={shlex.quote(namespaces_dict['ossm'])};"
         if namespaces_dict.get("knative"):
-            namespace_str += f"export KNATIVE_NS={namespaces_dict['knative']};"
+            namespace_str += f"export KNATIVE_NS={shlex.quote(namespaces_dict['knative'])};"
         if namespaces_dict.get("auth"):
-            namespace_str += f"export AUTH_NS={namespaces_dict['auth']};"
-        must_gather_command += " /usr/bin/gather"
+            namespace_str += f"export AUTH_NS={shlex.quote(namespaces_dict['auth'])};"
+        must_gather_command += f" -- '{namespace_str} /usr/bin/gather'"
 
     return run_command(command=shlex.split(must_gather_command), check=False)[1]
 
 
 def get_must_gather_image_info(architecture: str = "linux/amd64") -> str:
-    csv_version = get_rhods_csv_version()
-    must_gather_image_manifest = f"quay.io/modh/must-gather:rhoai-{csv_version.major}.{csv_version.minor}"
-    image_info = get_oc_image_info(
-        image=must_gather_image_manifest, architecture=architecture, pull_secret=generate_openshift_pull_secret_file()
-    )
-    return f"quay.io/modh/must-gather@{image_info['digest']}"
+    try:
+        csv_version = get_rhods_csv_version()
+        if csv_version:
+            must_gather_image_manifest = f"quay.io/modh/must-gather:rhoai-{csv_version.major}.{csv_version.minor}"
+            pull_secret = generate_openshift_pull_secret_file()
+            image_info = get_oc_image_info(
+                image=must_gather_image_manifest, architecture=architecture, pull_secret=pull_secret
+            )
+            return f"quay.io/modh/must-gather@{image_info['digest']}"
+        else:
+            LOGGER.warning(
+                "No RHAOI CSV found. Potentially ODH cluster and must-gather collection is not "
+                "relevant for this cluster"
+            )
+            return ""
+    except Exception as exec:
+        raise RuntimeError(f"Failed to retrieve must-gather image info: {str(exec)}") from exec
 
 
 def collect_rhoai_must_gather(
     target_dir: str, since: int, save_collection_output: bool = True, architecture: str = "linux/amd64"
 ) -> str:
     must_gather_image = get_must_gather_image_info(architecture=architecture)
-    output = run_must_gather(image_url=must_gather_image, target_dir=target_dir, since=f"{since}s")
-    if save_collection_output:
-        with open(os.path.join(target_dir, "output.log"), "w") as _file:
-            _file.write(output)
-    return get_must_gather_output_dir(must_gather_path=target_dir)
+    if must_gather_image:
+        output = run_must_gather(image_url=must_gather_image, target_dir=target_dir, since=f"{since}s")
+        if save_collection_output:
+            with open(os.path.join(target_dir, "output.log"), "w") as _file:
+                _file.write(output)
+        return get_must_gather_output_dir(must_gather_path=target_dir)
+    else:
+        LOGGER.warning("Must-gather collection would be skipped.")
+        return ""