address review comments

updates! 50ec24b

updates! f3a6c3e

updates! 792156f

Author: dbasunag

conftest.py

@@ -226,6 +226,7 @@ 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()
 
@@ -264,7 +265,7 @@ def pytest_runtest_setup(item: Item) -> None:
             db = Database()
             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")
@@ -323,7 +324,8 @@ def pytest_sessionfinish(session: Session, exitstatus: int) -> None:
         db = Database()
         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):

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/must_gather_collector.py

@@ -5,24 +5,25 @@
 from pytest import Item
 from pyhelper_utils.shell import run_command
 
-from utilities.exceptions import InvalidArguments
+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/"
 
 
 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 f"{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 +82,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,33 +111,37 @@ 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()
+        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']}"
+    except Exception as exec:
+        raise RuntimeError(f"Failed to retrieve must-gather image info: {str(exec)}") from exec
 
 
 def collect_rhoai_must_gather(