Wait for MC pod before api call#580
Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom Sep 4, 2025
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a wait_for_pods_running synchronization for the model registry namespace in test setup. Updates model catalog utilities: imports wait_for_pods_running, increases default consecutive checks to 6, replaces a polling loop with a pod-creation check plus RUNNING-state wait, renames wait_for_model_catalog_update to wait_for_model_catalog_pod_created. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/lgtm', '/cherry-pick', '/build-push-pr-image', '/wip', '/verified'} |
fege
reviewed
Sep 3, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/conftest.py (2)
12-12: Prefer targeted readiness check over namespace-wide pod wait
wait_for_pods_runningwaits for all pods in the namespace, which can add flakiness/latency unrelated to Model Catalog. Sinceis_model_catalog_readyis already imported and scoped to MC, use it instead and drop this import.Apply:
-from tests.model_registry.utils import wait_for_pods_running
32-35: Use MC-scoped readiness check; class-scoped wait may miss inter-test pod churnThe current wait targets all namespace pods; switch to MC-specific readiness for precision. Also, because this fixture is class-scoped, if pods are torn down between test methods, the once-per-class wait may be insufficient—consider a function-scoped readiness helper for tests that issue API calls.
Apply:
- # ensure that model catalog pod is up and running: - wait_for_pods_running( - admin_client=admin_client, namespace_name=model_registry_namespace, number_of_consecutive_checks=3 - ) + # Ensure Model Catalog pods are ready before constructing URLs + is_model_catalog_ready(client=admin_client, model_registry_namespace=model_registry_namespace)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/model_registry/model_catalog/conftest.py(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/utils.py:14-32
Timestamp: 2025-08-08T16:00:51.421Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job, keep get_latest_job_pod (tests/model_registry/async_job/utils.py) simple: sorting by creationTimestamp string and selecting pods via label_selector="job-name=<job.name>" is acceptable; do not add datetime parsing or controller-uid filtering unless a concrete failure arises (per guidance from user lugi0).
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-07-17T15:42:04.167Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Applied to files:
tests/model_registry/model_catalog/conftest.py
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/conftest.py (3)
tests/model_registry/utils.py (1)
wait_for_pods_running(249-282)tests/conftest.py (1)
admin_client(68-69)tests/model_registry/conftest.py (1)
model_registry_namespace(63-64)
🔇 Additional comments (1)
tests/model_registry/model_catalog/conftest.py (1)
28-30: Signature change approved — no external imports or direct calls ofmodel_catalog_rest_urlfound.
sheltoncyril
previously approved these changes
Sep 3, 2025
Contributor
|
/lgtm |
rhods-ci-bot
previously approved these changes
Sep 3, 2025
dbasunag
dismissed stale reviews from rhods-ci-bot and sheltoncyril
via
7eef321
dbasunag
force-pushed
the
wait_for_mc_pod
branch
from
f976fa3 to
7eef321
Compare
dbasunag
force-pushed
the
wait_for_mc_pod
branch
from
a189a81 to
f0cfc6c
Compare
dbasunag
force-pushed
the
wait_for_mc_pod
branch
from
f0cfc6c to
c0f0b31
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/utils.py (2)
57-57: Make pod deletion deterministicDeletion is async; add wait=True so the follow-up creation check doesn’t race the terminating pod.
Apply:
- pod.delete() + pod.delete(wait=True)
59-62: Optional: wait specifically for MC pods, not the whole namespaceTo avoid coupling readiness to unrelated pods in the namespace, consider waiting by label (component=model-catalog) instead of namespace-wide. This can reduce flakiness and shorten waits.
If you choose this, import the helper and switch the call:
- from tests.model_registry.utils import get_model_catalog_pod, wait_for_pods_running + from tests.model_registry.utils import get_model_catalog_pod, wait_for_pods_running + from utilities.general import wait_for_pods_by_labels- wait_for_pods_running( - admin_client=client, namespace_name=model_registry_namespace, number_of_consecutive_checks=consecutive_try - ) + wait_for_pods_by_labels( + admin_client=client, + namespace=model_registry_namespace, + label_selector="component=model-catalog", + expected_num_pods=1, + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/model_registry/conftest.py(1 hunks)tests/model_registry/model_catalog/utils.py(2 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: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/conftest.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/utils.py
📚 Learning: 2025-07-09T08:50:22.172Z
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.
Applied to files:
tests/model_registry/model_catalog/utils.py
🧬 Code graph analysis (2)
tests/model_registry/conftest.py (1)
tests/model_registry/utils.py (1)
wait_for_pods_running(249-282)
tests/model_registry/model_catalog/utils.py (1)
tests/model_registry/utils.py (2)
get_model_catalog_pod(728-731)wait_for_pods_running(249-282)
🔇 Additional comments (3)
tests/model_registry/model_catalog/utils.py (3)
18-18: Import addition looks goodImporting wait_for_pods_running aligns with the new readiness flow.
52-52: Bumping consecutive_try to 6 is reasonableMatches the stabilization approach used elsewhere in this PR.
66-71: Raise a retriable exception to trigger @Retry and extend timeout
The retry decorator won’t retry on a False return, so change the function to raiseResourceNotFoundErrorwhen no pod is found and increasewait_timeoutto 60 s:-@retry(wait_timeout=30, sleep=5) -def wait_for_model_catalog_pod_created(client: DynamicClient, model_registry_namespace: str) -> bool: - pods = get_model_catalog_pod(client=client, model_registry_namespace=model_registry_namespace) - if pods: - return True - return False +@retry(wait_timeout=60, sleep=5, exceptions_dict={ResourceNotFoundError: []}) +def wait_for_model_catalog_pod_created(client: DynamicClient, model_registry_namespace: str) -> bool: + pods = get_model_catalog_pod(client=client, model_registry_namespace=model_registry_namespace) + if not pods: + raise ResourceNotFoundError("model-catalog pod not created yet") + return TrueVerify that your retry decorator supports the
exceptions_dictparameter and will retry onResourceNotFoundError.
dbasunag
force-pushed
the
wait_for_mc_pod
branch
from
c0f0b31 to
a9afd4a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/model_registry/model_catalog/utils.py (3)
52-52: Add explicit return annotation for clarityThis helper doesn’t return a value; add -> None.
-def is_model_catalog_ready(client: DynamicClient, model_registry_namespace: str, consecutive_try: int = 6): +def is_model_catalog_ready(client: DynamicClient, model_registry_namespace: str, consecutive_try: int = 6) -> None:
65-69: Scope exception to MC pods or enrich contextConsider either renaming to ModelCatalogPodNotFound or ensure raised messages include namespace and label to aid triage.
30-33: Retry on transient network errors tooConnectionRefused/Timeout from requests won’t be retried. Include RequestException to make wait_for_model_catalog_api robust while MC starts.
-@retry(wait_timeout=60, sleep=5, exceptions_dict={ResourceNotFoundError: []}) +@retry(wait_timeout=60, sleep=5, exceptions_dict={ResourceNotFoundError: [], requests.exceptions.RequestException: []}) def wait_for_model_catalog_api(url: str, headers: dict[str, str], verify: bool | str = False) -> requests.Response: return _execute_get_call(url=f"{url}sources", headers=headers, verify=verify)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/model_registry/conftest.py(1 hunks)tests/model_registry/model_catalog/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/conftest.py
🧰 Additional context used
🧠 Learnings (3)
📓 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: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
📚 Learning: 2025-07-09T08:50:22.172Z
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.
Applied to files:
tests/model_registry/model_catalog/utils.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/utils.py
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/utils.py (1)
tests/model_registry/utils.py (2)
get_model_catalog_pod(728-731)wait_for_pods_running(249-282)
🔇 Additional comments (2)
tests/model_registry/model_catalog/utils.py (2)
18-18: LGTM: import of wait_for_pods_runningImport is used below; no issues.
52-63: Negative tests don’t use Running-state helpers
Verified no calls tois_model_catalog_readyorwait_for_pods_runningin tests/model_registry/negative_tests; negative DB migration tests won’t hang waiting for pods.
adolfo-ab
approved these changes
Sep 4, 2025
|
Status of building tag latest: success. |
mwaykole
pushed a commit
to mwaykole/opendatahub-tests
that referenced
this pull request
Jan 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Between tests we have to kill pods. And we should not try the apis when the pods are not in running state.
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit