Skip to content

Add test for Model Registry RBAC for SA token#296

Merged
dbasunag merged 5 commits intoopendatahub-io:mainfrom
lugi0:feature/rbac-sa
May 7, 2025
Merged

Add test for Model Registry RBAC for SA token#296
dbasunag merged 5 commits intoopendatahub-io:mainfrom
lugi0:feature/rbac-sa

Conversation

@lugi0
Copy link
Copy Markdown
Contributor

@lugi0 lugi0 commented May 6, 2025

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Added tests for Role-Based Access Control (RBAC) in the Model Registry using ServiceAccount tokens, including scenarios for both access denial and granted permissions.
    • Introduced test fixtures for automated setup and cleanup of Kubernetes namespaces, ServiceAccounts, roles, and role bindings to support RBAC testing.
    • Added utility functions for generating random names and namespace names for use in tests.
  • Bug Fixes

    • Removed an unused import from the test setup to improve code cleanliness.

@lugi0 lugi0 self-assigned this May 6, 2025
@lugi0 lugi0 requested a review from a team as a code owner May 6, 2025 16:43
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented May 6, 2025

"""

Walkthrough

The changes introduce comprehensive RBAC (Role-Based Access Control) testing for the Model Registry using Kubernetes ServiceAccounts. New fixtures and utility functions are added to manage test namespaces, ServiceAccounts, RBAC roles, and role bindings. Tests verify access denial and granting scenarios. An unused import is removed, and utility functions for naming are introduced.

Changes

File(s) Change Summary
tests/model_registry/conftest.py Removed an unused import statement (schemathesis.schemas).
tests/model_registry/rbac/test_mr_rbac_sa.py Added a new test module to validate RBAC for Model Registry REST endpoints using ServiceAccount tokens. Introduces helper functions and a parametrized test class with methods for access denied and granted scenarios.
tests/model_registry/rbac/conftest.py Added new pytest fixtures for creating and cleaning up Kubernetes namespaces, ServiceAccounts, roles, and role bindings. Includes token retrieval and error handling.
tests/model_registry/utils.py Added generate_random_name and generate_namespace_name utility functions for generating unique resource names and namespaces.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test Method
    participant Fixtures as Pytest Fixtures
    participant K8s as Kubernetes API
    participant Client as ModelRegistryClient

    Test->>Fixtures: Request test namespace, ServiceAccount, Role, RoleBinding
    Fixtures->>K8s: Create Namespace
    Fixtures->>K8s: Create ServiceAccount in Namespace
    Fixtures->>K8s: Create Role and RoleBinding (if access granted test)
    Fixtures->>K8s: Retrieve ServiceAccount token

    Test->>Client: Attempt connection with token
    alt Access Denied
        Client-->>Test: Raise Forbidden Error (403)
    else Access Granted
        Client-->>Test: Connection Succeeds
    end

    Test->>Fixtures: Teardown resources
    Fixtures->>K8s: Delete RoleBinding, Role, ServiceAccount, Namespace
Loading

Poem

In the warren of tests, new fixtures appear,
With ServiceAccounts hopping, their purpose is clear.
Roles and bindings, namespaces anew,
Rabbits ensure cleanup, as only they do.
Now access is tested, both denied and allowed—
A registry safer, of which we are proud!
🐇✨
"""

✨ Finishing Touches
  • 📝 Docstrings were successfully generated. (🔄 Check again to generate docstrings again)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 6, 2025

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee

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.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    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.
Supported labels

{'/wip', '/lgtm', '/hold', '/verified'}

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (7)
conftest.py (2)

112-114: Typo fixed – thanks, but description still ambiguous

You corrected “Comma-separated” (good) but the phrase
"Comma-separated str; specify inference service deployment modes tests to run in upgrade tests."
is grammatically off and a bit hard to parse. Consider something like:

-        help="Comma-separated str; specify inference service deployment modes tests to run in upgrade tests. "
+        help="Comma-separated list of deployment modes to execute during upgrade tests "
+             "(e.g. 'serverless,model_mesh').  If omitted, all modes will be tested."

133-139: Redundant default=False and missing dest clarification

action="store_true" already sets the default to False, so the explicit
default=False is unnecessary (and slightly misleading if you later decide to
change the default). Also note that pytest converts --foo-bar into the
destination foo_bar. You access the flag later via the raw option string,
which works, but is less discoverable than using the canonical dest name
(cluster_sanity_continue_on_failure). Proposed minimal clean-up:

-    cluster_sanity_group.addoption(
-        "--cluster-sanity-continue-on-failure",
-        action="store_true",
-        default=False,
-        help="If set, log a warning on cluster sanity failure but continue running tests. "
-        "Default is to skip tests on failure.",
-    )
+    cluster_sanity_group.addoption(
+        "--cluster-sanity-continue-on-failure",
+        action="store_true",
+        help="Continue running the suite even if cluster-sanity checks fail "
+             "(logs a warning instead of aborting).",
+    )
utilities/infra.py (1)

887-899: Early-exit path ignores junitxml_property

When --cluster-sanity-skip-check is supplied we return early, but the JUnit
report still claims the check passed (nothing is recorded). If you rely on
those properties downstream you may want to note that the check was skipped:

if request.session.config.getoption(skip_check_opt):
     LOGGER.warning(f"Skipping cluster sanity check entirely due to {skip_check_opt} flag.")
+    if junitxml_property:
+        junitxml_property("cluster_sanity_check_skipped", True)  # type: ignore[call-arg]
     return
tests/model_registry/rbac/test_mr_rbac_sa.py (2)

89-97: ForbiddenException assertions may raise AttributeError

mr_openapi.exceptions.ForbiddenException doesn’t guarantee .body or
.status. Safer pattern:

with pytest.raises(ForbiddenException) as exc:
    ModelRegistryClient(**client_args)

err: ForbiddenException = exc.value
status = getattr(err, "status", None)
assert status == 403, f"Expected 403, got {status}"

Otherwise the test itself can error instead of failing gracefully.


130-140: Success path does not verify functionality

You assert that the client initialises, but not that it can perform a simple
action (e.g. mr_client_success.list_models() or get_version()). A bad
token may still allow instantiation but fail on the first API call. Consider
adding one lightweight call to guarantee real access.

tests/model_registry/conftest.py (2)

377-378: Remove unused variables res and err

Ruff correctly flags these as unused. They can simply be replaced with _
place-holders:

-        res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True)
+        _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True)
🧰 Tools
🪛 Ruff (0.8.2)

377-377: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


377-377: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)


328-341: Generic except Exception hides real failure reason

Catching all exceptions then calling pytest.fail throws away the original
traceback, making debugging harder. Narrow the except clause to the expected
timeout/error type (e.g. TimeoutExpiredError) or re-raise after logging.

-        except Exception:
-            LOGGER.error(...)
-            pytest.fail(...)
+        except TimeoutExpiredError:
+            LOGGER.error(...)
+            pytest.fail(...)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd0335c and 9b5871d.

📒 Files selected for processing (4)
  • conftest.py (2 hunks)
  • tests/model_registry/conftest.py (5 hunks)
  • tests/model_registry/rbac/test_mr_rbac_sa.py (1 hunks)
  • utilities/infra.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
utilities/infra.py (2)
tests/conftest.py (3)
  • nodes (496-497)
  • dsci_resource (334-335)
  • dsc_resource (339-340)
utilities/exceptions.py (1)
  • ResourceNotReadyError (99-100)
tests/model_registry/rbac/test_mr_rbac_sa.py (2)
utilities/constants.py (3)
  • DscComponents (146-164)
  • Protocols (87-94)
  • ManagementState (151-153)
tests/model_registry/conftest.py (2)
  • sa_token (365-393)
  • model_registry_instance_rest_endpoint (197-200)
tests/model_registry/conftest.py (1)
tests/conftest.py (1)
  • admin_client (50-51)
🪛 Ruff (0.8.2)
tests/model_registry/conftest.py

377-377: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


377-377: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag left a comment

Choose a reason for hiding this comment

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

Thanks a lot for creating these tests so fast @lugi0 ! I have a few comments. Mostly about reorganizing things, removing cluster sanity part and cleaning up some generated docs/comments.

lugi0 added 2 commits May 7, 2025 12:36
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
@lugi0 lugi0 force-pushed the feature/rbac-sa branch from bfc204f to da9a5ac Compare May 7, 2025 10:36
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 3

🧹 Nitpick comments (5)
tests/model_registry/utils.py (3)

243-246: Docstring parameter name & typo – fix length spelling

ength (int, optional): … is missing the leading “l”.
The name should match the actual parameter (length) so IDEs and doc generators display correct information.

-        ength (int, optional): The desired length for the UUID-derived suffix.
+        length (int, optional): The desired length for the UUID-derived suffix.

254-256: Raise message refers to a non-existent variable

The error text mentions suffix_length, but the function argument is length. This can mislead callers and automated linters.

-        raise ValueError("suffix_length must be an integer between 1 and 32.")
+        raise ValueError("length must be an integer between 1 and 32.")

258-262: Kubernetes naming limits not enforced

A generated resource name may exceed 63 characters (prefix + “-” + length).
Consider guarding against this to avoid 422 Unprocessable Entity errors when the name is used for a Kubernetes object.

+    full_name = f"{prefix}-{suffix}"
+    if len(full_name) > 63:
+        raise ValueError(
+            f"Resulting name '{full_name}' is {len(full_name)} chars – "
+            "Kubernetes resource names must be ≤ 63."
+        )
+    return full_name
-
-    return f"{prefix}-{suffix}"
tests/model_registry/rbac/conftest.py (2)

60-64: Unused variables res, err – silence Ruff F841

The result and stderr are captured but never referenced.
Either log them or replace with “_” to indicate intentional discard.

-        res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
-        token = out.strip()
+        _, out, _ = run_command(
+            command=shlex.split(cmd),
+            verify_stderr=False,
+            check=True,
+            timeout=30,
+        )
+        token = out.strip()
🧰 Tools
🪛 Ruff (0.8.2)

62-62: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


62-62: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)


147-157: Possible 63-character overflow on binding_name

binding_name = f"{mr_access_role.name}-binding" can easily exceed K8s limits when the role name itself is already near 63 chars, causing RoleBinding creation to fail.

Consider truncating and/or hashing:

-from tests.model_registry.utils import generate_random_name
+from hashlib import sha1
+
+hash_suffix = sha1(mr_access_role.name.encode()).hexdigest()[:6]
+binding_name = f"{mr_access_role.name[:55]}-{hash_suffix}"
🛑 Comments failed to post (3)
tests/model_registry/utils.py (1)

265-266: 🛠️ Refactor suggestion

Incorrect use of str.strip() – may remove unintended characters

file_path.strip(".py") removes any combination of the characters “.”, “p”, “y” from both ends of the string, not the “.py” suffix specifically.
Use removesuffix(".py") (3.9+) or Path(file_path).stem to avoid accidental truncation.

-from tests.model_registry.utils import generate_random_name, generate_namespace_name
+# NOTE: use Path and removesuffix to safely drop '.py'
+from pathlib import Path
+
+def generate_namespace_name(file_path: str) -> str:
+    """Generate a DNS-1123-compliant namespace from a test file path."""
+    name = (
+        Path(file_path).name.removesuffix(".py")
+        .replace("/", "-")
+        .replace("_", "-")
+    )
+    return name[-63:].split("-", 1)[-1]

Committable suggestion skipped: line range outside the PR's diff.

tests/model_registry/rbac/conftest.py (2)

27-33: 🛠️ Refactor suggestion

Path split may raise IndexError in nested/invoked contexts

request.fspath.strpath.split(f"{os.path.dirname(__file__)}/")[1] assumes the test file path always contains the current directory as a separator and that the split returns ≥ 2 parts.
In parametrised, symlinked or external-execution scenarios this breaks.

Recommend using Path.relative_to or os.path.relpath with robust fallback:

-from os.path import dirname
-test_file = Path(request.fspath).relative_to(dirname(__file__)).as_posix()
-ns_name = generate_namespace_name(file_path=test_file)
+test_file = os.path.relpath(request.fspath.strpath, start=os.path.dirname(__file__))
+ns_name = generate_namespace_name(test_file)

Committable suggestion skipped: line range outside the PR's diff.


70-93: ⚠️ Potential issue

Overly broad except Exception hides programming errors

Catching the base Exception masks unexpected issues (e.g., TypeError, AttributeError).
Catch the specific exceptions raised by run_command (CalledProcessError, TimeoutExpired, FileNotFoundError) and let everything else propagate.

-    except Exception as e:  # Catch all exceptions from the try block
+    except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError) as e:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError) as e:
        error_type = type(e).__name__
        log_message = (
            f"Failed during token retrieval for SA '{sa_name}' in namespace '{namespace}'. "
            f"Error Type: {error_type}, Message: {str(e)}"
        )
        if isinstance(e, subprocess.CalledProcessError):
            # Add specific details for CalledProcessError
            # run_command already logs the error if log_errors=True and returncode !=0,
            # but we can add context here.
            stderr_from_exception = e.stderr.strip() if e.stderr else "N/A"
            log_message += f". Exit Code: {e.returncode}. Stderr from exception: {stderr_from_exception}"
        elif isinstance(e, subprocess.TimeoutExpired):
            timeout_value = getattr(e, "timeout", "N/A")
            log_message += f". Command timed out after {timeout_value} seconds."
        elif isinstance(e, FileNotFoundError):
            # This occurs if 'oc' is not found.
            # e.filename usually holds the name of the file that was not found.
            command_not_found = e.filename if hasattr(e, "filename") and e.filename else shlex.split(cmd)[0]
            log_message += f". Command '{command_not_found}' not found. Is it installed and in PATH?"

        LOGGER.error(log_message, exc_info=True)  # exc_info=True adds stack trace to the log
        raise

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/model_registry/rbac/conftest.py (1)

50-93: Robust token retrieval with comprehensive error handling

The sa_token fixture includes detailed error handling for different failure scenarios, making troubleshooting easier. However, there are unused variables in the command execution that should be addressed.

On line 62, variables res and err are assigned but never used. Consider using underscore notation for these unused variables:

-        res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
+        _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
🧰 Tools
🪛 Ruff (0.8.2)

62-62: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


62-62: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bfc204f and da9a5ac.

📒 Files selected for processing (4)
  • tests/model_registry/conftest.py (0 hunks)
  • tests/model_registry/rbac/conftest.py (1 hunks)
  • tests/model_registry/rbac/test_mr_rbac_sa.py (1 hunks)
  • tests/model_registry/utils.py (2 hunks)
💤 Files with no reviewable changes (1)
  • tests/model_registry/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/model_registry/utils.py
  • tests/model_registry/rbac/test_mr_rbac_sa.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/conftest.py (3)
tests/model_registry/utils.py (2)
  • generate_random_name (235-262)
  • generate_namespace_name (265-266)
tests/conftest.py (1)
  • admin_client (50-51)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (49-50)
🪛 Ruff (0.8.2)
tests/model_registry/rbac/conftest.py

62-62: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


62-62: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

🔇 Additional comments (6)
tests/model_registry/rbac/conftest.py (6)

1-15: Clean and comprehensive imports

The imports cover all necessary dependencies for Kubernetes resource management, command execution, and logging. The code properly leverages existing utilities like generate_random_name and generate_namespace_name.


21-34: Well-implemented namespace fixture with proper cleanup

The sa_namespace fixture correctly generates a namespace name based on the test file path and uses a context manager to ensure automatic cleanup after tests. The implementation includes appropriate waiting for the namespace to become active and comprehensive logging.


36-48: Good service account creation and cleanup pattern

The service_account fixture follows best practices by using a context manager for lifecycle management and waiting for resource readiness. The random name generation helps ensure test isolation.


98-135: Well-defined role with appropriate permissions

The mr_access_role fixture creates a role with minimal permissions (only "get" access to a specific service). The use of labels for tracking and the descriptive naming strategy make this easy to understand and debug.


137-177: Correct role binding implementation with group-based access

The mr_access_role_binding fixture properly links the role to all service accounts in the test namespace using a group binding. This approach is more flexible than binding to a specific service account. The consistent use of labeling and resource management aligns with the rest of the fixtures.


1-177: Overall well-structured test fixtures with good isolation

The overall implementation of these fixtures provides a solid foundation for RBAC testing. The consistent function-level scope ensures each test runs with a fresh environment, and the use of context managers guarantees proper cleanup. The detailed logging throughout will be valuable for troubleshooting test failures.

🧰 Tools
🪛 Ruff (0.8.2)

62-62: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


62-62: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

Signed-off-by: lugi0 <lgiorgi@redhat.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/model_registry/rbac/conftest.py (1)

63-63: Remove unused variables from run_command assignment

The variables res and err are assigned but never used in the function.

- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
+ _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
🧰 Tools
🪛 Ruff (0.8.2)

63-63: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


63-63: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da9a5ac and 84dfba7.

📒 Files selected for processing (2)
  • tests/model_registry/rbac/conftest.py (1 hunks)
  • tests/model_registry/utils.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/model_registry/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/conftest.py (3)
tests/model_registry/utils.py (2)
  • generate_random_name (235-262)
  • generate_namespace_name (265-266)
tests/conftest.py (1)
  • admin_client (50-51)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (49-50)
🪛 Ruff (0.8.2)
tests/model_registry/rbac/conftest.py

63-63: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


63-63: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

🔇 Additional comments (6)
tests/model_registry/rbac/conftest.py (6)

1-19: Well-structured imports and constants setup

The imports are comprehensive and well-organized, covering all necessary Kubernetes resources and utility functions. The constant for token duration is appropriately defined with a descriptive name.


21-35: Good test isolation with namespace fixture

The sa_namespace fixture follows best practices for test isolation by creating a temporary namespace with automatic cleanup via context manager. The use of generate_namespace_name to derive names from test file paths ensures uniqueness and traceability.


37-49: Well-implemented ServiceAccount fixture

The service_account fixture correctly creates a test ServiceAccount with a randomly generated name, proper scoping, and automatic cleanup. This provides good isolation for RBAC testing.


51-94: Comprehensive error handling in token retrieval

The sa_token fixture has excellent error handling for different failure scenarios (command errors, timeouts, missing binaries). The detailed error logging with context and stack traces will be valuable for debugging test failures.

🧰 Tools
🪛 Ruff (0.8.2)

63-63: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


63-63: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)


96-136: Well-defined RBAC Role fixture

The mr_access_role fixture creates a properly scoped Role with specific permissions limited to the model registry service. The use of a context manager ensures cleanup, and the labels provide good traceability.


138-178: Properly configured RoleBinding fixture

The mr_access_role_binding fixture correctly links the Role to all ServiceAccounts in the test namespace through the group binding. The logging provides clear visibility into the binding creation and cleanup process.

Signed-off-by: lugi0 <lgiorgi@redhat.com>
@lugi0
Copy link
Copy Markdown
Contributor Author

lugi0 commented May 7, 2025

/verified

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/model_registry/rbac/conftest.py (1)

51-94: Remove unused variables from run_command

The error handling in the sa_token fixture is comprehensive, but there are unused variables in the run_command call.

- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
+ _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)

Or if you need to preserve the variables for future use:

- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
+ result, out, error = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
  # Use result and error variables somewhere
🧰 Tools
🪛 Ruff (0.8.2)

63-63: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


63-63: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84dfba7 and ff71406.

📒 Files selected for processing (1)
  • tests/model_registry/rbac/conftest.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/conftest.py (3)
tests/model_registry/utils.py (2)
  • generate_random_name (235-262)
  • generate_namespace_name (265-266)
tests/conftest.py (1)
  • admin_client (50-51)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (49-50)
🪛 Ruff (0.8.2)
tests/model_registry/rbac/conftest.py

63-63: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


63-63: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

🔇 Additional comments (7)
tests/model_registry/rbac/conftest.py (7)

1-15: Good organization of imports

The imports are well-organized with standard library imports first, followed by Kubernetes-related imports, and finally project-specific utilities and constants. This follows Python best practices for import organization.


17-18: Constants naming follows convention

Good practice using uppercase for the DEFAULT_TOKEN_DURATION constant, and setting up a module-level logger with a proper name.


21-35: Well-implemented namespace fixture with automatic cleanup

The sa_namespace fixture is well-designed with:

  • Appropriate function scope
  • Dynamic namespace naming based on test file path
  • Context manager for automatic cleanup
  • Status verification with timeout
  • Clear logging at each step

This ensures test isolation and proper resource management.


37-49: Good service account setup with dependency on namespace

The service_account fixture correctly depends on sa_namespace and follows the same pattern of using a context manager for cleanup, with proper logging and random name generation.


71-93: Excellent error handling implementation

The error handling is very thorough, with:

  • Type-specific error messages for different exception types
  • Detailed context for each error type
  • Proper logging with stack traces
  • Appropriate re-raising of exceptions

This will greatly help with debugging test failures.


99-136: Well-structured role creation with appropriate permissions

The mr_access_role fixture:

  • Creates a role with a clear naming convention
  • Properly scopes permissions using resourceNames to limit to the specific MR instance
  • Uses appropriate labels for tracking test resources
  • Correctly uses the model_registry_namespace from the imported fixture
  • Implements automatic cleanup via context manager

This follows security best practices by granting minimal necessary permissions.


138-177: Properly implemented role binding with group-based access

The mr_access_role_binding fixture correctly:

  • Creates a binding between the role and all service accounts in the test namespace
  • Uses the Group kind with system:serviceaccounts prefix (Kubernetes best practice)
  • Includes proper labels for resource tracking
  • Waits for the resource to be fully created
  • Uses a context manager for cleanup

This approach allows multiple service accounts in the namespace to receive the role, which is more flexible than individual bindings.

@lugi0
Copy link
Copy Markdown
Contributor Author

lugi0 commented May 7, 2025

/verified

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff71406 and fdb067c.

📒 Files selected for processing (1)
  • tests/model_registry/rbac/conftest.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/conftest.py (3)
tests/model_registry/utils.py (2)
  • generate_random_name (235-262)
  • generate_namespace_name (265-266)
tests/conftest.py (1)
  • admin_client (50-51)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (49-50)
🪛 Ruff (0.8.2)
tests/model_registry/rbac/conftest.py

59-59: Local variable res is assigned to but never used

Remove assignment to unused variable res

(F841)


59-59: Local variable err is assigned to but never used

Remove assignment to unused variable err

(F841)

🔇 Additional comments (6)
tests/model_registry/rbac/conftest.py (6)

1-15: Well-structured import section with appropriate dependencies.

The imports are well-organized and include all necessary modules for Kubernetes resource management, command execution, and utility functions. Good job on importing the specific utility functions generate_random_name and generate_namespace_name from the existing utilities module.


21-33: Good implementation of namespace fixture with proper cleanup.

The sa_namespace fixture effectively:

  • Creates an isolated test namespace with a deterministic name based on the test file path
  • Uses context manager pattern for automatic cleanup
  • Appropriately waits for the namespace to become active with a reasonable timeout
  • Includes helpful logging for troubleshooting

This provides a clean environment for each test and ensures proper resource cleanup.


35-45: Clean ServiceAccount fixture with appropriate error handling.

The implementation properly generates a random name with a useful prefix, creates the account within the test namespace, and ensures resource readiness before returning. The function-scoped fixture correctly ties the ServiceAccount's lifecycle to the namespace.


67-89: Excellent error handling with detailed logging.

This comprehensive error handling is a great practice. The code:

  • Handles different exception types with specific contextual information
  • Provides detailed error messages with context about which resource failed
  • Includes stack traces in logs for debugging
  • Properly propagates exceptions after logging

This will make troubleshooting much easier if token retrieval fails.


95-132: Well-structured Role fixture with proper resource configuration.

The mr_access_role fixture correctly:

  • Creates a named role in the model registry namespace
  • Defines precise RBAC rules limiting access to only the specific model registry service
  • Includes appropriate labels for tracking and management
  • Uses context manager for automatic cleanup
  • Provides detailed logging throughout the resource lifecycle

The granular permission (only "get" on the specific service) follows security best practices by granting minimal permissions.


134-173: Complete RoleBinding implementation with appropriate subject configuration.

The mr_access_role_binding fixture properly:

  • Creates a binding with a descriptive name derived from the role
  • Correctly links to all service accounts in the test namespace via group binding
  • Includes the same labels as the role for consistency
  • Uses context manager for automatic cleanup
  • Provides clear logging of the binding details

Binding to the service account group rather than individual accounts is an efficient approach.

@rhods-ci-bot rhods-ci-bot added the Verified Verified pr in Jenkins label May 7, 2025
@lugi0 lugi0 requested review from dbasunag and fege May 7, 2025 11:08
"apiGroups": [""],
"resources": ["services"],
"resourceNames": [MR_INSTANCE_NAME], # Grant access only to the specific MR service object
"verbs": ["get"],
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.

should we pass the verbs as input param so to have more flexibility?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in our case we only really care about get on the MR Instance, so I don't think it's really needed. If we were to generalize this to create any Role then I'd agree with you

subjects_name=f"system:serviceaccounts:{sa_namespace.name}",
subjects_api_group="rbac.authorization.k8s.io", # This is the default apiGroup for Group kind
# Role reference parameters
role_ref_kind="Role",
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.

since we are passing already mr_access_role we could use it mr_access_role.kind

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, good idea. if you rebase in your PR can you add it?

Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

@dbasunag dbasunag merged commit c956dff into opendatahub-io:main May 7, 2025
18 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 7, 2025

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

coderabbitai bot added a commit that referenced this pull request May 7, 2025
Docstrings generation was requested by @lugi0.

* #296 (comment)

The following files were modified:

* `tests/model_registry/rbac/conftest.py`
* `tests/model_registry/rbac/test_mr_rbac_sa.py`
* `tests/model_registry/utils.py`
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented May 7, 2025

Note

Generated docstrings for this pull request at #298

dbasunag pushed a commit to dbasunag/opendatahub-tests that referenced this pull request May 8, 2025
* feat: add RBAC test for SA token

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: address review comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: incorporate coderabbit suggestions

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unneeded variable

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove excessive logs

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>
dbasunag pushed a commit to dbasunag/opendatahub-tests that referenced this pull request May 8, 2025
* feat: add RBAC test for SA token

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: address review comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: incorporate coderabbit suggestions

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unneeded variable

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove excessive logs

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>
dbasunag added a commit that referenced this pull request May 20, 2025
* updates to test_registering_model() based on previous review comments

* [do-not-review]must-gather collection at failure point

updates! 1176505

updates! 12d9c08

updates! 12d9c08

updates! 65e0213

* [ModelRegistry] ensure RunAsUser and RunAsGroup are not set explicitly (#226)

updates! 4813f2b

updates! 20cd457

updates! b126825

updates! 809cca7

* Lock file maintenance (#241)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* RHOAIENG-22058: chore(workbenches): add test_create_simple_notebook to smoke (#238)

* Remove uv cache from dockerfile to support running in envs like openshift-ci (#239)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: remove uv cache from dockerfile

* `is_managed_cluster` fix condition (#243)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: replace iter with list

* fix: add logger info

* RHOAIENG-22057: fix(workbenches): correct the check for spawned workbench (#242)

There can only ever be a single workbench pod started.

Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>

* RHOAIENG-22057: fix(workbenches): check for internal image registry and adjust the image path accordingly (#244)

* now yielding TimeoutSampler get_pods_by_isvc_label func output and handling raised ResourceNotFoundError (#237)

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [model server] add auth test to upgrade (#245)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: add auth test to upgrade

* feat: add auth test to upgrade

feat: add auth test to upgrade

* fix: dsci name in func

* [pre-commit.ci] pre-commit autoupdate (#246)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.11.5](astral-sh/ruff-pre-commit@v0.11.4...v0.11.5)
- [github.com/gitleaks/gitleaks: v8.24.2 → v8.24.3](gitleaks/gitleaks@v8.24.2...v8.24.3)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix add-remove-labels workflow (#249)

* Add Cluster sanity checks before test execution (#235)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity add readme

* fix: tix str typo

* fix: address comments

* fix: address review comments

* fix: address comment

* fix: use dsci from global config

* fix: remove duplicate fixture

* add labeler to add labels to prs based on areas impacted (#248)

* on rebase clean commented-by- labels (#251)

* [model registry] update namespace code and rearrange tests (#247)

* updates to test_registering_model() based on previous review comments

* update namespace code and rearrange tests

* remove unnecessary argument from function call (#255)

* on rebase clean commented-by- labels

* remove unnecessary argument from function call

* feat: add ocp_interop marker (#260)

* Lock file maintenance (#259)

* Lock file maintenance

* fix: add marshmallow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* [pre-commit.ci] pre-commit autoupdate (#263)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.5 → v0.11.6](astral-sh/ruff-pre-commit@v0.11.5...v0.11.6)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* feat: add upgrade tests (#258)

* Remove flake8 ignore list (#265)

* fix: remove flake8 ignore

* fix: remove flake8 ignore

* [model server] Remove pod pre-checks for image pull and fix `TestServerlessScaleToZero` (#256)

* fix: update tests

* fix: update tests

* fix: update tests

* fix: save test dep name

* fix: minio mm external route

* fix: address comemnt

* fix: address comemnt

* fix: address comemnt

* Update python-dependencies (major) (#267)

* Update python-dependencies

* fix: marshmellow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* Adding Test For InferenceService Zero Initial Scale (#262)

* adding test for zero initial scale

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fixing precommit error

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* using label_selectors when getting deployment

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* adding argument names to func call and running pre-commit on all files

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* fixing bug in ovms_kserve_inference_service function that was preventing isvcs from being created with 0 min-replicas

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat: move interop marker (#268)

* feat: Add upgrade tests for TrustyAIService (#250)

* feat: Add upgrade tests for TrustyAIService

* Move upgrade README.md to docs/UPGRADE.md

* fix: reuse kwargs in TrustyAIService fixture

* fix: address comments, reuse kwargs, add docstrings

---------

Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix ns deletion logic  (#272)

* fix: fix resource deletion fixture logic

* fix: fix resource deletion fixture logic

* feat: fail on missing operators (#257)

* fix: update tests

* fix: update tests

* feat: fail on missing operators

* fix: rename to dependent

* fix: address comment

* fix: add log on failure

* fix: type in raise

* fix: remove MR check

* fix: remove MR check

* fix: use package scope

* Add basic InferenceGraph deployment check (#233)

* Add basic InferenceGraph deployment check

This adds a test that deploys an InferenceGraph (IG), sends an inference request to the IG and verifies that the request succeeds.

The deployed InferenceGraph is based on the example on the KServe documentation available in the following URL: https://kserve.github.io/website/0.15/modelserving/inference_graph/image_pipeline/. The example was adapted to run in openvino (which is a supported server in ODH), rather than TorchServe.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Use cloud storage in InferenceGraph test

Use cloud storage for the models, instead of OCI

* Feedback: Ruth

* Feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Apply Ruth suggestions

Acknowledgement to @rnester for these changes.

* More feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* fix: address 503 (#274)

* [model server] Move to using unprivileged_client in tests (#273)

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* fix: unpri selection

* Update MinIo pod privileges to run on ocp 4.19 (#277)

* fix: add securityContext for minio pod

* fix: minio on 4.19

* [model server]  add multi node args check (#276)

* feat: add multi node args

* feat: add multi node args

* fix: add wait on delete

* fix: update new test

* [pre-commit.ci] pre-commit autoupdate (#279)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.6 → v0.11.7](astral-sh/ruff-pre-commit@v0.11.6...v0.11.7)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* `verify_no_failed_pods` - exclude container failures when model mesh deployment (#278)

* fix: mm container

* fix: update condition

* feat: add test for incorrect DB TLS config in Trusty AI (#221)

* feat: add test for incorrect DB TLS config in Trusty AI

* refactor: remove unused method from utils

* feat: move TrustyAI test to own file

* refactor: change name of db fixtures and deduplicate code

* TrustyAI Service creation code refactor into own method
* Move db secret setter to utils
* Remove test from test_fairness as test moved to own file

* docs: add description to TrustyAI invalid DB TLS config test

* fix: check TrustyAIService container for Terminated status in lastStatus

* fix: change name of terminal_state getter function

* fix: change to a valid certificate and check for service failure

* fix: address PR 221 reviewer feedback

* revert wait_for_pods to wait_for_mariadb_pods
* improve error checking logic
* remove un-necessary wrapper function

* docs: add docstring to create_trustyai_service method

* docs: add docstring to trustyai_service_with_invalid_db_cert

* fix: fix invalid return type for trustyai_db_ca_secret

* feat: use retry decorator in validate trustyai_service_db_conn_failure method

* fix: remove unnecessary return from validate db_conn_failure method

* docs: add spacing between lines of docstring

* refactor: create constants trustyai metrics and db storage config

* refactor: address reviewer feedback

- change docstring to correct formatting
- remove len(0) check
- no templating for error text

* fix: use regex instead of in operator to check for error condition

* docs: add correct formatting to docstrings

* fix: use namespace.name instead of namespace in Pod.get

* fix: remove \s from regex to check for spaces

* refactor: add Raises section in docstring and use single string for pytest.fail

* feat: use raise instead of pytest.fail

- create new exception TooManyPodsError
- create new exception UnexpectedFailureError
- replace pytest.fail with raise and handle exceptions in retry
-

* fix: change default of teardown to True in TrustyAIService

* docs: correct typo in trustyai docstring

* docs: fix raises in docs and fix formatting

* fix: fix create_trustyai_service namespace args issue

* docs: add default for name arg in create tai svc func

* [model server] Fix runtime request.param name to use external route (#280)

* fix: fix param name

* fix: fix param name

* feat: add certs when sending requests to TrustyAIService (#266)

* Wait for pods to be in running state before attempting to create ModelRegistry (#270)

* on rebase clean commented-by- labels

* Wait for pods to be in running state before attempting to create ModelRegistry

* Address Exception in thread Thread-1 (_monitor) error (#286)

* chore(deps): lock file maintenance (#287)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [pre-commit.ci] pre-commit autoupdate (#292)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.7 → v0.11.8](astral-sh/ruff-pre-commit@v0.11.7...v0.11.8)
- [github.com/gitleaks/gitleaks: v8.24.3 → v8.25.1](gitleaks/gitleaks@v8.24.3...v8.25.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Wait for dsc and dsci ready state in cluster_sanity check (#293)

* fix(workbenches): implement get_username for OpenShift <=4.14 (#275)

Turns out SelfSubjectReview is only available starting OpenShift 4.15.

fixup incorporate User resource
* RedHatQE/openshift-python-wrapper#2387

fixup incorporate SelfSubjectReview resource
* RedHatQE/openshift-python-wrapper#2389

Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>

* replace the bot account with one owned by testdevops (#291)

* Fix for post upgarde operator check (#297)

Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>

* Add test for Model Registry RBAC for SA token (#296)

* feat: add RBAC test for SA token

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: address review comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: incorporate coderabbit suggestions

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unneeded variable

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove excessive logs

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* Support /build-push-pr-image comment to push image to quay for testing via jenkins (#290)

updates! 678b389

* Add tests for model_artifact update validations (#284)

* Add tests for model_artifact update validations

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updates fixing pre-commit

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update package

* minor updates

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* address review comments

updates! 50ec24b

updates! f3a6c3e

updates! 792156f

updates! 399aa10

updates! 5080e3b

updates! c34f4e7

updates! a1d7baa

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Jiri Daněk <jdanek@redhat.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>
Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>
Co-authored-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
Co-authored-by: Edgar Hernández <ehernand@redhat.com>
Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com>
Co-authored-by: Milind Waykole <mwaykole@redhat.com>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
sheltoncyril added a commit to sheltoncyril/opendatahub-tests that referenced this pull request Jun 3, 2025
* updates to test_registering_model() based on previous review comments

* [do-not-review]must-gather collection at failure point

updates! 1176505

updates! 12d9c08

updates! 12d9c08

updates! 65e0213

* [ModelRegistry] ensure RunAsUser and RunAsGroup are not set explicitly (#226)

updates! 4813f2b

updates! 20cd457

updates! b126825

updates! 809cca7

* Lock file maintenance (#241)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* RHOAIENG-22058: chore(workbenches): add test_create_simple_notebook to smoke (#238)

* Remove uv cache from dockerfile to support running in envs like openshift-ci (#239)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: remove uv cache from dockerfile

* `is_managed_cluster` fix condition (#243)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: replace iter with list

* fix: add logger info

* RHOAIENG-22057: fix(workbenches): correct the check for spawned workbench (#242)

There can only ever be a single workbench pod started.

Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>

* RHOAIENG-22057: fix(workbenches): check for internal image registry and adjust the image path accordingly (#244)

* now yielding TimeoutSampler get_pods_by_isvc_label func output and handling raised ResourceNotFoundError (#237)

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [model server] add auth test to upgrade (#245)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: add auth test to upgrade

* feat: add auth test to upgrade

feat: add auth test to upgrade

* fix: dsci name in func

* [pre-commit.ci] pre-commit autoupdate (#246)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.11.5](astral-sh/ruff-pre-commit@v0.11.4...v0.11.5)
- [github.com/gitleaks/gitleaks: v8.24.2 → v8.24.3](gitleaks/gitleaks@v8.24.2...v8.24.3)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix add-remove-labels workflow (#249)

* Add Cluster sanity checks before test execution (#235)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity add readme

* fix: tix str typo

* fix: address comments

* fix: address review comments

* fix: address comment

* fix: use dsci from global config

* fix: remove duplicate fixture

* add labeler to add labels to prs based on areas impacted (#248)

* on rebase clean commented-by- labels (#251)

* [model registry] update namespace code and rearrange tests (#247)

* updates to test_registering_model() based on previous review comments

* update namespace code and rearrange tests

* remove unnecessary argument from function call (#255)

* on rebase clean commented-by- labels

* remove unnecessary argument from function call

* feat: add ocp_interop marker (#260)

* Lock file maintenance (#259)

* Lock file maintenance

* fix: add marshmallow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* [pre-commit.ci] pre-commit autoupdate (#263)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.5 → v0.11.6](astral-sh/ruff-pre-commit@v0.11.5...v0.11.6)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* feat: add upgrade tests (#258)

* Remove flake8 ignore list (#265)

* fix: remove flake8 ignore

* fix: remove flake8 ignore

* [model server] Remove pod pre-checks for image pull and fix `TestServerlessScaleToZero` (#256)

* fix: update tests

* fix: update tests

* fix: update tests

* fix: save test dep name

* fix: minio mm external route

* fix: address comemnt

* fix: address comemnt

* fix: address comemnt

* Update python-dependencies (major) (#267)

* Update python-dependencies

* fix: marshmellow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* Adding Test For InferenceService Zero Initial Scale (#262)

* adding test for zero initial scale

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fixing precommit error

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* using label_selectors when getting deployment

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* adding argument names to func call and running pre-commit on all files

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* fixing bug in ovms_kserve_inference_service function that was preventing isvcs from being created with 0 min-replicas

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat: move interop marker (#268)

* feat: Add upgrade tests for TrustyAIService (#250)

* feat: Add upgrade tests for TrustyAIService

* Move upgrade README.md to docs/UPGRADE.md

* fix: reuse kwargs in TrustyAIService fixture

* fix: address comments, reuse kwargs, add docstrings

---------

Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix ns deletion logic  (#272)

* fix: fix resource deletion fixture logic

* fix: fix resource deletion fixture logic

* feat: fail on missing operators (#257)

* fix: update tests

* fix: update tests

* feat: fail on missing operators

* fix: rename to dependent

* fix: address comment

* fix: add log on failure

* fix: type in raise

* fix: remove MR check

* fix: remove MR check

* fix: use package scope

* Add basic InferenceGraph deployment check (#233)

* Add basic InferenceGraph deployment check

This adds a test that deploys an InferenceGraph (IG), sends an inference request to the IG and verifies that the request succeeds.

The deployed InferenceGraph is based on the example on the KServe documentation available in the following URL: https://kserve.github.io/website/0.15/modelserving/inference_graph/image_pipeline/. The example was adapted to run in openvino (which is a supported server in ODH), rather than TorchServe.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Use cloud storage in InferenceGraph test

Use cloud storage for the models, instead of OCI

* Feedback: Ruth

* Feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Apply Ruth suggestions

Acknowledgement to @rnester for these changes.

* More feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* fix: address 503 (#274)

* [model server] Move to using unprivileged_client in tests (#273)

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* fix: unpri selection

* Update MinIo pod privileges to run on ocp 4.19 (#277)

* fix: add securityContext for minio pod

* fix: minio on 4.19

* [model server]  add multi node args check (#276)

* feat: add multi node args

* feat: add multi node args

* fix: add wait on delete

* fix: update new test

* [pre-commit.ci] pre-commit autoupdate (#279)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.6 → v0.11.7](astral-sh/ruff-pre-commit@v0.11.6...v0.11.7)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* `verify_no_failed_pods` - exclude container failures when model mesh deployment (#278)

* fix: mm container

* fix: update condition

* feat: add test for incorrect DB TLS config in Trusty AI (#221)

* feat: add test for incorrect DB TLS config in Trusty AI

* refactor: remove unused method from utils

* feat: move TrustyAI test to own file

* refactor: change name of db fixtures and deduplicate code

* TrustyAI Service creation code refactor into own method
* Move db secret setter to utils
* Remove test from test_fairness as test moved to own file

* docs: add description to TrustyAI invalid DB TLS config test

* fix: check TrustyAIService container for Terminated status in lastStatus

* fix: change name of terminal_state getter function

* fix: change to a valid certificate and check for service failure

* fix: address PR 221 reviewer feedback

* revert wait_for_pods to wait_for_mariadb_pods
* improve error checking logic
* remove un-necessary wrapper function

* docs: add docstring to create_trustyai_service method

* docs: add docstring to trustyai_service_with_invalid_db_cert

* fix: fix invalid return type for trustyai_db_ca_secret

* feat: use retry decorator in validate trustyai_service_db_conn_failure method

* fix: remove unnecessary return from validate db_conn_failure method

* docs: add spacing between lines of docstring

* refactor: create constants trustyai metrics and db storage config

* refactor: address reviewer feedback

- change docstring to correct formatting
- remove len(0) check
- no templating for error text

* fix: use regex instead of in operator to check for error condition

* docs: add correct formatting to docstrings

* fix: use namespace.name instead of namespace in Pod.get

* fix: remove \s from regex to check for spaces

* refactor: add Raises section in docstring and use single string for pytest.fail

* feat: use raise instead of pytest.fail

- create new exception TooManyPodsError
- create new exception UnexpectedFailureError
- replace pytest.fail with raise and handle exceptions in retry
-

* fix: change default of teardown to True in TrustyAIService

* docs: correct typo in trustyai docstring

* docs: fix raises in docs and fix formatting

* fix: fix create_trustyai_service namespace args issue

* docs: add default for name arg in create tai svc func

* [model server] Fix runtime request.param name to use external route (#280)

* fix: fix param name

* fix: fix param name

* feat: add certs when sending requests to TrustyAIService (#266)

* Wait for pods to be in running state before attempting to create ModelRegistry (#270)

* on rebase clean commented-by- labels

* Wait for pods to be in running state before attempting to create ModelRegistry

* Address Exception in thread Thread-1 (_monitor) error (opendatahub-io#286)

* chore(deps): lock file maintenance (opendatahub-io#287)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [pre-commit.ci] pre-commit autoupdate (opendatahub-io#292)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.7 → v0.11.8](astral-sh/ruff-pre-commit@v0.11.7...v0.11.8)
- [github.com/gitleaks/gitleaks: v8.24.3 → v8.25.1](gitleaks/gitleaks@v8.24.3...v8.25.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Wait for dsc and dsci ready state in cluster_sanity check (opendatahub-io#293)

* fix(workbenches): implement get_username for OpenShift <=4.14 (#275)

Turns out SelfSubjectReview is only available starting OpenShift 4.15.

fixup incorporate User resource
* RedHatQE/openshift-python-wrapper#2387

fixup incorporate SelfSubjectReview resource
* RedHatQE/openshift-python-wrapper#2389

Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>

* replace the bot account with one owned by testdevops (opendatahub-io#291)

* Fix for post upgarde operator check (opendatahub-io#297)

Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>

* Add test for Model Registry RBAC for SA token (opendatahub-io#296)

* feat: add RBAC test for SA token

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: address review comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: incorporate coderabbit suggestions

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unneeded variable

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove excessive logs

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* Support /build-push-pr-image comment to push image to quay for testing via jenkins (opendatahub-io#290)

updates! 678b389

* Add tests for model_artifact update validations (#284)

* Add tests for model_artifact update validations

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updates fixing pre-commit

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update package

* minor updates

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* address review comments

updates! 50ec24b

updates! f3a6c3e

updates! 792156f

updates! 399aa10

updates! 5080e3b

updates! c34f4e7

updates! a1d7baa

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Jiri Daněk <jdanek@redhat.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>
Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>
Co-authored-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
Co-authored-by: Edgar Hernández <ehernand@redhat.com>
Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com>
Co-authored-by: Milind Waykole <mwaykole@redhat.com>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
@coderabbitai coderabbitai bot mentioned this pull request Jun 5, 2025
3 tasks
adolfo-ab added a commit to adolfo-ab/opendatahub-tests that referenced this pull request Jun 11, 2025
* updates to test_registering_model() based on previous review comments

* [do-not-review]must-gather collection at failure point

updates! 1176505

updates! 12d9c08

updates! 12d9c08

updates! 65e0213

* [ModelRegistry] ensure RunAsUser and RunAsGroup are not set explicitly (opendatahub-io#226)

updates! 4813f2b

updates! 20cd457

updates! b126825

updates! 809cca7

* Lock file maintenance (opendatahub-io#241)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* RHOAIENG-22058: chore(workbenches): add test_create_simple_notebook to smoke (opendatahub-io#238)

* Remove uv cache from dockerfile to support running in envs like openshift-ci (opendatahub-io#239)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: remove uv cache from dockerfile

* `is_managed_cluster` fix condition (opendatahub-io#243)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* fix: replace iter with list

* fix: add logger info

* RHOAIENG-22057: fix(workbenches): correct the check for spawned workbench (opendatahub-io#242)

There can only ever be a single workbench pod started.

Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>

* RHOAIENG-22057: fix(workbenches): check for internal image registry and adjust the image path accordingly (opendatahub-io#244)

* now yielding TimeoutSampler get_pods_by_isvc_label func output and handling raised ResourceNotFoundError (opendatahub-io#237)

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [model server] add auth test to upgrade (opendatahub-io#245)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: add auth test to upgrade

* feat: add auth test to upgrade

feat: add auth test to upgrade

* fix: dsci name in func

* [pre-commit.ci] pre-commit autoupdate (opendatahub-io#246)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.11.5](astral-sh/ruff-pre-commit@v0.11.4...v0.11.5)
- [github.com/gitleaks/gitleaks: v8.24.2 → v8.24.3](gitleaks/gitleaks@v8.24.2...v8.24.3)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix add-remove-labels workflow (opendatahub-io#249)

* Add Cluster sanity checks before test execution (opendatahub-io#235)

* Create size-labeler.yml

* Delete .github/workflows/size-labeler.yml

* model mesh - add auth tests

* xx

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity

* feat: cluster sanity add readme

* fix: tix str typo

* fix: address comments

* fix: address review comments

* fix: address comment

* fix: use dsci from global config

* fix: remove duplicate fixture

* add labeler to add labels to prs based on areas impacted (opendatahub-io#248)

* on rebase clean commented-by- labels (opendatahub-io#251)

* [model registry] update namespace code and rearrange tests (opendatahub-io#247)

* updates to test_registering_model() based on previous review comments

* update namespace code and rearrange tests

* remove unnecessary argument from function call (opendatahub-io#255)

* on rebase clean commented-by- labels

* remove unnecessary argument from function call

* feat: add ocp_interop marker (opendatahub-io#260)

* Lock file maintenance (opendatahub-io#259)

* Lock file maintenance

* fix: add marshmallow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* [pre-commit.ci] pre-commit autoupdate (opendatahub-io#263)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.5 → v0.11.6](astral-sh/ruff-pre-commit@v0.11.5...v0.11.6)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* feat: add upgrade tests (opendatahub-io#258)

* Remove flake8 ignore list (opendatahub-io#265)

* fix: remove flake8 ignore

* fix: remove flake8 ignore

* [model server] Remove pod pre-checks for image pull and fix `TestServerlessScaleToZero` (opendatahub-io#256)

* fix: update tests

* fix: update tests

* fix: update tests

* fix: save test dep name

* fix: minio mm external route

* fix: address comemnt

* fix: address comemnt

* fix: address comemnt

* Update python-dependencies (major) (opendatahub-io#267)

* Update python-dependencies

* fix: marshmellow version

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: rnetser <rnetser@redhat.com>

* Adding Test For InferenceService Zero Initial Scale (opendatahub-io#262)

* adding test for zero initial scale

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fixing precommit error

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* using label_selectors when getting deployment

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* adding argument names to func call and running pre-commit on all files

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

* fixing bug in ovms_kserve_inference_service function that was preventing isvcs from being created with 0 min-replicas

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat: move interop marker (opendatahub-io#268)

* feat: Add upgrade tests for TrustyAIService (opendatahub-io#250)

* feat: Add upgrade tests for TrustyAIService

* Move upgrade README.md to docs/UPGRADE.md

* fix: reuse kwargs in TrustyAIService fixture

* fix: address comments, reuse kwargs, add docstrings

---------

Co-authored-by: Ruth Netser <rnetser@redhat.com>

* Fix ns deletion logic  (opendatahub-io#272)

* fix: fix resource deletion fixture logic

* fix: fix resource deletion fixture logic

* feat: fail on missing operators (opendatahub-io#257)

* fix: update tests

* fix: update tests

* feat: fail on missing operators

* fix: rename to dependent

* fix: address comment

* fix: add log on failure

* fix: type in raise

* fix: remove MR check

* fix: remove MR check

* fix: use package scope

* Add basic InferenceGraph deployment check (opendatahub-io#233)

* Add basic InferenceGraph deployment check

This adds a test that deploys an InferenceGraph (IG), sends an inference request to the IG and verifies that the request succeeds.

The deployed InferenceGraph is based on the example on the KServe documentation available in the following URL: https://kserve.github.io/website/0.15/modelserving/inference_graph/image_pipeline/. The example was adapted to run in openvino (which is a supported server in ODH), rather than TorchServe.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Use cloud storage in InferenceGraph test

Use cloud storage for the models, instead of OCI

* Feedback: Ruth

* Feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Apply Ruth suggestions

Acknowledgement to @rnester for these changes.

* More feedback: Ruth

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* fix: address 503 (opendatahub-io#274)

* [model server] Move to using unprivileged_client in tests (opendatahub-io#273)

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* feat: use unprivileged_client

* fix: unpri selection

* Update MinIo pod privileges to run on ocp 4.19 (opendatahub-io#277)

* fix: add securityContext for minio pod

* fix: minio on 4.19

* [model server]  add multi node args check (opendatahub-io#276)

* feat: add multi node args

* feat: add multi node args

* fix: add wait on delete

* fix: update new test

* [pre-commit.ci] pre-commit autoupdate (opendatahub-io#279)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.6 → v0.11.7](astral-sh/ruff-pre-commit@v0.11.6...v0.11.7)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>

* `verify_no_failed_pods` - exclude container failures when model mesh deployment (opendatahub-io#278)

* fix: mm container

* fix: update condition

* feat: add test for incorrect DB TLS config in Trusty AI (opendatahub-io#221)

* feat: add test for incorrect DB TLS config in Trusty AI

* refactor: remove unused method from utils

* feat: move TrustyAI test to own file

* refactor: change name of db fixtures and deduplicate code

* TrustyAI Service creation code refactor into own method
* Move db secret setter to utils
* Remove test from test_fairness as test moved to own file

* docs: add description to TrustyAI invalid DB TLS config test

* fix: check TrustyAIService container for Terminated status in lastStatus

* fix: change name of terminal_state getter function

* fix: change to a valid certificate and check for service failure

* fix: address PR 221 reviewer feedback

* revert wait_for_pods to wait_for_mariadb_pods
* improve error checking logic
* remove un-necessary wrapper function

* docs: add docstring to create_trustyai_service method

* docs: add docstring to trustyai_service_with_invalid_db_cert

* fix: fix invalid return type for trustyai_db_ca_secret

* feat: use retry decorator in validate trustyai_service_db_conn_failure method

* fix: remove unnecessary return from validate db_conn_failure method

* docs: add spacing between lines of docstring

* refactor: create constants trustyai metrics and db storage config

* refactor: address reviewer feedback

- change docstring to correct formatting
- remove len(0) check
- no templating for error text

* fix: use regex instead of in operator to check for error condition

* docs: add correct formatting to docstrings

* fix: use namespace.name instead of namespace in Pod.get

* fix: remove \s from regex to check for spaces

* refactor: add Raises section in docstring and use single string for pytest.fail

* feat: use raise instead of pytest.fail

- create new exception TooManyPodsError
- create new exception UnexpectedFailureError
- replace pytest.fail with raise and handle exceptions in retry
-

* fix: change default of teardown to True in TrustyAIService

* docs: correct typo in trustyai docstring

* docs: fix raises in docs and fix formatting

* fix: fix create_trustyai_service namespace args issue

* docs: add default for name arg in create tai svc func

* [model server] Fix runtime request.param name to use external route (opendatahub-io#280)

* fix: fix param name

* fix: fix param name

* feat: add certs when sending requests to TrustyAIService (opendatahub-io#266)

* Wait for pods to be in running state before attempting to create ModelRegistry (opendatahub-io#270)

* on rebase clean commented-by- labels

* Wait for pods to be in running state before attempting to create ModelRegistry

* Address Exception in thread Thread-1 (_monitor) error (opendatahub-io#286)

* chore(deps): lock file maintenance (opendatahub-io#287)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [pre-commit.ci] pre-commit autoupdate (opendatahub-io#292)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.7 → v0.11.8](astral-sh/ruff-pre-commit@v0.11.7...v0.11.8)
- [github.com/gitleaks/gitleaks: v8.24.3 → v8.25.1](gitleaks/gitleaks@v8.24.3...v8.25.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Wait for dsc and dsci ready state in cluster_sanity check (opendatahub-io#293)

* fix(workbenches): implement get_username for OpenShift <=4.14 (opendatahub-io#275)

Turns out SelfSubjectReview is only available starting OpenShift 4.15.

fixup incorporate User resource
* RedHatQE/openshift-python-wrapper#2387

fixup incorporate SelfSubjectReview resource
* RedHatQE/openshift-python-wrapper#2389

Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>

* replace the bot account with one owned by testdevops (opendatahub-io#291)

* Fix for post upgarde operator check (opendatahub-io#297)

Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>

* Add test for Model Registry RBAC for SA token (opendatahub-io#296)

* feat: add RBAC test for SA token

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: address review comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: incorporate coderabbit suggestions

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unneeded variable

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove excessive logs

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* Support /build-push-pr-image comment to push image to quay for testing via jenkins (opendatahub-io#290)

updates! 678b389

* Add tests for model_artifact update validations (opendatahub-io#284)

* Add tests for model_artifact update validations

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updates fixing pre-commit

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* update package

* minor updates

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* address review comments

updates! 50ec24b

updates! f3a6c3e

updates! 792156f

updates! 399aa10

updates! 5080e3b

updates! c34f4e7

updates! a1d7baa

---------

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Jiri Daněk <jdanek@redhat.com>
Co-authored-by: Ruth Netser <rnetser@redhat.com>
Co-authored-by: Luca Giorgi <lgiorgi@redhat.com>
Co-authored-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
Co-authored-by: Edgar Hernández <ehernand@redhat.com>
Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com>
Co-authored-by: Milind Waykole <mwaykole@redhat.com>
Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants