Merge branch 'opendatahub-io:main' into main

Author: dbasunag

.flake8

@@ -19,6 +19,7 @@ fcn_exclude_functions =
     re,
     logging,
     LOGGER,
+    BASIC_LOGGER,
     os,
     json,
     pytest,

.github/workflows/scripts/constants.py

@@ -27,6 +27,7 @@
  * Run [pre-commit](https://pre-commit.ci/)
  * Run [tox](https://tox.wiki/)
  * Add PR author as the PR assignee
+ * Build image based on the PR
 
 Available user actions:
  * To mark a PR as `WIP`, add `/wip` in a comment. To remove it from the PR comment `/wip cancel` to the PR.
@@ -37,6 +38,8 @@
         `verified` label removed on each new commit push.
  * To Cherry-pick a merged PR `/cherry-pick <target_branch_name>` to the PR. If <target_branch_name> is valid,
          and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
+ * To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
+ pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
 
 <details>
 <summary>Supported labels</summary>

.github/workflows/verify_build_container.yml

@@ -9,8 +9,6 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v4
-
       - name: Checkout Pull Request Branch
         uses: actions/checkout@v4
         with:

.pre-commit-config.yaml

@@ -35,7 +35,7 @@ repos:
       - id: detect-secrets
 
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.11.9
+    rev: v0.11.11
     hooks:
       - id: ruff
       - id: ruff-format
@@ -49,7 +49,7 @@ repos:
   #     - id: renovate-config-validator
 
   - repo: https://github.com/gitleaks/gitleaks
-    rev: v8.25.1
+    rev: v8.26.0
     hooks:
       - id: gitleaks
 

Dockerfile

@@ -35,6 +35,12 @@ RUN curl -sSL "https://github.com/fullstorydev/grpcurl/releases/download/v1.9.2/
     && tar xvf /tmp/grpcurl_1.2.tar.gz --no-same-owner \
     && mv grpcurl /usr/bin/grpcurl
 
+# Install htpasswd
+RUN apt-get update && \
+    apt-get install -y apache2-utils && \
+    apt-get clean && \
+    rm -rf /var/lib/apt/lists/*
+
 RUN useradd -ms /bin/bash $USER
 USER $USER
 WORKDIR $HOME_DIR

conftest.py

@@ -2,14 +2,19 @@
 import os
 import pathlib
 import shutil
+import datetime
+import traceback
 
 import shortuuid
+from _pytest.runner import CallInfo
+from _pytest.reports import TestReport
 from pytest import (
     Parser,
     Session,
     FixtureRequest,
     FixtureDef,
     Item,
+    Collector,
     Config,
     CollectReport,
 )
@@ -18,8 +23,15 @@
 from pytest_testconfig import config as py_config
 
 from utilities.constants import KServeDeploymentType
+from utilities.database import Database
 from utilities.logger import separator, setup_logging
-
+from utilities.must_gather_collector import (
+    set_must_gather_collector_directory,
+    set_must_gather_collector_values,
+    get_must_gather_collector_dir,
+    collect_rhoai_must_gather,
+    get_base_dir,
+)
 
 LOGGER = logging.getLogger(name=__name__)
 BASIC_LOGGER = logging.getLogger(name="basic")
@@ -30,7 +42,7 @@ def pytest_addoption(parser: Parser) -> None:
     buckets_group = parser.getgroup(name="Buckets")
     runtime_group = parser.getgroup(name="Runtime details")
     upgrade_group = parser.getgroup(name="Upgrade options")
-    platform_group = parser.getgroup(name="Platform")
+    must_gather_group = parser.getgroup(name="MustGather")
     cluster_sanity_group = parser.getgroup(name="ClusterSanity")
 
     # AWS config and credentials options
@@ -112,11 +124,11 @@ def pytest_addoption(parser: Parser) -> None:
         help="Coma-separated str; specify inference service deployment modes tests to run in upgrade tests. "
         "If not set, all will be tested.",
     )
-
-    # Platform options
-    platform_group.addoption(
-        "--applications-namespace",
-        help="RHOAI/ODH applications namespace",
+    must_gather_group.addoption(
+        "--collect-must-gather",
+        help="Indicate if must-gather should be collected on failure.",
+        action="store_true",
+        default=False,
     )
 
     # Cluster sanity options
@@ -205,14 +217,22 @@ def _add_upgrade_test(_item: Item, _upgrade_deployment_modes: list[str]) -> bool
 
 
 def pytest_sessionstart(session: Session) -> None:
-    tests_log_file = session.config.getoption("log_file") or "pytest-tests.log"
+    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,
     )
+    must_gather_dict = set_must_gather_collector_values()
+    shutil.rmtree(
+        path=must_gather_dict["must_gather_base_directory"],
+        ignore_errors=True,
+    )
 
 
 def pytest_fixture_setup(fixturedef: FixtureDef[Any], request: FixtureRequest) -> None:
@@ -226,9 +246,23 @@ def pytest_runtest_setup(item: Item) -> None:
     2. Adds `fail_if_missing_dependent_operators` fixture for Serverless tests.
     3. Adds fixtures to enable KServe/model mesh in DSC for model server tests.
     """
-
     BASIC_LOGGER.info(f"\n{separator(symbol_='-', val=item.name)}")
     BASIC_LOGGER.info(f"{separator(symbol_='-', val='SETUP')}")
+    if item.config.getoption("--collect-must-gather"):
+        # set must-gather collection directory:
+        set_must_gather_collector_directory(item=item, directory_path=get_must_gather_collector_dir())
+
+        # At the begining of setup work, insert current epoch time into the database to indicate test
+        # start time
+
+        try:
+            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().timestamp()),
+            )
+        except Exception as db_exception:
+            LOGGER.error(f"Database error: {db_exception}. Must-gather collection may not be accurate")
 
     if KServeDeploymentType.SERVERLESS.lower() in item.keywords:
         item.fixturenames.insert(0, "fail_if_missing_dependent_operators")
@@ -252,6 +286,10 @@ def pytest_runtest_call(item: Item) -> None:
 
 def pytest_runtest_teardown(item: Item) -> None:
     BASIC_LOGGER.info(f"{separator(symbol_='-', val='TEARDOWN')}")
+    # reset must-gather collector after each tests
+    py_config["must_gather_collector"]["collector_directory"] = py_config["must_gather_collector"][
+        "must_gather_base_directory"
+    ]
 
 
 def pytest_report_teststatus(report: CollectReport, config: Config) -> None:
@@ -276,10 +314,56 @@ def pytest_sessionfinish(session: Session, exitstatus: int) -> None:
     session.config.option.log_listener.stop()
     if session.config.option.setupplan or session.config.option.collectonly:
         return
-    base_dir = py_config["tmp_base_dir"]
-    LOGGER.info(f"Deleting pytest base dir {base_dir}")
-    shutil.rmtree(path=base_dir, ignore_errors=True)
+    if session.config.getoption("--collect-must-gather"):
+        db = session.config.option.must_gather_db
+        file_path = db.database_file_path
+        LOGGER.info(f"Removing database file path {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):
+        for root, dirs, files in os.walk(collector_directory, topdown=False):
+            for _dir in dirs:
+                dir_path = os.path.join(root, _dir)
+                if not os.listdir(dir_path):
+                    shutil.rmtree(path=dir_path, ignore_errors=True)
+    LOGGER.info(f"Deleting pytest base dir {session.config.option.basetemp}")
+    shutil.rmtree(path=session.config.option.basetemp, ignore_errors=True)
 
     reporter: Optional[TerminalReporter] = session.config.pluginmanager.get_plugin("terminalreporter")
     if reporter:
         reporter.summary_stats()
+
+
+def calculate_must_gather_timer(test_start_time: int) -> int:
+    default_duration = 300
+    if test_start_time > 0:
+        duration = int(datetime.datetime.now().timestamp()) - test_start_time
+        return duration if duration > 60 else default_duration
+    else:
+        LOGGER.warning(f"Could not get start time of test. Collecting must-gather for last {default_duration}s")
+        return default_duration
+
+
+def pytest_exception_interact(node: Item | Collector, call: CallInfo[Any], report: TestReport | CollectReport) -> None:
+    LOGGER.error(report.longreprtext)
+    if node.config.getoption("--collect-must-gather"):
+        test_name = f"{node.fspath}::{node.name}"
+        LOGGER.info(f"Must-gather collection is enabled for {test_name}.")
+
+        try:
+            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
+            LOGGER.warning(f"Error: {db_exception} in accessing database.")
+
+        try:
+            collect_rhoai_must_gather(
+                since=calculate_must_gather_timer(test_start_time=test_start_time),
+                target_dir=os.path.join(get_must_gather_collector_dir(), "pytest_exception_interact"),
+            )
+
+        except Exception as current_exception:
+            LOGGER.warning(f"Failed to collect logs: {test_name}: {current_exception} {traceback.format_exc()}")

docs/DEVELOPER_GUIDE.md

@@ -141,6 +141,7 @@ secret = Secret(name=request.param["name"], model_dir=request.param["model-dir"]
 Check [pytest.ini](../pytest.ini) for available markers; additional markers can always be added when needed.
 - Classes are good to group related tests together, for example when they share a fixture.  
 You should NOT group unrelated tests in one class (because it is misleading the reader).
+- All the tests should be properly documented. Every test (or test class), should have a docstring explaning what the test does so that anyone (engineers from other components, managers, PMs, or non-technical users) can have a basic understanding of what the code is trying to test without having to dive into the technical details of related functions or fixtures.
 
 
 ## Check the code

pyproject.toml

@@ -65,8 +65,10 @@ dependencies = [
     "jira>=3.8.0",
     "openshift-python-wrapper>=11.0.50",
     "semver>=3.0.4",
+    "sqlalchemy>=2.0.40",
     "pytest-order>=1.3.0",
     "marshmallow==3.26.1,<4", # this version is needed for pytest-jira
+    "pytest-html>=4.1.1",
 ]
 
 [project.urls]

tests/conftest.py

@@ -34,6 +34,7 @@
     get_operator_distribution,
     login_with_user_password,
     get_openshift_token,
+    get_data_science_cluster,
 )
 from utilities.constants import (
     AcceleratorType,
@@ -66,17 +67,19 @@ def tests_tmp_dir(request: FixtureRequest, tmp_path_factory: TempPathFactory) ->
 
 @pytest.fixture(scope="session")
 def updated_global_config(request: FixtureRequest, admin_client: DynamicClient) -> None:
-    if get_operator_distribution(client=admin_client) == "Open Data Hub":
+    distribution = get_operator_distribution(client=admin_client)
+    if distribution == "Open Data Hub":
         py_config["distribution"] = "upstream"
 
-    else:
+    elif distribution.startswith("OpenShift AI"):
         py_config["distribution"] = "downstream"
-
-    if applications_namespace := request.config.getoption("applications_namespace"):
-        py_config["applications_namespace"] = applications_namespace
-
     else:
-        py_config["applications_namespace"] = get_dsci_applications_namespace(client=admin_client)
+        pytest.exit(f"Unknown distribution: {distribution}")
+
+    py_config["applications_namespace"] = get_dsci_applications_namespace(client=admin_client)
+    py_config["model_registry_namespace"] = get_data_science_cluster(
+        client=admin_client
+    ).instance.spec.components.modelregistry.registriesNamespace
 
 
 @pytest.fixture(scope="session")

tests/global_config.py

@@ -1,12 +1,14 @@
 global config  # type:ignore[unused-ignore]
 
-distribution: str = "downstream"
-applications_namespace: str = "redhat-ods-applications"  # overwritten in conftest.py if distribution is upstream
 dsc_name: str = "default-dsc"
+must_gather_base_dir: str = "must-gather-base-dir"
 dsci_name: str = "default-dsci"
 dependent_operators: str = "servicemeshoperator,authorino-operator,serverless-operator"
-
 use_unprivileged_client: bool = True
+# overwrite the followings in conftest.py, in updated_global_config() if distribution is upstream
+distribution: str = "downstream"
+applications_namespace: str = "redhat-ods-applications"
+model_registry_namespace: str = "rhoai-model-registries"
 
 for _dir in dir():
     val = locals()[_dir]