change: wait for container to be in the wanted status#426
Merged
dbasunag merged 8 commits intoopendatahub-io:mainfrom Jul 15, 2025
Merged
change: wait for container to be in the wanted status#426dbasunag merged 8 commits intoopendatahub-io:mainfrom
dbasunag merged 8 commits intoopendatahub-io:mainfrom
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new utility function was introduced to wait for a container within a pod to reach a specific status, specifically "CRASH_LOOPBACK_OFF". This function is now used in a negative test for database migration, ensuring the test waits for the pod to crash before inspecting logs for errors. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 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)
Other keywords and placeholders
Documentation and Community
|
Contributor
Author
|
/verified |
![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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/negative_tests/test_db_migration.py(2 hunks)utilities/general.py(2 hunks)
🧰 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.
utilities/general.py (1)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: utilities/user_utils.py:71-76
Timestamp: 2025-06-17T00:29:48.834Z
Learning: In utilities/user_utils.py, the wait_for_user_creation function's retry decorator is intentionally configured to only catch ExceptionUserLogin, not ClusterLoginError. The function catches the result of login_with_user_password and raises ExceptionUserLogin based on the boolean return value, which is the intended retry pattern.
tests/model_registry/negative_tests/test_db_migration.py (6)
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: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:257-264
Timestamp: 2025-06-18T08:27:21.114Z
Learning: In tests/model_registry/rest_api/conftest.py, the user fege prefers using next() without a default value to raise StopIteration when the mysql container is not found, rather than gracefully handling it with None checks. This is a deliberate "fail fast" design choice.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
🧬 Code Graph Analysis (2)
utilities/general.py (2)
utilities/constants.py (2)
KServeDeploymentType(6-9)Timeout(209-220)utilities/exceptions.py (2)
UnexpectedResourceCountError(121-122)ResourceValueMismatch(125-128)
tests/model_registry/negative_tests/test_db_migration.py (1)
utilities/general.py (1)
wait_for_container_status(337-357)
🔇 Additional comments (3)
utilities/general.py (1)
14-15: LGTM: Import additions are appropriate.The new imports support the utility function being added.
tests/model_registry/negative_tests/test_db_migration.py (2)
10-10: LGTM: Import addition is appropriate.The new import supports the test enhancement.
55-56: Confirm CrashLoopBackOff status constant in Pod.StatusSince
Podis defined in the externalocp_resourcespackage (not in this repo), we weren’t able to locateCRASH_LOOPBACK_OFFlocally. Please verify in your environment thatPod.Status.CRASH_LOOPBACK_OFFexists and corresponds to Kubernetes’ “CrashLoopBackOff” reason. For example:from ocp_resources.pod import Pod print([s for s in dir(Pod.Status) if "CRASH" in s])• File: tests/model_registry/negative_tests/test_db_migration.py
• Lines: 55–56
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/cherry-pick', '/build-push-pr-image', '/lgtm', '/hold', '/wip'} |
lugi0
previously approved these changes
Jul 14, 2025
Contributor
lugi0
left a comment
lugi0
left a comment
There was a problem hiding this comment.
LGTM but please double check if the code rabbit suggestion is needed
rhods-ci-bot
added
commented-by-fege
and removed
lgtm-by-lugi0
commented-by-dbasunag
labels
Contributor
Author
|
/verified |
dbasunag
reviewed
Jul 14, 2025
dbasunag
approved these changes
Jul 14, 2025
|
Status of building tag pr-426: success. |
Collaborator
|
/build-push-pr-image |
dbasunag
previously approved these changes
Jul 15, 2025
|
Status of building tag pr-426: success. |
…o#414) * Add ONNX test case with config files (gRPC/REST templates, conftest, constants) and supporting utils * Incorporate review comments Signed-off-by: Vaibhav Jain <vajain@redhat.com> * Fix pre-commit errors Signed-off-by: Vaibhav Jain <vajain@redhat.com> * Incorporate review comments Signed-off-by: Vaibhav Jain <vajain@redhat.com> --------- Signed-off-by: Vaibhav Jain <vajain@redhat.com> Co-authored-by: Vaibhav Jain <vajain@redhat.com> Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
Contributor
Author
|
/verified |
dbasunag
approved these changes
Jul 15, 2025
|
Status of building tag latest: success. |
3 tasks
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
6 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.
Description
How Has This Been Tested?
Merge criteria: