Add first pre/post upgrade skeleton for Model Registry#371
Add first pre/post upgrade skeleton for Model Registry#371lugi0 merged 15 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: lugi0 <lgiorgi@redhat.com>
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change refactors model registry test fixtures to support a new post-upgrade testing mode, introducing pre/post upgrade fixtures for DataScienceCluster component state management. It also adds a new upgrade test module with pre- and post-upgrade validation tests, updates fixture signatures, and removes an obsolete OAuth-related fixture. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🪛 Pylint (3.3.7)tests/model_registry/conftest.py[refactor] 155-155: Too many arguments (8/5) (R0913) [refactor] 155-155: Too many positional arguments (8/5) (R0917) [refactor] 192-192: Too many arguments (6/5) (R0913) [refactor] 192-192: Too many positional arguments (6/5) (R0917) [refactor] 391-391: Either all return statements in a function should return an expression, or none of them should. (R1710) 🔇 Additional comments (9)
✨ 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
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/build-push-pr-image', '/wip', '/hold', '/cherry-pick', '/verified'} |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
42-44: Consider validating more model attributes.Currently, only the
nameattribute is validated. For a comprehensive post-upgrade test, consider validating additional attributes like URI, version, description, etc., to ensure all model data persists correctly across upgrades.tests/model_registry/conftest.py (2)
402-421: Remove unnecessaryelseblock afterreturn.The
elseblock is unnecessary since the function returns in theifbranch.def pre_upgrade_dsc_patch( dsc_resource: DataScienceCluster, admin_client: DynamicClient, ) -> DataScienceCluster: original_components = dsc_resource.instance.spec.components component_patch = {DscComponents.MODELREGISTRY: {"managementState": DscComponents.ManagementState.MANAGED}} if ( original_components.get(DscComponents.MODELREGISTRY).get("managementState") == DscComponents.ManagementState.MANAGED ): LOGGER.error("Model Registry is already set to Managed before upgrade - was this intentional?") return dsc_resource - else: - editor = ResourceEditor(patches={dsc_resource: {"spec": {"components": component_patch}}}) - editor.update() - dsc_resource.wait_for_condition(condition=DscComponents.COMPONENT_MAPPING["modelregistry"], status="True") - namespace = Namespace( - name=dsc_resource.instance.spec.components.modelregistry.registriesNamespace, ensure_exists=True - ) - namespace.wait_for_status(status=Namespace.Status.ACTIVE) - wait_for_pods_running( - admin_client=admin_client, - namespace_name=py_config["applications_namespace"], - number_of_consecutive_checks=6, - ) - return dsc_resource + editor = ResourceEditor(patches={dsc_resource: {"spec": {"components": component_patch}}}) + editor.update() + dsc_resource.wait_for_condition(condition=DscComponents.COMPONENT_MAPPING["modelregistry"], status="True") + namespace = Namespace( + name=dsc_resource.instance.spec.components.modelregistry.registriesNamespace, ensure_exists=True + ) + namespace.wait_for_status(status=Namespace.Status.ACTIVE) + wait_for_pods_running( + admin_client=admin_client, + namespace_name=py_config["applications_namespace"], + number_of_consecutive_checks=6, + ) + return dsc_resource
452-454: Remove commented-out parameters.These commented parameters should be removed as they add clutter without providing value.
@pytest.fixture(scope="class") def model_registry_client( - # pytestconfig: Config, - # admin_client: DynamicClient, - # dsc_resource: DataScienceCluster, current_client_token: str, model_registry_instance_rest_endpoint: str, ) -> ModelRegistryClient:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/model_registry/upgrade/test_model_registry_upgrade.py
[refactor] 21-21: Too few public methods (1/2)
(R0903)
[refactor] 35-35: Too few public methods (1/2)
(R0903)
tests/model_registry/conftest.py
[refactor] 166-166: Too many arguments (7/5)
(R0913)
[refactor] 166-166: Too many positional arguments (7/5)
(R0917)
[refactor] 402-421: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (3)
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
41-43: ```shell
#!/bin/bashLocate any constants files
echo "Searching for constants files..."
find . -type f -iname '*constants.py'Search within located constants files for MODEL_NAME and MODEL_DICT
for file in $(find . -type f -iname '*constants.py'); do
echo -e "\nChecking $file for definitions:"
rg -n '^ *MODEL_NAME' "$file" || echo "No MODEL_NAME found in $file"
rg -n '^ *MODEL_DICT' "$file" || echo "No MODEL_DICT found in $file"
doneAs a fallback, search all tests for any MODEL_NAME and MODEL_DICT usages
echo -e "\nSearching all tests for MODEL_NAME and MODEL_DICT usages..."
rg -n "MODEL_NAME" tests || echo "No MODEL_NAME usages in tests"
rg -n "MODEL_DICT" tests || echo "No MODEL_DICT usages in tests"</details> <details> <summary>tests/model_registry/conftest.py (2)</summary> `78-78`: **Consider the implications of explicit resource deletion in post-upgrade mode.** All resources are explicitly deleted with `wait=True` after the fixture yields in post-upgrade mode. This could be problematic if: - Other tests or processes depend on these resources - The deletion fails and blocks test execution - Resources are meant to persist beyond the test session Consider using the teardown mechanism consistently or documenting why explicit deletion is necessary in post-upgrade scenarios. Also applies to: 123-123, 151-151, 184-184, 225-225 --- `265-265`: **Good simplification of the MySQL config fixture.** Using the constants directly instead of reading from the secret resource reduces coupling and simplifies the fixture. Also applies to: 269-269 </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: lugi0 <lgiorgi@redhat.com>
|
/build-push-pr-image |
|
Status of building tag pr-371: success. |
|
/build-push-pr-image |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/model_registry/upgrade/test_model_registry_upgrade.py (2)
37-38: Consider expanding the TODO for better clarityThe TODO comment mentions testing v1alpha1 to v1beta1 conversion, but this functionality is actually implemented in
test_model_registry_instance_status_conversion_post_upgrade. Consider updating this TODO to reflect what's still needed or remove it if the conversion testing is complete.- # TODO: if we are in <=2.21, we can create a servicemesh MR here instead of oauth (v1alpha1), and then in - # post-upgrade check that it automatically gets converted to oauth (v1beta1) - to be done in 2.21 branch directly. + # TODO: Add servicemesh MR creation for <=2.21 versions to test automatic conversion to oauth (v1beta1) post-upgrade
76-87: Consider adding error context to the status validationThe test validates the v1alpha1 status conversion, but the failure message could be more descriptive about what constitutes a valid status.
if not status: - pytest.fail(f"Empty status found for {mr_instance}") + pytest.fail(f"Empty status found for {mr_instance}. Expected status to contain conversion information or other relevant fields.")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/upgrade/test_model_registry_upgrade.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/model_registry/upgrade/test_model_registry_upgrade.py
[refactor] 24-24: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tox-tests
🔇 Additional comments (6)
tests/model_registry/upgrade/test_model_registry_upgrade.py (6)
1-11: LGTM: Clean imports and setupThe imports are well-organized and all necessary dependencies are properly imported. The logger setup follows good practices.
14-22: Good use of parametrized fixtures with indirect modeThe parametrization with
indirect=Trueallows theregistered_modelfixture to receive theMODEL_DICTparameter, enabling flexible test data configuration.
26-35: Excellent improvement from previous empty test bodyThe test now properly validates the registered model using the utility function
get_and_validate_registered_model. This addresses the past review concern about the empty test body and provides meaningful validation.
44-54: Good consistency in validation approachThe post-upgrade test correctly uses the same utility function for validation, maintaining consistency with the pre-upgrade test while appropriately omitting the
registered_modelparameter since it's validating retrieval rather than comparing against a fixture.
57-64: Solid API version validation for 2.22+ compatibilityThe test correctly validates the expected API version format and the comment appropriately indicates the version compatibility requirement, which aligns with the PR objectives.
67-73: Clear spec validation with meaningful assertionsThe test properly validates key spec fields (istio disabled, oauthProxy.serviceRoute enabled) that are important for post-upgrade verification.
for more information, see https://pre-commit.ci
|
Status of building tag pr-371: success. |
|
/build-push-pr-image |
|
Status of building tag pr-371: success. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/conftest.py (1)
391-415: Fix inconsistent return statement pattern.The fixture has an inconsistent return pattern - it returns
dsc_resourcein the success path but has no return in the failure path afterpytest.fail(). Whilepytest.fail()raises an exception, maintaining consistency is important for code clarity.- if ( - original_components.get(DscComponents.MODELREGISTRY).get("managementState") - == DscComponents.ManagementState.MANAGED - ): - pytest.fail("Model Registry is already set to Managed before upgrade - was this intentional?") - else: - editor = ResourceEditor(patches={dsc_resource: {"spec": {"components": component_patch}}}) - editor.update() - dsc_resource.wait_for_condition(condition=DscComponents.COMPONENT_MAPPING["modelregistry"], status="True") - namespace = Namespace( - name=dsc_resource.instance.spec.components.modelregistry.registriesNamespace, ensure_exists=True - ) - namespace.wait_for_status(status=Namespace.Status.ACTIVE) - wait_for_pods_running( - admin_client=admin_client, - namespace_name=py_config["applications_namespace"], - number_of_consecutive_checks=6, - ) - return dsc_resource + if ( + original_components.get(DscComponents.MODELREGISTRY).get("managementState") + == DscComponents.ManagementState.MANAGED + ): + pytest.fail("Model Registry is already set to Managed before upgrade - was this intentional?") + + editor = ResourceEditor(patches={dsc_resource: {"spec": {"components": component_patch}}}) + editor.update() + dsc_resource.wait_for_condition(condition=DscComponents.COMPONENT_MAPPING["modelregistry"], status="True") + namespace = Namespace( + name=dsc_resource.instance.spec.components.modelregistry.registriesNamespace, ensure_exists=True + ) + namespace.wait_for_status(status=Namespace.Status.ACTIVE) + wait_for_pods_running( + admin_client=admin_client, + namespace_name=py_config["applications_namespace"], + number_of_consecutive_checks=6, + ) + return dsc_resource
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/conftest.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/model_registry/conftest.py
206-206: Local variable iistio_config is assigned to but never used
Remove assignment to unused variable iistio_config
(F841)
🪛 Flake8 (7.2.0)
tests/model_registry/conftest.py
[error] 206-206: local variable 'iistio_config' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/model_registry/conftest.py
[refactor] 155-155: Too many arguments (8/5)
(R0913)
[refactor] 155-155: Too many positional arguments (8/5)
(R0917)
[refactor] 192-192: Too many arguments (6/5)
(R0913)
[refactor] 192-192: Too many positional arguments (6/5)
(R0917)
[error] 222-222: Possibly using variable 'istio_config' before assignment
(E0606)
[refactor] 391-391: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🔇 Additional comments (4)
tests/model_registry/conftest.py (4)
64-99: Post-upgrade logic implementation looks correct.The conditional logic for handling post-upgrade mode versus normal resource creation is well implemented. The fixture properly retrieves existing resources with
ensure_exists=Trueand explicitly deletes them during teardown in post-upgrade mode.
256-260: Good improvement using constants instead of secret data.Using static constants from
MODEL_REGISTRY_DB_SECRET_STR_DATAinstead of reading from the secret instance makes the configuration more reliable and predictable, especially in upgrade scenarios.
419-441: Well-designed post-upgrade teardown fixture.The fixture properly implements the generator pattern with immediate yield for setup and teardown logic afterward. The state validation and cleanup logic for setting ModelRegistry to REMOVED and deleting the namespace is well structured.
505-506: Good addition of missing fixture.This fixture addresses the missing
is_model_registry_oauthdependency that was flagged in multiple past review comments. The implementation correctly extracts the OAuth proxy setting from request parameters with a sensible default.
Signed-off-by: lugi0 <lgiorgi@redhat.com>
|
/build-push-pr-image |
|
Status of building tag pr-371: success. |
|
Status of building tag latest: success. |
4 participants
Description
This draft PR adds a skeleton for pre and post upgrade tests for Model Registry. They currently don't do anything too interesting (register a model in pre-upgrade, retrieve it in post), but they implement the logic needed to keep the environment we set up prior to the upgrade and tear it down after the upgrade.
Due to the current implementation of our fixtures it needed a fair amount of refactoring and additional checks needed to be implemented in order to not try to create duplicate resources; this might not be the best approach overall but it's the only solution I could come up with within our constraints.
How Has This Been Tested?
Tested it locally (without triggering a RHOAI upgrade in between) to check the validity of the setup/teardown logic between the two scenarios.
Merge criteria: