Add tests to verify pull secrets are updated correctly for rawdeployment#281
Conversation
WalkthroughThis update introduces and tests support for managing image pull secrets in Kubernetes inference services. New constants for original and updated pull secrets are defined. Pytest fixtures are added to set up inference services with, update, and remove image pull secrets. A utility function is introduced to verify the presence or absence of image pull secrets in inference service pods. Tests are implemented to check the correct setting, updating, and removal of pull secrets. Additionally, the inference service creation utility is updated to accept image pull secrets as an optional parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Fixture as Pytest Fixture
participant ISVC as InferenceService
participant Utils as verify_pull_secret
participant K8s as Kubernetes Pod
Test->>Fixture: Request ISVC with pull secret
Fixture->>ISVC: Create/Update ISVC (with/without pull secret)
ISVC->>K8s: Deploy pod with imagePullSecrets
Test->>Utils: verify_pull_secret(ISVC, pull_secret, secret_exists)
Utils->>K8s: Retrieve pod spec
Utils-->>Test: Assert presence/absence of pull secret
Poem
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/hold', '/lgtm', '/verified'} |
VedantMahabaleshwarkar
force-pushed
the
j-19717
branch
from
a217cc8 to
c8a1c79
Compare
VedantMahabaleshwarkar
changed the title
![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_serving/model_server/inference_service_configuration/constants.py (1)
14-15: Pull secret constants for testingThese constants provide test values for the original and updated pull secrets. The
# pragma: allowlist-secretcomments appropriately prevent false positives in security scans.Consider using UPPER_SNAKE_CASE naming convention for consistency with other constants in this file:
-original_pull_secret = "pull-secret-1" # pragma: allowlist-secret -updated_pull_secret = "updated-pull-secret" # pragma: allowlist-secret +ORIGINAL_PULL_SECRET = "pull-secret-1" # pragma: allowlist-secret +UPDATED_PULL_SECRET = "updated-pull-secret" # pragma: allowlist-secrettests/model_serving/model_server/inference_service_configuration/utils.py (1)
113-139: Well-implemented verification utility for pull secretsThis function provides a clean way to verify that pull secrets are correctly applied to inference services. The assertions have helpful error messages to aid debugging.
Consider handling the case where no pods are found and verifying all pods instead of just the first one:
def verify_pull_secret(isvc: InferenceService, pull_secret: str, secret_exists: bool) -> None: """ Verify that the ImagePullSecret in the InferenceService pods match the expected values. Args: isvc (InferenceService): InferenceService object. pull_secret (str): Pull secret to verify secret_exists (bool): False if the pull secret should not exist in the pod. Raises: AssertionError: If the imagePullSecrets do not match the expected presence or name. """ - pod = get_pods_by_isvc_label( + pods = get_pods_by_isvc_label( client=isvc.client, isvc=isvc, - )[0] - image_pull_secrets = pod.instance.spec.imagePullSecrets or [] - - secrets = [s.name for s in image_pull_secrets] - - if secret_exists: - assert secrets, "Expected imagePullSecrets to exist, but none were found." - assert pull_secret in secrets, f"Expected pull secret '{pull_secret}' not found in imagePullSecrets: {secrets}" - else: - assert pull_secret not in secrets, ( - f"Did not expect pull secret '{pull_secret}', but found in imagePullSecrets: {secrets}" - ) + ) + + assert pods, f"No pods found for inference service {isvc.name}" + + for pod in pods: + image_pull_secrets = pod.instance.spec.imagePullSecrets or [] + secrets = [s.name for s in image_pull_secrets] + + if secret_exists: + assert secrets, f"Expected imagePullSecrets to exist in pod {pod.name}, but none were found." + assert pull_secret in secrets, f"Expected pull secret '{pull_secret}' not found in pod {pod.name} imagePullSecrets: {secrets}" + else: + assert pull_secret not in secrets, ( + f"Did not expect pull secret '{pull_secret}', but found in pod {pod.name} imagePullSecrets: {secrets}" + )tests/model_serving/model_server/inference_service_configuration/test_isvc_pull_secret_updates.py (1)
40-41: Add a docstring for consistencyThe test correctly verifies the removal of pull secrets, but is missing a docstring unlike the other tests.
Add a docstring for consistency with the other test methods:
def test_remove_pull_secret(self, updated_isvc_remove_pull_secret): + """Verify that removing the pull secret is correctly reflected in the pod""" verify_pull_secret(isvc=updated_isvc_remove_pull_secret, pull_secret=updated_pull_secret, secret_exists=False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_serving/model_server/inference_service_configuration/conftest.py(2 hunks)tests/model_serving/model_server/inference_service_configuration/constants.py(1 hunks)tests/model_serving/model_server/inference_service_configuration/test_isvc_pull_secret_updates.py(1 hunks)tests/model_serving/model_server/inference_service_configuration/utils.py(1 hunks)utilities/inference_utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_serving/model_server/inference_service_configuration/utils.py (1)
utilities/infra.py (1)
get_pods_by_isvc_label(445-472)
tests/model_serving/model_server/inference_service_configuration/test_isvc_pull_secret_updates.py (3)
tests/model_serving/model_server/inference_service_configuration/utils.py (1)
verify_pull_secret(113-139)utilities/constants.py (3)
ModelFormat(12-19)ModelName(22-27)RuntimeTemplates(65-73)tests/model_serving/model_server/inference_service_configuration/conftest.py (3)
model_car_raw_inference_service_with_pull_secret(78-95)updated_isvc_pull_secret(99-109)updated_isvc_remove_pull_secret(113-129)
🔇 Additional comments (9)
utilities/inference_utils.py (2)
537-537: Good enhancement to support image pull secretsThis new parameter allows specifying image pull secrets when creating inference services. The type hint clearly indicates the expected format and makes it optional.
612-613: Correctly transforms pull secrets for KubernetesThe implementation correctly transforms the list of secret names into the format expected by Kubernetes for imagePullSecrets.
tests/model_serving/model_server/inference_service_configuration/test_isvc_pull_secret_updates.py (3)
11-28: Well-structured test parameterizationGood job setting up the test with a parameterized fixture that uses a real OCI image, which provides a realistic test scenario for testing pull secrets.
30-34: Good initial test for pull secret verificationThis test properly verifies that the pull secret is correctly set when the inference service is initially created.
36-38: Good test for verifying pull secret updatesThis test correctly verifies that updating the pull secret is properly reflected in the pod.
tests/model_serving/model_server/inference_service_configuration/conftest.py (4)
7-10: Required imports added for new fixtures.These imports are correctly added to support the new fixtures that manage inference services with image pull secrets.
15-16: New constants imported for pull secret testing.The
original_pull_secretandupdated_pull_secretconstants are properly imported from the constants file to be used in the fixtures.
98-110: Properly implemented fixture for updating pull secret.This fixture correctly updates the inference service to use the new pull secret value. The implementation uses the context manager pattern appropriately to ensure proper resource management.
112-129: Correctly implemented fixture for removing pull secret.This fixture properly removes the pull secret by explicitly setting
imagePullSecretstoNone. The code is well-structured and the comment on line 124 adds clarity about the intent to remove the field completely.
tests/model_serving/model_server/inference_service_configuration/conftest.py
Show resolved
Hide resolved
tests/model_serving/model_server/inference_service_configuration/constants.py
Outdated
Show resolved
Hide resolved
|
hey @VedantMahabaleshwarkar, how much time does the test take to run? |
less than 2 minutes from my testing |
VedantMahabaleshwarkar
force-pushed
the
j-19717
branch
from
c8a1c79 to
0e1746e
Compare
|
Signed-off-by: Vedant Mahabaleshwarkar <vmahabal@redhat.com>
VedantMahabaleshwarkar
force-pushed
the
j-19717
branch
from
0e1746e to
79e9e3e
Compare
.../model_serving/model_server/inference_service_configuration/test_isvc_pull_secret_updates.py
Show resolved
Hide resolved
|
Status of building tag latest: success. |
6 participants
Description
Adds tests for JIRAs : https://issues.redhat.com//browse/RHOAIENG-19717 , https://issues.redhat.com/browse/RHOAIENG-20331 which have the same rootcause
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Tests
Chores