Skip to content

Migrates private endpoint tests to use OVMS runtime#1194

Merged
mwaykole merged 5 commits intoopendatahub-io:mainfrom
mwaykole:fix-privavte
Mar 10, 2026
Merged

Migrates private endpoint tests to use OVMS runtime#1194
mwaykole merged 5 commits intoopendatahub-io:mainfrom
mwaykole:fix-privavte

Conversation

@mwaykole
Copy link
Copy Markdown
Member

@mwaykole mwaykole commented Mar 10, 2026

Summary

  • Migrates private endpoint tests to use OVMS runtime with S3 storage (openvino-example-model)
    instead of Caikit-TGIS runtime which is no longer available on test clusters
  • Switches deployment mode from Serverless to RawDeployment since test clusters lack Knative Serving
  • Removes Istio sidecar logic (not applicable to RawDeployment) and simplifies curl tests
    to verify same-namespace and cross-namespace internal endpoint connectivity
  • Uses OVMS v2 health endpoint (/v2/health/ready) instead of Caikit's /health

Summary by CodeRabbit

  • Tests

    • Updated test suite to validate OVMS-based raw deployments and internal endpoint health checks.
    • Simplified health-check logic to use a consistent readiness endpoint and expect HTTP 200.
    • Reworked namespace test scenarios for clearer same- and cross-namespace behavior.
  • Refactor

    • Test utilities refactored to use a dedicated curl pod and support optional port overrides for connectivity checks.

Signed-off-by: Milind waykole <mwaykole@redhat.com>
@mwaykole mwaykole changed the title Fix test for private endpoint Migrates private endpoint tests to use OVMS runtime Mar 10, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: ebab9a8a-240b-4ef6-b6c6-1eabe6ae388e

📥 Commits

Reviewing files that changed from the base of the PR and between 8936a6d and 1e142ec.

📒 Files selected for processing (1)
  • tests/model_serving/model_server/kserve/private_endpoint/test_kserve_private_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/model_serving/model_server/kserve/private_endpoint/test_kserve_private_endpoint.py

📝 Walkthrough

Walkthrough

Migrates KServe private-endpoint tests from CAIKIT/TGIS to OVMS (OpenVINO Model Server) using RAW_DEPLOYMENT; updates fixtures, test cases, and utilities to use OVMS health endpoint (v2/health/ready), curl pod helpers, and port-aware URL parsing.

Changes

Cohort / File(s) Summary
Fixture configuration
tests/model_serving/model_server/kserve/private_endpoint/conftest.py
endpoint_isvc fixture signature now accepts model_service_account: ServiceAccount; deployment_mode changed to KServeDeploymentType.RAW_DEPLOYMENT, storage switched to ModelStoragePath.OPENVINO_EXAMPLE_MODEL, model_format derived from runtime spec, wait_for_predictor_pods set to False. Renamed fixtures: endpoint_pod_with_istio_sidecarsame_namespace_pod, diff_pod_with_istio_sidecardiff_namespace_pod. Removed endpoint_pod_without_istio_sidecar and diff_pod_without_istio_sidecar. Replaced create_sidecar_pod calls with create_curl_pod and pass pod_name overrides.
Tests (behavior & constants)
tests/model_serving/model_server/kserve/private_endpoint/test_kserve_private_endpoint.py
Switched parametrization to OVMS runtime (ovms-endpoint-runtime / RuntimeTemplates.OVMS_KSERVE). Added OVMS constants: OVMS_REST_PORT, OVMS_HEALTH_ENDPOINT, HTTP_OK. Renamed test_curl_with_istio_same_nstest_curl_same_namespace; removed Istio-specific tests and replaced with test_curl_diff_namespace. Tests now assert internal endpoint URL presence (includes name and namespace) and perform health-checks against OVMS v2/health/ready on port 8888 expecting HTTP 200. Simplified imports accordingly.
Utilities (curl & pod helpers)
tests/model_serving/model_server/kserve/private_endpoint/utils.py
curl_from_pod now accepts optional port override, forces scheme, reconstructs URL explicitly, and returns HTTP status code as a string. create_sidecar_pod renamed to create_curl_pod and removed use_istio parameter. Pod construction moved from Pod(**pod_kwargs) to explicit Pod(client=..., name=..., namespace=..., containers=...) usage.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the main changes (OVMS migration, RawDeployment switch, Istio removal, health endpoint update) but omits required template sections (Related Issues, How it has been tested, Additional Requirements). Add Related Issues section with issue/JIRA links, specify testing method (Locally/Jenkins), and confirm Additional Requirements checklist items.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: migrating private endpoint tests from Caikit-TGIS to OVMS runtime, which is the primary focus of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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: 2

🧹 Nitpick comments (1)
tests/model_serving/model_server/kserve/private_endpoint/utils.py (1)

47-47: Potential double-slash in URL if endpoint starts with /.

If endpoint is passed as /v2/health/ready instead of v2/health/ready, the URL becomes http://host:port//v2/health/ready. Consider stripping leading slashes:

Proposed fix
-    url = f"{parsed.geturl()}/{endpoint}"
+    url = f"{parsed.geturl()}/{endpoint.lstrip('/')}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/model_serving/model_server/kserve/private_endpoint/utils.py` at line
47, The URL concatenation can produce a double slash when endpoint starts with a
leading slash; update the construction of url in utils.py to normalize parts
(e.g., ensure parsed.geturl() has no trailing slash and endpoint has no leading
slash) before joining—refer to the variables parsed and endpoint and the
assignment to url and change the join logic to strip the trailing/leading
slashes (or use a safe join like urllib.parse.urljoin) so the resulting url is
always well-formed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/model_serving/model_server/kserve/private_endpoint/conftest.py`:
- Line 42: The fixture in conftest.py reads
serving_runtime_from_template.instance.spec.supportedModelFormats[0].name
without validating the list; modify the code to first extract supported_formats
= serving_runtime_from_template.instance.spec.supportedModelFormats, validate
that supported_formats is truthy and non-empty, and then use
supported_formats[0].name, otherwise raise a clear ValueError (or return a
sensible fallback) with a descriptive message so tests that exercise empty
supportedModelFormats do not raise IndexError.
- Line 45: Add the pytest ordering marker to ensure ISVC readiness is verified
before other tests when running in parallel: annotate the test function named
test_deploy_model_state_loaded with `@pytest.mark.order`(1) so that, given
wait_for_predictor_pods=False, the readiness assertion runs first and prevents
curl/other tests from starting before the predictor is confirmed ready; place
the decorator immediately above the test_deploy_model_state_loaded definition in
the same test module.

---

Nitpick comments:
In `@tests/model_serving/model_server/kserve/private_endpoint/utils.py`:
- Line 47: The URL concatenation can produce a double slash when endpoint starts
with a leading slash; update the construction of url in utils.py to normalize
parts (e.g., ensure parsed.geturl() has no trailing slash and endpoint has no
leading slash) before joining—refer to the variables parsed and endpoint and the
assignment to url and change the join logic to strip the trailing/leading
slashes (or use a safe join like urllib.parse.urljoin) so the resulting url is
always well-formed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 338d406c-7110-4a0a-9e3d-153c7c79264f

📥 Commits

Reviewing files that changed from the base of the PR and between 4628796 and 8936a6d.

📒 Files selected for processing (3)
  • tests/model_serving/model_server/kserve/private_endpoint/conftest.py
  • tests/model_serving/model_server/kserve/private_endpoint/test_kserve_private_endpoint.py
  • tests/model_serving/model_server/kserve/private_endpoint/utils.py

@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

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

Signed-off-by: Milind waykole <mwaykole@redhat.com>
@mwaykole mwaykole enabled auto-merge (squash) March 10, 2026 15:41
@mwaykole mwaykole merged commit 37679e2 into opendatahub-io:main Mar 10, 2026
7 checks passed
@github-actions
Copy link
Copy Markdown

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

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.

4 participants