Fixes for Integrating KServe with Openshift #2853

skonto · 2023-04-25T20:15:22Z

What this PR does / why we need it:

This adds the following fixes for running KServe on Openshift:

Adds the capability to inject custom annotations to the ksvc required by the Openshift Serverless (deploys Knative Serving) and Service Mesh Integration.
The annotation for example serving.knative.openshift.io/enablePassthrough: “true” needs to be added at the ksvc level for inference services and graphs. This will not allow to add arbitrary annotations, thus should not change current semantics.
Adds the ability to run the storage injector with user id 1337 in order to solve the issue when Istio cni and dns proxy are enabled.
Right now the security context is copied from the user container but we cant use the same user id there for many reasons eg. is not allowed, security etc. The latter applies if we use pod security context or some other workaround.
The uid can be set to an arbitrary value using the annotation: serving.kserve.io/storage-initializer-uid: "...".
Note that on Openshift that uid is not 1337 but project_id_range+1, so we need this to be configurable.
Note: In a future PR we should be able to setup the Knative Serving init-containers feature flag and avoid the custom injection done or provide some template to allow the user to configure that part.

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Feature/Issue validation/testing:

Tested on OCP 4.12 with the following inference service:

oc apply -n test -f - <<EOF
apiVersion: serving.kserve.io/v1beta1
kind: InferenceService
metadata:
  name: sklearn-from-uri
  annotations:
    sidecar.istio.io/inject: "true"
    sidecar.istio.io/rewriteAppHTTPProbers: "true"
    serving.knative.openshift.io/enablePassthrough: “true”
spec:
  predictor:
    sklearn:
      storageUri: https://github.com/skonto/serverless-guides/blob/main/kserve/kfserving-examples/models/sklearn/1.0/model/model.joblib?raw=true
EOF

The manager deployment was started with the right env var.

Special notes for your reviewer:

No

Release note:

 Openshift integration improvements.

skonto · 2023-04-26T22:28:24Z

@yuzisun @alexagriffith gentle ping.

skonto · 2023-04-27T11:35:43Z

/assign @yuzisun

skonto · 2023-04-27T13:41:27Z

TODO: support scenario where OCP SM uses a uid other than 1337 to run the Istio side car.

ReToCode · 2023-05-02T06:14:12Z

TODO: support scenario where OCP SM uses a uid other than 1337 to run the Istio side car.

We could make it configurable? The default in Istio is 1337, but I think users could overwrite it.

skonto · 2023-05-07T14:11:08Z

We could make it configurable? The default in Istio is 1337, but I think users could overwrite it.

Yes that is my idea too given that OCP requires a project based id that varies.

skonto · 2023-05-11T10:36:51Z

I have updated the PR. I am using an annotation to pass the uid for the storage initializer.
This can be refactored if a more generic solution is added for adjusting that container's properties.
The annotations on Openshift can be used as follows:

kubectl apply -n test -f - <<EOF
apiVersion: serving.kserve.io/v1beta1
kind: InferenceService
metadata:
  name: sklearn-from-uri
  annotations:
    sidecar.istio.io/inject: "true"
    sidecar.istio.io/rewriteAppHTTPProbers: "true"
    serving.knative.openshift.io/enablePassthrough: "true"
    serving.kserve.io/storage-initializer-uid: "1000720001"
spec:
  predictor:
    sklearn:
      storageUri: https://github.com/skonto/serverless-guides/blob/main/kserve/kfserving-examples/models/sklearn/1.0/model/model.joblib?raw=true
EOF

skonto · 2023-05-11T10:40:13Z

@yuzisun pls review, this is meant to make the Openshift guide added lately to work out of the box with Istio.

skonto · 2023-05-25T13:30:34Z

@alexagriffith @yuzisun gentle ping.

yuzisun · 2023-05-31T14:36:14Z

@alexagriffith and I am not too familiar with OpenShift, @ckadner can you help review this PR?

ckadner · 2023-06-05T22:00:50Z

@skonto -- I have not deployed KServe in while, and not on OpenShift. Can you point me to how you deployed and configured your cluster? You can find me on Slack @ckadner (both on Kubeflow and the IBM workspaces), if that makes it easier.

yuzisun · 2023-06-12T07:48:26Z

@skonto Can you help sign off the commit following https://github.com/kserve/kserve/pull/2853/checks?check_run_id=13400347618 ?

skonto · 2023-06-14T15:05:46Z

@yuzisun I think its ready now.

yuzisun · 2023-06-16T03:20:39Z

pkg/controller/v1beta1/inferenceservice/reconcilers/knative/ksvc_reconciler.go

