Skip to content

Add wait for minio pod and update images to multi-arch#643

Merged
lugi0 merged 2 commits intoopendatahub-io:mainfrom
dbasunag:multi_arch
Sep 26, 2025
Merged

Add wait for minio pod and update images to multi-arch#643
lugi0 merged 2 commits intoopendatahub-io:mainfrom
dbasunag:multi_arch

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Sep 25, 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

    • None
  • Chores

    • Updated OCI registry image to a newer version.
  • Refactor

    • Centralized and standardized MinIO configuration with unified labels/annotations.
  • Tests

    • Added explicit wait so MinIO is Running before tests proceed to reduce flakiness.
    • Updated async upload test to use the model-registry MinIO configuration.

@dbasunag dbasunag requested review from a team, fege and lugi0 as code owners September 25, 2025 18:15
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 25, 2025

📝 Walkthrough

Walkthrough

Wait for MinIO pod to reach Pod.Status.RUNNING in test fixture, switch a test parametrization to use MODEL_REGISTRY_MINIO_CONFIG, and refactor MinIO-related constants while updating the OCI registry image reference.

Changes

Cohort / File(s) Summary of Changes
Test fixture synchronization
tests/conftest.py
Added an explicit wait for the created MinIO pod to reach Pod.Status.RUNNING before yielding from the fixture.
Test parametrization change
tests/model_registry/async_job/test_async_upload_e2e.py
Replaced pytest param MinIo.PodConfig.MODEL_MESH_MINIO_CONFIG with MinIo.PodConfig.MODEL_REGISTRY_MINIO_CONFIG.
Constants refactor & image update
utilities/constants.py
Renamed/introduced MINIO_BASE_LABELS_ANNOTATIONS, added a new MINIO_BASE_CONFIG that merges labels/annotations and includes args, added MODEL_REGISTRY_MINIO_CONFIG, and updated OCI registry image from ghcr.io/project-zot/zot-linux-amd64:v2.1.7 to ghcr.io/project-zot/zot:v2.1.8.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly highlights the two primary updates in this PR: adding an explicit wait for the MinIO pod to reach running status and switching image tags to a multi-architecture variant, which aligns precisely with the changes in the test fixtures and constants.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 758917c and 0006a88.

📒 Files selected for processing (3)
  • tests/conftest.py (1 hunks)
  • tests/model_registry/async_job/test_async_upload_e2e.py (1 hunks)
  • utilities/constants.py (4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
📚 Learning: 2025-08-08T15:59:16.170Z
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.

Applied to files:

  • tests/model_registry/async_job/test_async_upload_e2e.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).

Applied to files:

  • tests/model_registry/async_job/test_async_upload_e2e.py
📚 Learning: 2025-08-11T07:34:44.352Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/utils.py:34-36
Timestamp: 2025-08-11T07:34:44.352Z
Learning: In the opendatahub-tests repository, container images should use digest references (e.g., `imagesha256:...`) instead of version tags like `:latest`, especially for disconnected environments where specific digests are required for image mirroring. This applies to test fixtures and deployment configurations.

Applied to files:

  • utilities/constants.py
🧬 Code graph analysis (1)
tests/model_registry/async_job/test_async_upload_e2e.py (1)
utilities/constants.py (3)
  • MinIo (273-342)
  • PodConfig (256-263)
  • PodConfig (289-338)
🪛 Ruff (0.13.1)
utilities/constants.py

294-301: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


303-306: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


334-338: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🔇 Additional comments (2)
tests/conftest.py (1)

520-521: Thanks for hardening the MinIO fixture

Explicitly waiting for the pod to reach Running eliminates the intermittent race we kept seeing in async job tests. 👍

tests/model_registry/async_job/test_async_upload_e2e.py (1)

35-38: Good call on using the model-registry MinIO profile

Routing this test through MODEL_REGISTRY_MINIO_CONFIG aligns it with the new multi-arch MinIO image and the fixture defaults. Looks solid.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • 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.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/hold', '/wip', '/cherry-pick', '/verified', '/lgtm', '/build-push-pr-image'}

@dbasunag
Copy link
Copy Markdown
Collaborator Author

/build-push-pr-image

@github-actions
Copy link
Copy Markdown

Status of building tag pr-643: success.
Status of pushing tag pr-643 to image registry: success.

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

🧹 Nitpick comments (1)
tests/model_registry/async_job/test_async_upload_e2e.py (1)

135-135: Update the completion log message

We now run with MODEL_REGISTRY_MINIO_CONFIG, but the log still claims we’re using the KSERVE image. Please align the message with the new config so the test output stays accurate.

-        LOGGER.info("Async upload job test with KSERVE_MINIO_IMAGE: PASSED")
+        LOGGER.info("Async upload job test with MODEL_REGISTRY_MINIO_CONFIG: PASSED")
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6265ddf and 758917c.

📒 Files selected for processing (3)
  • tests/conftest.py (1 hunks)
  • tests/model_registry/async_job/test_async_upload_e2e.py (1 hunks)
  • utilities/constants.py (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-08T15:59:16.170Z
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.

Applied to files:

  • tests/model_registry/async_job/test_async_upload_e2e.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).

Applied to files:

  • tests/model_registry/async_job/test_async_upload_e2e.py
🧬 Code graph analysis (1)
tests/model_registry/async_job/test_async_upload_e2e.py (1)
utilities/constants.py (3)
  • MinIo (273-342)
  • PodConfig (256-263)
  • PodConfig (289-338)
🪛 Ruff (0.13.1)
utilities/constants.py

294-301: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


303-306: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


334-338: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🔇 Additional comments (1)
tests/conftest.py (1)

520-521: Explicit MinIO readiness wait looks good

Adding the wait_for_status(Pod.Status.RUNNING) call ensures downstream fixtures don’t race against pod startup. 👍

Comment thread utilities/constants.py Outdated
sheltoncyril
sheltoncyril previously approved these changes Sep 25, 2025
Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril 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
Copy link
Copy Markdown
Collaborator Author

/build-push-pr-image

@github-actions
Copy link
Copy Markdown

Status of building tag pr-643: success.
Status of pushing tag pr-643 to image registry: success.

@sheltoncyril sheltoncyril enabled auto-merge (squash) September 25, 2025 21:44
@sheltoncyril
Copy link
Copy Markdown
Contributor

/lgtm

@lugi0
Copy link
Copy Markdown
Contributor

lugi0 commented Sep 26, 2025

Tested through Jenkins and works fine, merging

Copy link
Copy Markdown
Contributor

@kpunwatk kpunwatk left a comment

Choose a reason for hiding this comment

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

/lgtm

@lugi0 lugi0 merged commit 8a1c676 into opendatahub-io:main Sep 26, 2025
9 checks passed
@github-actions
Copy link
Copy Markdown

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

sheltoncyril pushed a commit to sheltoncyril/opendatahub-tests that referenced this pull request Oct 2, 2025
mwaykole pushed a commit to mwaykole/opendatahub-tests that referenced this pull request Jan 23, 2026
@dbasunag dbasunag deleted the multi_arch branch March 19, 2026 00:06
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.

5 participants