fix: update kserve models web application for PR #163 restructure#3393
Conversation
|
Welcome to the Kubeflow Manifests Repository Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community. Before making more PRs: Community Resources:
Thanks again for helping to improve Kubeflow. |
There was a problem hiding this comment.
Pull request overview
This PR prepares kubeflow/manifests for a restructuring of KServe models web application manifests (kserve/models-web-app#163). It renames all Kubernetes resources from kserve-models-web-app to kserve-models-web-application, reorganizes Kustomize manifests into a base/components/overlays structure, and improves test reliability.
Changes:
- Renamed all Kubernetes resources, labels, and selectors from
kserve-models-web-apptokserve-models-web-applicationacross Kustomize manifests, Helm chart, CI workflows, and tests. - Restructured Kustomize manifests: moved Istio resources (AuthorizationPolicy, VirtualService, sidecar injection) into a
components/istioKustomize component, added acomponents/commoncomponent, and removed the standaloneistio.yamlfrom base. - Improved Test 3 in
kserve_test.shwith a dedicated port-forward,kubeflow-useridauthentication headers, retry logic for bootstrap, and better error diagnostics.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/kserve_test.sh |
Added auth headers, dedicated port-forward, retry bootstrap loop, better error handling for Test 3 |
tests/kserve_install.sh |
Updated deployment wait name |
tests/helm_kustomize_compare.py |
Removed stale expected extra-resource allowance for base scenario |
scripts/synchronize-kserve-web-application-manifests.sh |
Updated source manifests path and commit placeholder |
experimental/helm/charts/kserve-models-web-app/values.yaml |
Updated imageTag to 0.16.0, disabled VirtualService by default, added useridPrefix |
experimental/helm/charts/kserve-models-web-app/templates/istio/virtualservice.yaml |
Render VirtualService when kubeflow is enabled |
experimental/helm/charts/kserve-models-web-app/templates/deployment.yaml |
Use fullname for container name, add GUNICORN_CMD_ARGS env var |
experimental/helm/charts/kserve-models-web-app/templates/configmap.yaml |
Added USERID_PREFIX config |
experimental/helm/charts/kserve-models-web-app/templates/_helpers.tpl |
Hardcoded kserve-models-web-application as default name, simplified fullname |
experimental/helm/charts/kserve-models-web-app/Chart.yaml |
Updated appVersion to v0.16.0 |
applications/kserve/models-web-app/base/* |
Renamed resources, updated indentation |
applications/kserve/models-web-app/components/istio/* |
New Istio component with AuthorizationPolicy, VirtualService, sidecar |
applications/kserve/models-web-app/components/common/kustomization.yaml |
New common component with shared labels |
applications/kserve/models-web-app/overlays/kubeflow/* |
Updated to use components, renamed references |
.github/workflows/kserve_models_web_application_test.yaml |
Updated deployment name references |
You can also share your feedback on Copilot code review. Take the survey.
da7f833 to
26043c9
Compare
…ture Prepares kubeflow/manifests for kserve/models-web-app PR kubeflow#163 which: - Moves manifests from config/ to manifests/kustomize/ - Adds components/ layer for Istio and common labels - Renames deployment: kserve-models-web-app -> kserve-models-web-application Changes: - Sync new manifests/kustomize/ structure into applications/kserve/models-web-app/ - Rename all resources, labels, selectors to kserve-models-web-application - Update tests/kserve_install.sh deployment wait - Update tests/kserve_test.sh Test 3: port-forward to new service, kubeflow-userid auth headers, retry bootstrap loop - Update .github/workflows/kserve_models_web_application_test.yaml - Update Helm chart parity for the renamed manifests and Kubeflow overlay - Update Chart.yaml appVersion and values.yaml imageTag to 0.16.0 - Update tests/helm_kustomize_compare.py expectations - Update sync script SOURCE_MANIFESTS_PATH and COMMIT placeholder NOTE: COMMIT=195cabdf is a placeholder for PR kubeflow#163 HEAD SHA. Update to real release tag once kserve/models-web-app PR kubeflow#163 merges. Related: kserve/models-web-app#163 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
tests/kserve_test.sh: revert the port-forward, auth headers, retry loop, and extra XSRF validation so the file returns to upstream behavior with only the deployment rename. This matches the green fork validation where Test 3 passed through the standard gateway path. experimental/helm/charts/kserve-models-web-app: - Chart.yaml: rename chart name to kserve-models-web-application - templates/_helpers.tpl: restore standard name/fullname/chart helpers that derive from .Chart.Name To keep the repo's parity gate consistent with the new chart name, tests/helm_kustomize_compare.sh now renders the KServe chart with Helm release name kserve-models-web-application for both scenarios. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
52e13ab to
c4955ed
Compare
|
Please also do an automatic git diff style difference between the old rendered manifest and the new rendered manifests and post the differences here. |
|
|
Please revert all changes in tests/kserve_test.sh except for model-web-app -> models-web-application. Everything else must be justifies with strong arguments. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the downstream KServe Models Web Application manifests and integration scripts to match the upstream restructure from kserve/models-web-app#163, including the new Kustomize base/components layout and the renamed deployment/resources.
Changes:
- Sync upstream
manifests/kustomizeintoapplications/kserve/models-web-app/and introduce an Istio Kustomize component. - Rename K8s resources/selectors from
kserve-models-web-apptokserve-models-web-application. - Update synchronization scripts, README upstream reference, and CI/test scripts to use the new names and paths.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/kserve_test.sh | Adjust web app API test to use new deployment/service name and add userid header + port-forward bootstrap flow. |
| tests/kserve_install.sh | Update readiness wait to the renamed deployment. |
| scripts/synchronize-kserve-web-application-manifests.sh | Sync script now tracks the upstream merge commit and new manifests path. |
| applications/kserve/models-web-app/overlays/kubeflow/kustomization.yaml | Wire in new component layout and renamed config/patch targets. |
| applications/kserve/models-web-app/overlays/kubeflow/patches/web-application-vsvc.yaml | Update VirtualService destination host for renamed service. |
| applications/kserve/models-web-app/components/istio/kustomization.yaml | New Kustomize component for Istio resources/patches. |
| applications/kserve/models-web-app/components/istio/authorization-policy.yaml | Reintroduce AuthorizationPolicy under the new component and naming. |
| applications/kserve/models-web-app/components/istio/virtual-service.yaml | Rename VirtualService and update destination host. |
| applications/kserve/models-web-app/components/istio/web-application-sidecar.yaml | Patch deployment name for sidecar injection. |
| applications/kserve/models-web-app/components/common/kustomization.yaml | Add common component labels (currently not referenced). |
| applications/kserve/models-web-app/base/{kustomization.yaml,deployment.yaml,service.yaml,rbac.yaml} | Rename core resources/selectors and configmap names to the new deployment/resource naming. |
| README.md | Update upstream sync reference to the merge commit and new path. |
| .github/workflows/kserve_models_web_application_test.yaml | Update workflow waits to the renamed deployment. |
Comments suppressed due to low confidence (3)
tests/kserve_test.sh:213
- The cookie jar filename uses
kserve_xcrf(likely a typo for XSRF). Renaming it tokserve_xsrf(and updating all references) would reduce confusion when debugging failures.
# Test unauthorized access to models web application
UNAUTHORIZED_TOKEN="$(kubectl -n default create token default)"
RESPONSE=$(curl -s -w "\n%{http_code}" "${BASE_URL}/api/namespaces/${NAMESPACE}/inferenceservices" -H "Authorization: Bearer ${UNAUTHORIZED_TOKEN}")
tests/kserve_test.sh:179
- This script now starts a background
kubectl port-forwardfor the models web application and adds an EXIT trap to clean it up, but later it also starts another backgroundkubectl port-forward(cluster-local-gateway) without cleanup. Consider centralizing cleanup in a single EXIT trap that reliably terminates both background port-forward processes to avoid leaving straykubectlprocesses (and potential port conflicts) on test exit/failure.
name: "sklearn-iris"
namespace: ${NAMESPACE}
spec:
predictor:
sklearn:
storageUri: "gs://kfserving-examples/models/sklearn/1.0/model"
resources:
requests:
cpu: "50m"
applications/kserve/models-web-app/overlays/kubeflow/patches/web-application-vsvc.yaml:15
- This JSON6902 patch repeats the same
replaceoperation for/spec/gatewaystwice. The second occurrence is redundant and makes the patch harder to reason about; please remove the duplicate operation so the patch has a single authoritative gateway replacement.
|
@Raakshass can you help here ? |
|
@juliusvonkohout reverted `curl -s
http://localhost:8080/kserve-endpoints/ -H 'Authorization: Bearer ***' -v
-c /tmp/kserve_xcrf.txt | grep -i set-cookie` -> `Process completed with
exit code 1`.the root cause is that the old gateway bootstrap path no longer returns the That missing-cookie failure is why I had added the extra Test 3 blocks |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
Could it be that there is an error in the manifests then? Why should we need the extra headers? |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
@juliusvonkohout all 9 checks are green now. Looking at kserve_install.sh I can replace the retry loop with explicit precondition checks if @danish9039 enables collaboration on the branch. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
Thank you. There are anyway open copilot comments that you need to take a look at. So we can also fix in the same PR #3393 (comment) |
Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
@juliusvonkohout My bad, the missing-cookie failure came from the stale NetworkPolicy selector under I have also addressed the comment regarding explicit precondition checks in |
|
@juliusvonkohout question : we still need to make changes in helm compare tooling as well .so, do you want that in a follow up PR ?? Also this PR use the commit SHA as the image tag is not cut yet |
|
Thank you |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I think |
Yes helm can also be done in a follow up PR please |
|
On the next realease of kserve models web application we will automatically get a proper tag. But you can also ping me on slack and I can cut a 1.16.2 release today |
Summary of Changes
Prepares
kubeflow/manifestsfor the merged upstream change in kserve/models-web-app#163, which restructures the Kustomize manifests and renames the deployment.Scope
This draft PR is intentionally limited to the synced application manifests and the downstream integration updates needed for that upstream change.
Deferred to a follow-up PR:
experimental/helm/charts/kserve-models-web-app/**tests/helm_kustomize_compare.pytests/helm_kustomize_compare.shChanges
manifests/kustomize/structure intoapplications/kserve/models-web-app/base/components/overlayslayout from upstreamkserve-models-web-apptokserve-models-web-applicationscripts/synchronize-kserve-web-application-manifests.shSOURCE_MANIFESTS_PATH=config->manifests/kustomizeCOMMIT=v0.16.1->c71ee4309f0335159d9fdfd4559a538b5c782c92.github/workflows/kserve_models_web_application_test.yamltests/kserve_install.shtests/kserve_test.shNotes
c71ee4309f0335159d9fdfd4559a538b5c782c92because releasev0.16.1was cut beforekserve/models-web-app#163merged.Validation
kustomize build applications/kserve/models-web-app/basekustomize build applications/kserve/models-web-app/overlays/kubeflowbash -n scripts/synchronize-kserve-web-application-manifests.sh tests/kserve_install.sh tests/kserve_test.shContributor Checklist