@@ -108,6 +109,14 @@ func createKnativeService(componentMeta metav1.ObjectMeta,
 		}
 	}

+	// Allow custom annotations for ksvcs that start with serving.knative but not part of serving.knative.dev group name.
+	for aKey, _ := range annotations {
+		if !strings.HasPrefix(aKey, constants.KnativeServingAPIGroupName) && strings.HasPrefix(aKey, constants.KnativeServingAPIGroupNamePrefix) {


Is it better to just check the full api group name serving.knative.openshift.io?

@yuzisun I thought about that but I didnt want to add vendor specific annotations, trying to keep it neutral for future use as well. Do you want me to change that?

@skonto I think this is no longer needed as we now allow component level labels and annotations, see #2925

@yuzisun unofrtunately that is per component. The requirement is to set this at the ksvc level, because the OCP Serverless reconciler expects that annotation (serving.knative.openshift.io/enablePassthrough: "true") to be at that level.
For example here is the diff between a kserve ksvc (does not work) and a regular ksvc that works as expected (I tried this using the new feature).

ok, l'd prefer using a more explicit list, we can add serving.knative.openshift.io/enablePassthrough to managedKsvcAnnotations ?

Yes that is possible. I will update it.

@yuzisun done.

Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

ReToCode · 2023-07-06T06:08:15Z

@yuzisun , @alexagriffith would you mind taking another look? We'd like to do some additional patching in opendatahub-io/kserve which is based on these changes.

yuzisun · 2023-07-06T10:21:38Z

pkg/constants/constants.go

-	DefaultMinReplicas  = 1
+	ControllerLabelName          = KServeName + "-controller-manager"
+	DefaultMinReplicas           = 1
+	IstioSidecarUIDAnnotationKey = KServeAPIGroupName + "/storage-initializer-uid"


Are we planning to make this configurable here also without having to add this annotation to every inference service?

The "issue" is, that this is specific to the namespace where the service is deployed. At the moment there is no better way than to specify it per InferenceService. We're working with the istio folks to hopefully get rid of the underlying issue.

yuzisun · 2023-07-06T10:23:33Z

@yuzisun , @alexagriffith would you mind taking another look? We'd like to do some additional patching in opendatahub-io/kserve which is based on these changes.

Looks good, do we need more documentation updates?

ReToCode · 2023-07-06T11:39:07Z

Looks good, do we need more documentation updates?

Yes we should definitely update https://github.com/kserve/kserve/blob/master/docs/OPENSHIFT_GUIDE.md#prerequisites. Is it ok if I do a followup PR for this?

yuzisun · 2023-07-07T12:26:47Z

Thanks @skonto @ReToCode ! We are including this to the upcoming 0.11 release

/lgtm
/approve

kserve-oss-bot · 2023-07-07T12:26:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto, yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yuzisun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* fixes for SM and OCP Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> * updates Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> * add pass through annotation support Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> --------- Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

* fixes for SM and OCP Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> * updates Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> * add pass through annotation support Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> --------- Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com> Signed-off-by: iamlovingit <freecode666@gmail.com>

kserve-oss-bot added the do-not-merge/work-in-progress label Apr 25, 2023

kserve-oss-bot requested review from alexagriffith and yuzisun April 25, 2023 20:15

skonto force-pushed the fixes_for_ocp branch from 48f4edb to 8f97b9a Compare April 25, 2023 20:17

skonto changed the title ~~[WIP] Fixes for Integrating KServe with Openshift~~ Fixes for Integrating KServe with Openshift Apr 26, 2023

kserve-oss-bot removed the do-not-merge/work-in-progress label Apr 26, 2023

ReToCode mentioned this pull request Apr 26, 2023

Update the OpenShift guide to version 4.12 and use OpenShift Serverless #2855

Merged

kserve-oss-bot assigned yuzisun Apr 27, 2023

skonto changed the title ~~Fixes for Integrating KServe with Openshift~~ [wip] Fixes for Integrating KServe with Openshift Apr 27, 2023

kserve-oss-bot added the do-not-merge/work-in-progress label Apr 27, 2023

skonto force-pushed the fixes_for_ocp branch from 8f97b9a to b37d471 Compare May 11, 2023 10:33

skonto changed the title ~~[wip] Fixes for Integrating KServe with Openshift~~ Fixes for Integrating KServe with Openshift May 11, 2023

kserve-oss-bot removed the do-not-merge/work-in-progress label May 11, 2023

yuzisun requested a review from ckadner May 31, 2023 14:35

ckadner assigned ckadner and unassigned yuzisun Jun 5, 2023

ckadner requested a review from rafvasq June 5, 2023 21:45

skonto force-pushed the fixes_for_ocp branch 2 times, most recently from 169556d to 6793200 Compare June 14, 2023 14:12

yuzisun reviewed Jun 16, 2023

View reviewed changes

yuzisun force-pushed the fixes_for_ocp branch from 6793200 to 6478949 Compare June 18, 2023 19:39

skonto mentioned this pull request Jun 26, 2023

Add KServe manifests to odh-manifests opendatahub-io/odh-manifests#838

Merged

3 tasks

skonto added 3 commits June 27, 2023 10:12

fixes for SM and OCP

a75c892

Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

updates

19c411c

Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

add pass through annotation support

8fdd11a

Signed-off-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

skonto force-pushed the fixes_for_ocp branch from 6478949 to 8fdd11a Compare July 5, 2023 09:38

skonto mentioned this pull request Jul 6, 2023

OpenShift patches to Kustomize manifests opendatahub-io/kserve#14

Merged

yuzisun reviewed Jul 6, 2023

View reviewed changes

kserve-oss-bot assigned yuzisun Jul 7, 2023

kserve-oss-bot added the lgtm label Jul 7, 2023

kserve-oss-bot added the approved label Jul 7, 2023

kserve-oss-bot merged commit ee0ff96 into kserve:master Jul 7, 2023
56 checks passed

ReToCode mentioned this pull request Jul 10, 2023

add documentation about storage-initializer userid for istio-cni #3028

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for Integrating KServe with Openshift #2853

Fixes for Integrating KServe with Openshift #2853

skonto commented Apr 25, 2023 •

edited

skonto commented Apr 26, 2023 •

edited

skonto commented Apr 27, 2023

skonto commented Apr 27, 2023

ReToCode commented May 2, 2023

skonto commented May 7, 2023 •

edited

skonto commented May 11, 2023 •

edited

skonto commented May 11, 2023 •

edited

skonto commented May 25, 2023

yuzisun commented May 31, 2023

ckadner commented Jun 5, 2023

yuzisun commented Jun 12, 2023

skonto commented Jun 14, 2023

yuzisun Jun 16, 2023

skonto Jun 21, 2023 •

edited

yuzisun Jun 24, 2023

skonto Jun 27, 2023 •

edited

yuzisun Jun 29, 2023

skonto Jul 4, 2023

skonto Jul 5, 2023

ReToCode commented Jul 6, 2023

yuzisun Jul 6, 2023

ReToCode Jul 6, 2023

yuzisun commented Jul 6, 2023

ReToCode commented Jul 6, 2023

yuzisun commented Jul 7, 2023

kserve-oss-bot commented Jul 7, 2023

Fixes for Integrating KServe with Openshift #2853

Fixes for Integrating KServe with Openshift #2853

Conversation

skonto commented Apr 25, 2023 • edited

skonto commented Apr 26, 2023 • edited

skonto commented Apr 27, 2023

skonto commented Apr 27, 2023

ReToCode commented May 2, 2023

skonto commented May 7, 2023 • edited

skonto commented May 11, 2023 • edited

skonto commented May 11, 2023 • edited

skonto commented May 25, 2023

yuzisun commented May 31, 2023

ckadner commented Jun 5, 2023

yuzisun commented Jun 12, 2023

skonto commented Jun 14, 2023

yuzisun Jun 16, 2023

Choose a reason for hiding this comment

skonto Jun 21, 2023 • edited

Choose a reason for hiding this comment

yuzisun Jun 24, 2023

Choose a reason for hiding this comment

skonto Jun 27, 2023 • edited

Choose a reason for hiding this comment

yuzisun Jun 29, 2023

Choose a reason for hiding this comment

skonto Jul 4, 2023

Choose a reason for hiding this comment

skonto Jul 5, 2023

Choose a reason for hiding this comment

ReToCode commented Jul 6, 2023

yuzisun Jul 6, 2023

Choose a reason for hiding this comment

ReToCode Jul 6, 2023

Choose a reason for hiding this comment

yuzisun commented Jul 6, 2023

ReToCode commented Jul 6, 2023

yuzisun commented Jul 7, 2023

kserve-oss-bot commented Jul 7, 2023

skonto commented Apr 25, 2023 •

edited

skonto commented Apr 26, 2023 •

edited

skonto commented May 7, 2023 •

edited

skonto commented May 11, 2023 •

edited

skonto commented May 11, 2023 •

edited

skonto Jun 21, 2023 •

edited

skonto Jun 27, 2023 •

edited