RHOAIENG-33693: Add LlamaStack image validation tests and related fixture#618
Conversation
- Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod.
- Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod.
📝 WalkthroughWalkthroughAdds a new constant Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/wip', '/build-push-pr-image', '/cherry-pick', '/hold', '/verified'} |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
tests/llama_stack/constants.py (2)
17-17: Constant addition looks good; confirm selector correctness and uniquenessPlease verify this label key/value exactly matches the operator Pod labels in your cluster/manifests and that it uniquely identifies a single Pod (rollouts can briefly create >1). If there’s a more specific label available (e.g., component/controller), prefer that to reduce flakiness.
I can provide a quick check script if helpful.
17-17: Prefer “selector” over “filter” for k8s terminologyTo align with Kubernetes terminology and the
label_selectorparam used elsewhere, consider renaming toLLS_OPERATOR_POD_SELECTOR.Apply in this file:
-LLS_OPERATOR_POD_FILTER: str = "app.kubernetes.io/name=llama-stack-k8s-operator" +LLS_OPERATOR_POD_SELECTOR: str = "app.kubernetes.io/name=llama-stack-k8s-operator"…and update imports/usages in the related fixture (see comments there).
tests/llama_stack/image_validation/conftest.py (3)
13-20: Silence Ruff ARG001 and simplify fixture (return vs yield)The
model_namespacearg is intentionally unused for ordering, but Ruff flags it. Also, since there’s no teardown,returnis clearer thanyield.Apply:
-@pytest.fixture(scope="class") -def lls_operator_pod_by_label(admin_client: DynamicClient, model_namespace) -> Generator[Pod, Any, Any]: +@pytest.fixture(scope="class") +def lls_operator_pod_by_label(admin_client: DynamicClient, _model_namespace) -> Generator[Pod, Any, Any]: """Get the LLS operator pod.""" - yield wait_for_pods_by_labels( + return wait_for_pods_by_labels( admin_client=admin_client, namespace=py_config["applications_namespace"], - label_selector=LLS_OPERATOR_POD_FILTER, + label_selector=LLS_OPERATOR_POD_FILTER, expected_num_pods=1, )[0]
8-8: Follow-up if you adopt the “selector” renameIf you rename the constant per the other comment, update the import accordingly.
-from tests.llama_stack.constants import LLS_OPERATOR_POD_FILTER +from tests.llama_stack.constants import LLS_OPERATOR_POD_SELECTOR…and the call site:
- label_selector=LLS_OPERATOR_POD_FILTER, + label_selector=LLS_OPERATOR_POD_SELECTOR,
15-20: Brittleness: expecting exactly one Pod can flake during rollouts
expected_num_pods=1will fail if the controller briefly runs two Pods (e.g., during upgrades). If a more specific label is available, tighten the selector; otherwise, consider handling> 1by choosing the Ready Pod.I can propose a helper that picks the Ready Pod when multiple are returned.
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (5)
38-38: Remove unusedadmin_clientparameterNot used in the test; drop it to silence linters and clarify dependencies.
def test_verify_lls_operator_images( - self: Self, - admin_client: DynamicClient, + self: Self, lls_operator_pod_by_label: Pod, related_images_refs: Set[str], ):Additionally, remove the now-unused import:
-from kubernetes.dynamic import DynamicClient
42-50: Simplify: no need to wrap a single Pod in a listCall the validator once and fail on errors directly.
- validation_errors = [] - for pod in [lls_operator_pod_by_label]: - validation_errors.extend( - validate_container_images( - pod=pod, valid_image_refs=related_images_refs, skip_patterns=["openshift-service-mesh"] - ) - ) - - if validation_errors: - pytest.fail("\n".join(validation_errors)) + errors = validate_container_images( + pod=lls_operator_pod_by_label, + valid_image_refs=related_images_refs, + skip_patterns=["openshift-service-mesh"], + ) + if errors: + pytest.fail("\n".join(errors))
1-3: Drop unused logger
LOGGERisn’t used; remove the import and assignment to reduce noise.-from simple_logger.logger import get_logger @@ -LOGGER = get_logger(name=__name__)Also applies to: 12-12
16-24: Do we needmodel_namespaceparametrization here?The operator Pod is looked up in
py_config["applications_namespace"], not in the parametrizedmodel_namespace. If this namespace isn’t required for LLS operator validation, consider removing the parametrization to speed up tests and reduce setup.
2-2: Optional: avoidtyping.Selffor broader Python compatibilityUnless you’re pinned to 3.11+, consider dropping
Selfto avoid version coupling.-from typing import Self, Set +from typing import Set @@ - self: Self, + self,If you prefer modern types and are on 3.11+, this is fine to keep.
Also applies to: 37-37
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/llama_stack/constants.py(1 hunks)tests/llama_stack/image_validation/conftest.py(1 hunks)tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.pytests/llama_stack/image_validation/conftest.py
🧬 Code graph analysis (2)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (3)
utilities/general.py (1)
validate_container_images(259-294)tests/conftest.py (2)
admin_client(68-69)related_images_refs(603-606)tests/llama_stack/image_validation/conftest.py (1)
lls_operator_pod_by_label(13-20)
tests/llama_stack/image_validation/conftest.py (2)
utilities/general.py (1)
wait_for_pods_by_labels(225-256)tests/conftest.py (2)
admin_client(68-69)model_namespace(100-120)
🪛 Ruff (0.12.2)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py
38-38: Unused method argument: admin_client
(ARG002)
tests/llama_stack/image_validation/conftest.py
13-13: Unused function argument: model_namespace
(ARG001)
- Renamed the LlamaStack distribution fixture for clarity. - Updated the pod label filter constant to reflect the core pod. - Modified the image validation test to use the new pod fixture name.
… RHOAIENG-33693-mr-image-validation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/llama_stack/image_validation/conftest.py (1)
11-19: Singular naming and direct yield (optional).The fixture yields a single Pod; rename to singular to avoid confusion and drop list indexing at call sites.
Apply this diff here:
-@pytest.fixture(scope="class") -def lls_pods(admin_client: DynamicClient, model_namespace) -> Generator[Pod, Any, Any]: +@pytest.fixture(scope="class") +def lls_operator_pod(admin_client: DynamicClient, model_namespace) -> Generator[Pod, Any, Any]: @@ - yield wait_for_pods_by_labels( + yield wait_for_pods_by_labels( @@ - )[0] + )[0]And update test(s) to use lls_operator_pod.
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (4)
3-3: Remove unused LOGGER.Not used; keep the module lean.
-from simple_logger.logger import get_logger @@ -LOGGER = get_logger(name=__name__)Also applies to: 12-12
4-4: Drop unused admin_client param (ruff ARG002).Signature can be simplified; DynamicClient import becomes unnecessary.
-from kubernetes.dynamic import DynamicClient @@ - def test_verify_lls_operator_images( - self: Self, - admin_client: DynamicClient, - lls_pods: Pod, - related_images_refs: Set[str], - ): + def test_verify_lls_operator_images( + self: Self, + lls_pods: Pod, + related_images_refs: Set[str], + ):If you accept the singular fixture rename, also switch lls_pods → lls_operator_pod.
Also applies to: 36-41
2-2: Prefer built‑in set annotations; remove typing.Set import.Modern Python supports set[str].
-from typing import Self, Set +from typing import Self @@ - related_images_refs: Set[str], + related_images_refs: set[str],Also applies to: 40-41
42-49: Simplify iteration over a single pod.No need to build a one‑element list.
- validation_errors = [] - for pod in [lls_pods]: - validation_errors.extend( - validate_container_images( - pod=pod, valid_image_refs=related_images_refs, skip_patterns=["openshift-service-mesh"] - ) - ) + validation_errors = validate_container_images( + pod=lls_pods, + valid_image_refs=related_images_refs, + skip_patterns=["openshift-service-mesh"], + )If you adopt the singular fixture rename, replace lls_pods with lls_operator_pod.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/llama_stack/conftest.py(1 hunks)tests/llama_stack/constants.py(1 hunks)tests/llama_stack/image_validation/conftest.py(1 hunks)tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llama_stack/constants.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py
🧬 Code graph analysis (2)
tests/llama_stack/image_validation/conftest.py (2)
utilities/general.py (1)
wait_for_pods_by_labels(228-259)tests/conftest.py (2)
admin_client(68-69)model_namespace(100-120)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (3)
utilities/general.py (1)
validate_container_images(262-297)tests/conftest.py (2)
admin_client(68-69)related_images_refs(603-606)tests/llama_stack/image_validation/conftest.py (1)
lls_pods(12-19)
🪛 Ruff (0.13.1)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py
38-38: Unused method argument: admin_client
(ARG002)
🔇 Additional comments (2)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (1)
15-33: Confirm test intent: operator vs server images.Name and docstring say “operator images,” but fixture likely returns server pod. Align label selection (preferred) or adjust test name/docstring.
tests/llama_stack/conftest.py (1)
110-110: Fix typo in distribution name ("lama" → "llama").Only occurrence: tests/llama_stack/conftest.py:110. Apply:
- name="test-lama-stack-distribution", + name="test-llama-stack-distribution",
…on-line/opendatahub-tests into RHOAIENG-33693-mr-image-validation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (5)
27-32: Align naming: operator vs core (Distribution) podThe test name says “operator” but the fixture/doc for
lls_pods(andLLS_CORE_POD_FILTER) targets the core/Distribution pod. Align to avoid confusion.Apply this diff to reflect core pod validation:
class TestLLSImages: """ - Tests to verify that LLS (LlamaStack) Distribution image meets the requirements: + Tests to verify that LLS (LlamaStack) core (Distribution) pod images meet the requirements: 1. Images are hosted in registry.redhat.io 2. Images use sha256 digest instead of tags 3. Images are listed in the CSV's relatedImages section """ @pytest.mark.smoke - def test_verify_lls_operator_images( + def test_verify_lls_core_images(If you intended to check the operator Pod instead, please update the label selector used by the fixture to target the operator and keep the original test name.
Also applies to: 35-35
6-7: Remove unused logger
LOGGERis never used here.-from simple_logger.logger import get_logger - - -LOGGER = get_logger(name=__name__) +Also applies to: 11-11
1-2: Drop unused/over-specific typing and unused fixture arg
admin_clientisn’t used (Ruff ARG002); remove it.- Avoid annotating
self.- Prefer built-in generics (
set[str]) and remove unused imports.-from typing import Self, Set - -from kubernetes.dynamic import DynamicClient + @@ - def test_verify_lls_operator_images( - self: Self, - admin_client: DynamicClient, - lls_pods: Pod, - related_images_refs: Set[str], - ): + def test_verify_lls_core_images( + self, + lls_pods: Pod, + related_images_refs: set[str], + ):Also applies to: 3-3, 35-40
38-38: Nit: fixture name suggests plurality but returns a single Pod
lls_podsreturns a singlePod(index[0]in the fixture). Consider renaming the fixture tolls_podfor clarity.
41-44: Replace single-item loop with direct call and skip service-mesh sidecarsvalidate_container_images already accepts skip_patterns — simplify the loop and skip known service-mesh sidecars.
- validation_errors = [] - for pod in [lls_pods]: - validation_errors.extend(validate_container_images(pod=pod, valid_image_refs=related_images_refs)) + validation_errors = validate_container_images( + pod=lls_pods, + valid_image_refs=related_images_refs, + skip_patterns=["openshift-service-mesh"], + )If your environment uses different sidecar names (e.g., Istio’s
proxyv2), extendskip_patternsaccordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/llama_stack/image_validation/conftest.py(1 hunks)tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llama_stack/image_validation/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py
🧬 Code graph analysis (1)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (3)
utilities/general.py (1)
validate_container_images(262-297)tests/conftest.py (2)
admin_client(68-69)related_images_refs(603-606)tests/llama_stack/image_validation/conftest.py (1)
lls_pods(12-19)
🪛 Ruff (0.13.1)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py
37-37: Unused method argument: admin_client
(ARG002)
🔇 Additional comments (2)
tests/llama_stack/image_validation/test_verify_lls_rhoai_images.py (2)
14-23: Fixture usage and namespace parametrization look goodUsing
llama_stack_distributionand indirectmodel_namespaceparametrization is appropriate and keeps the test isolated.
24-26: Markers LGTM
rawdeployment,downstream_only, andsmokefit this test’s intent.
|
Status of building tag latest: success. |
…ture (opendatahub-io#618) * Add LlamaStack image validation tests and related fixture - Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod. * Add LlamaStack image validation tests and related fixture - Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: update LlamaStack test fixtures and constants - Renamed the LlamaStack distribution fixture for clarity. - Updated the pod label filter constant to reflect the core pod. - Modified the image validation test to use the new pod fixture name. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: fix review suggestions * [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>
…ture (opendatahub-io#618) * Add LlamaStack image validation tests and related fixture - Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod. * Add LlamaStack image validation tests and related fixture - Introduced a new fixture to retrieve the LlamaStack operator pod by label. - Added tests to verify that LlamaStack component images meet specified requirements, including image source and format. - Updated constants to include a label filter for the LlamaStack operator pod. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: update LlamaStack test fixtures and constants - Renamed the LlamaStack distribution fixture for clarity. - Updated the pod label filter constant to reflect the core pod. - Modified the image validation test to use the new pod fixture name. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: fix review suggestions * [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>
6 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit