feat(kserve): automates installation and configuration of KServe prerequisites #691

israel-hdez · 2023-11-02T06:55:06Z

Fixes #666

Description

KServe pre-requisites are:

Service Mesh (Istio)
- A minimal Control Plane is configured for KServe with only Pilot and default gateways.
- An additional knative: ingressgateway is set for the Istio Ingress gateway workload.
- Some ports are excluded from envoy to allow for metrics collection and KNative hooks.
- Metrics collection is configured for Pilot and the gateways.
Serverless (KNative)
- Only serving components are needed from KNative.
- For the most part, a typical Serving deployment is configured, with Istio as networking layer.
- By default, a self-signed certificate is generated using the OpenShift Ingress domain. Users can provide their own secret with a production ready TLS certificate.

Fixes opendatahub-io/kserve#105

How Has This Been Tested?

Manual testing, for the most part.

Some instructions (targeted for the serving team): opendatahub-io/kserve#105 (comment)

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

pkg/feature/templates/serverless/serving-install/knative-serving.tmpl

pkg/feature/templates/servicemesh/base/smcp.tmpl

KServe pre-requisites are: * Service Mesh (Istio) * A minimal Control Plane is configured for KServe with only Pilot and default gateways. * An additional knative: ingressgateway is set for the Istio Ingress gateway workload. * Some ports are excluded from envoy to allow for metrics collection and KNative hooks. * Metrics collection is configured for Pilot and the gateways. * Serverless (KNative) * Only serving components are needed from KNative. * For the most part, a typical Serving deployment is configured, with Istio as networking layer. * By default, a self-signed certificate is generated using the OpenShift Ingress domain. Users can provide their own secret with a production ready TLS certificate. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

pkg/feature/templates/serverless/serving-istio-gateways/local-gateway-svc.tmpl

zdtsw · 2023-11-03T08:08:16Z

ref #666

apis/dscinitialization/v1/servicemesh_types.go

pkg/feature/templates/servicemesh/metrics-collection/pilot-metrics-collection.tmpl

pkg/feature/serverless/serverless_feature.go

pkg/feature/servicemesh/conditions.go

pkg/feature/servicemesh/servicemesh_feature.go

pkg/feature/serverless/serverless_feature.go

bartoszmajsak

Thanks for all the great work! I have shared a few things worth considering before we merge it.

FWIW: There are some tests you might want to include as part of this PR (and perhaps write some more) - have a look here https://github.com/maistra/opendatahub-operator/blob/service-mesh-integration/tests/integration/servicemesh/service_mesh_setup_features_int_test.go

apis/dscinitialization/v1/dscinitialization_types.go

apis/dscinitialization/v1/serverless_types.go

apis/dscinitialization/v1/servicemesh_types.go

apis/dscinitialization/v1/dscinitialization_types.go

pkg/feature/servicemesh/conditions.go

pkg/feature/templates/servicemesh/base/member-roll.tmpl

pkg/feature/templates/servicemesh/metrics-collection/envoy-metrics-collection.tmpl

pkg/feature/templates/servicemesh/metrics-collection/pilot-metrics-collection.tmpl

apis/dscinitialization/v1/serverless_types.go

get_all_manifests.sh

* Implement EnsureServerlessAbsent func. * Fix PR feedback. * Revert get_all_manifests.sh Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

apis/dscinitialization/v1/servicemesh_types.go

pkg/feature/serverless/conditions.go

israel-hdez · 2023-11-07T01:13:46Z

FWIW: There are some tests you might want to include as part of this PR (and perhaps write some more) - have a look here https://github.com/maistra/opendatahub-operator/blob/service-mesh-integration/tests/integration/servicemesh/service_mesh_setup_features_int_test.go

@bartoszmajsak Definitely! Let me work on the tests on my Tuesday.

Although, I'll let the operator team to decide if we are good to merge while I work on the tests. I know there is a scheduled release planned for this week.

israel-hdez · 2023-11-07T02:59:01Z

/retest

israel-hdez · 2023-11-07T03:00:24Z

@Jooho this is ready for testing.
I will work on units, in the meantime.

bartoszmajsak · 2023-11-07T11:27:34Z

apis/dscinitialization/v1/servicemesh_types.go

+	// +kubebuilder:validation:Enum=Kubernetes;ThirdParty
+	// +kubebuilder:default=Kubernetes


I wonder if we can't pull maistra/api for this purpose? https://github.com/maistra/istio-operator/blob/d922eece2343343e27016ea0d498bda36bfa64d8/pkg/apis/maistra/v2/security.go#L202-L209

One big question I have in mind is that with these kind of settings, we are drifting away from a potential support of upstream Istio and locking it to Opesnhift Service Mesh.

This options seems to be related to this: https://istio.io/latest/docs/ops/best-practices/security/#configure-third-party-service-account-tokens.

If it is, I think it still would be applicable to upstream.

Upstream docs seem to have a hint about automating this: looks like a check against the TokenRequest API would tell if we can use ThirdParty. Perhaps we should set this to ThirdParty unless the TokenRequest API is not available. From those upstream docs, it looks like ThirdParty should be preferred over first party. Although, I'm not sure what really would be the recommendation on Maistra.

It is related, but wrapped in Maistra config/objects. How did you test it thus far without this feature as it is defaulting to Kubernetes (first-party-jwt)?

I mainly use a non-ROSA cluster, so first party tokens are OK.
And... well... I haven't tested this on ROSA. I'm just trusting this change would be enough on ROSA.

i set it to ThirdParty in my ROSA cluster and it created the 3 featuretrackers ( not enable monitoring one)
previously when not set it (fall back to Kubernetes) had issue to set FTer up

Conclusion: we will document this scenario rather than expose it as a config in DSCI. Relevant docs on OSSM side: https://access.redhat.com/documentation/en-us/openshift_container_platform/4.8/html-single/service_mesh/index#ossm-install-rosa_ossm-create-smcp

/cc @danielezonca

pkg/feature/serverless/serverless_feature.go

bartoszmajsak · 2023-11-07T11:57:08Z

FWIW: There are some tests you might want to include as part of this PR (and perhaps write some more) - have a look here maistra/opendatahub-operator@service-mesh-integration/tests/integration/servicemesh/service_mesh_setup_features_int_test.go

@bartoszmajsak Definitely! Let me work on the tests on my Tuesday.

Although, I'll let the operator team to decide if we are good to merge while I work on the tests. I know there is a scheduled release planned for this week.

@zdtsw @VaishnaviHire It's essential that we establish a consensus on key principles moving forward. From my perspective, every significant modification should come with automated tests. Without this, we not only waste valuable time on manual verification but also do not build safety net against potential regressions. The V2 overhaul presents us with an excellent chance to lay down a foundation of high-quality codebase. This, in turn, will enable us to develop exceptional tools for OpenDataHub moving forward.

Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

israel-hdez · 2023-11-13T22:52:03Z

Pushed commit to fix @zdtsw comments.

zdtsw · 2023-11-14T06:15:07Z

When creating a DSC, the spec by default does not contain kserve.serving which is needed for enabling Kserve/Service-Mesh (i.e. kserve.serving.managementState: Managed). It would be more efficient to include kserve.serving by default.

I see 2 approaches:

have serverless config under Kserve by default but set to Removed to both by default in ODH => what we have in the PR
do not have serverless config under Kserve by default, this will ask users to do the configs of serverless by themselves (not just a flip of Removed to Managed in the state) => as Matt suggested to make the enablement easiler

@danielezonca @israel-hdez @VaishnaviHire @bartoszmajsak
i dont think this will be a "must-done" to get the PR in, we can have a follow-up if everyone feels (2) is better for user.

bartoszmajsak · 2023-11-14T07:05:09Z

When creating a DSC, the spec by default does not contain kserve.serving which is needed for enabling Kserve/Service-Mesh (i.e. kserve.serving.managementState: Managed). It would be more efficient to include kserve.serving by default.

I see 2 approaches:

have serverless config under Kserve by default but set to Removed to both by default in ODH => what we have in the PR

do not have serverless config under Kserve by default, this will ask users to do the configs of serverless by themselves (not just a flip of Removed to Managed in the state) => as Matt suggested to make the enablement easiler

@danielezonca @israel-hdez @VaishnaviHire @bartoszmajsak i dont think this will be a "must-done" to get the PR in, we can have a follow-up if everyone feels (2) is better for user.

If we decide to make it all enabled by default it will mean having service mesh config enabled by default in DSCI too. I cannot answer from the requirements perspective what is preferred way. Perhaps @danielezonca can.

With that said I would rather make an issue and raise another PR as it seems like an improvement to me rather than an important fix.

zdtsw · 2023-11-14T07:32:20Z

to update:
from my live-build (of the latest commits from 13th Nov evening)
default-dsc was auto created with config like this:

zdtsw

/lgtm

zdtsw · 2023-11-14T11:12:47Z

forgot to add my screenshot after the test
to make it easier, only enabled kserve here

VaishnaviHire

/lgtm

openshift-ci · 2023-11-14T13:58:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartoszmajsak, cam-garrison, spolti, VaishnaviHire, zdtsw

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~~ [VaishnaviHire]

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

…equisites (opendatahub-io#691) * Implement installation and configuration of KServe prerequisites KServe pre-requisites are: * Service Mesh (Istio) * A minimal Control Plane is configured for KServe with only Pilot and default gateways. * An additional knative: ingressgateway is set for the Istio Ingress gateway workload. * Some ports are excluded from envoy to allow for metrics collection and KNative hooks. * Metrics collection is configured for Pilot and the gateways. * Serverless (KNative) * Only serving components are needed from KNative. * For the most part, a typical Serving deployment is configured, with Istio as networking layer. * By default, a self-signed certificate is generated using the OpenShift Ingress domain. Users can provide their own secret with a production ready TLS certificate. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * chore: updates apis/dscinitialization/v1/servicemesh_types.go Co-authored-by: Cameron Garrison <cgarriso@redhat.com> * wip: serverless as part of kserve * chore: removes identitytype from CRD * feat: ensures that serverless config is removed when KServe is enabled * chore: checks if service mesh is configured when serving is enabled * fix: returns error on fail to remove serverless * chore(dsci): attempts to reduce test flakyness * chore: removes misleading TODO * Fix Dockerfile Add new COPY statement, due to the new `infrastructure` directory Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * chore: regenerates bundle --------- Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Cameron Garrison <cgarriso@redhat.com>

* feat(kserve): automates installation and configuration of KServe prerequisites (opendatahub-io#691) * Implement installation and configuration of KServe prerequisites KServe pre-requisites are: * Service Mesh (Istio) * A minimal Control Plane is configured for KServe with only Pilot and default gateways. * An additional knative: ingressgateway is set for the Istio Ingress gateway workload. * Some ports are excluded from envoy to allow for metrics collection and KNative hooks. * Metrics collection is configured for Pilot and the gateways. * Serverless (KNative) * Only serving components are needed from KNative. * For the most part, a typical Serving deployment is configured, with Istio as networking layer. * By default, a self-signed certificate is generated using the OpenShift Ingress domain. Users can provide their own secret with a production ready TLS certificate. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * chore: updates apis/dscinitialization/v1/servicemesh_types.go Co-authored-by: Cameron Garrison <cgarriso@redhat.com> * wip: serverless as part of kserve * chore: removes identitytype from CRD * feat: ensures that serverless config is removed when KServe is enabled * chore: checks if service mesh is configured when serving is enabled * fix: returns error on fail to remove serverless * chore(dsci): attempts to reduce test flakyness * chore: removes misleading TODO * Fix Dockerfile Add new COPY statement, due to the new `infrastructure` directory Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * chore: regenerates bundle --------- Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Cameron Garrison <cgarriso@redhat.com> * Disable Service Mesh IOR (opendatahub-io#738) This disables automatic OpenShift routes creation. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> (cherry picked from commit 2c6f48c) * Add Defaults for new installs --------- Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> Co-authored-by: Edgar Hernández <ehernand@redhat.com> Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Cameron Garrison <cgarriso@redhat.com>

zdtsw · 2023-11-18T11:33:08Z

Btw, did someone try to run the operator build from this PR with the following DSC?
apiVersion: datasciencecluster.opendatahub.io/v1
kind: DataScienceCluster
metadata:
  name: example
  labels:
    app.kubernetes.io/name: datasciencecluster
spec:
  components:
    kserve:
      managementState: Managed
Results it:
2023-11-10T17:23:21Z DEBUG events DataScienceCluster instance example created, but have some failures in component 1 error occurred:
* DSCInitialization.dscinitialization.opendatahub.io "default" not found
The operator is creating the dsci as default-dsci. It seems to be changed by this pr: #708
Maybe this PR needs a rebase or?
correct. we need to to rebase this PR before testing it.
@spolti @zdtsw @VaishnaviHire The error is not affecting functionality of this PR. Yet, the error is present despite fetching changes from incubation branch.

I debugged a little bit, and found the hard-coded constant here:

opendatahub-operator/components/modelmeshserving/modelmeshserving.go

Line 142 in da25fa5

Name: "default",

.
Probably, it is worth to fix, before the freeze.

not sure why i got this message last night, but the code in the quote has been fixed in the incubation branch, unless you still see it in any of ODH nightly/downstream 2.5 build?

bdattoma · 2023-11-29T11:12:31Z

@israel-hdez

Users can provide their own secret with a production ready TLS certificate.

how?
EDIT. I guess by setting a secret name like done in the instruction godc

israel-hdez · 2023-11-29T15:54:30Z

@israel-hdez

Users can provide their own secret with a production ready TLS certificate.

how? EDIT. I guess by setting a secret name like done in the instruction godc

Yes, the secretName shown in the document is the default name.
You would need to use type: Provided if you want to use your own. Then, you will need to manually create the TLS secret with the specified name containing the certificate.

certificate:
  secretName: knative-serving-cert
  type: SelfSigned

openshift-ci bot requested review from etirelli and VaishnaviHire November 2, 2023 06:55

israel-hdez mentioned this pull request Nov 2, 2023

New Manifests for required CRs installation opendatahub-io/kserve#105

Closed

israel-hdez commented Nov 2, 2023

View reviewed changes

pkg/feature/templates/serverless/serving-install/knative-serving.tmpl Outdated Show resolved Hide resolved

israel-hdez commented Nov 2, 2023

View reviewed changes

pkg/feature/templates/servicemesh/base/smcp.tmpl Outdated Show resolved Hide resolved

israel-hdez force-pushed the kserve-mesh-serverless-prereq branch from 5223678 to 1d7055f Compare November 2, 2023 07:06

ReToCode reviewed Nov 2, 2023

View reviewed changes

pkg/feature/templates/serverless/serving-istio-gateways/local-gateway-svc.tmpl Show resolved Hide resolved

zdtsw added the rhods-2.5 label Nov 2, 2023

zdtsw reviewed Nov 3, 2023

View reviewed changes

apis/dscinitialization/v1/servicemesh_types.go Outdated Show resolved Hide resolved

zdtsw reviewed Nov 3, 2023

View reviewed changes

VaishnaviHire added odh-2.5 odh-2.4 and removed odh-2.5 labels Nov 6, 2023

bartoszmajsak requested changes Nov 6, 2023

View reviewed changes

openshift-ci bot assigned bartoszmajsak Nov 6, 2023

VedantMahabaleshwarkar reviewed Nov 6, 2023

View reviewed changes

get_all_manifests.sh Outdated Show resolved Hide resolved

israel-hdez force-pushed the kserve-mesh-serverless-prereq branch 2 times, most recently from b949942 to c241750 Compare November 7, 2023 00:53

Fix feedback and finish TODOs

b971312

* Implement EnsureServerlessAbsent func. * Fix PR feedback. * Revert get_all_manifests.sh Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

israel-hdez force-pushed the kserve-mesh-serverless-prereq branch from c241750 to b971312 Compare November 7, 2023 00:59

israel-hdez commented Nov 7, 2023

View reviewed changes

apis/dscinitialization/v1/servicemesh_types.go Outdated Show resolved Hide resolved

israel-hdez commented Nov 7, 2023

View reviewed changes

pkg/feature/serverless/conditions.go Outdated Show resolved Hide resolved

bartoszmajsak reviewed Nov 7, 2023

View reviewed changes

pkg/feature/serverless/serverless_feature.go Outdated Show resolved Hide resolved

Feedback: fix zdtsw comments

723b9d6

Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

openshift-ci bot removed the lgtm label Nov 13, 2023

zdtsw approved these changes Nov 14, 2023

View reviewed changes

openshift-ci bot assigned zdtsw Nov 14, 2023

openshift-ci bot added the lgtm label Nov 14, 2023

Merge branch 'incubation' into kserve-mesh-serverless-prereq

503e2d8

openshift-ci bot removed the lgtm label Nov 14, 2023

VaishnaviHire approved these changes Nov 14, 2023

View reviewed changes

openshift-ci bot assigned VaishnaviHire Nov 14, 2023

openshift-ci bot added the lgtm label Nov 14, 2023

openshift-ci bot added the approved label Nov 14, 2023

israel-hdez merged commit 0e086b4 into opendatahub-io:incubation Nov 14, 2023
6 of 7 checks passed

israel-hdez deleted the kserve-mesh-serverless-prereq branch November 14, 2023 18:45

This was referenced Nov 15, 2023

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

Closed

fix(ns): do not set owner ref for infra namespaces #743

Merged

VaishnaviHire mentioned this pull request Nov 15, 2023

Sync servicemesh changes red-hat-data-services/rhods-operator#117

Merged

Jooho mentioned this pull request Nov 20, 2023

Dependent Operators(Serverless/ServiceMesh) Manifests Delivery Method opendatahub-io/kserve#89

Closed

bartoszmajsak mentioned this pull request Jun 25, 2024

feat: reworks Feature to be generic data container #1052

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kserve): automates installation and configuration of KServe prerequisites #691

feat(kserve): automates installation and configuration of KServe prerequisites #691

israel-hdez commented Nov 2, 2023 •

edited

Loading

zdtsw commented Nov 3, 2023 •

edited

Loading

bartoszmajsak left a comment

israel-hdez commented Nov 7, 2023

israel-hdez commented Nov 7, 2023

israel-hdez commented Nov 7, 2023

bartoszmajsak Nov 7, 2023

israel-hdez Nov 7, 2023

bartoszmajsak Nov 8, 2023

israel-hdez Nov 8, 2023

zdtsw Nov 8, 2023

bartoszmajsak Nov 13, 2023

bartoszmajsak commented Nov 7, 2023 •

edited

Loading

israel-hdez commented Nov 13, 2023

zdtsw commented Nov 14, 2023

bartoszmajsak commented Nov 14, 2023 •

edited

Loading

zdtsw commented Nov 14, 2023

zdtsw left a comment

zdtsw commented Nov 14, 2023

VaishnaviHire left a comment

openshift-ci bot commented Nov 14, 2023

zdtsw commented Nov 18, 2023

bdattoma commented Nov 29, 2023 •

edited

Loading

israel-hdez commented Nov 29, 2023

		// +kubebuilder:validation:Enum=Kubernetes;ThirdParty
		// +kubebuilder:default=Kubernetes

feat(kserve): automates installation and configuration of KServe prerequisites #691

feat(kserve): automates installation and configuration of KServe prerequisites #691

Conversation

israel-hdez commented Nov 2, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

zdtsw commented Nov 3, 2023 • edited Loading

bartoszmajsak left a comment

Choose a reason for hiding this comment

israel-hdez commented Nov 7, 2023

israel-hdez commented Nov 7, 2023

israel-hdez commented Nov 7, 2023

bartoszmajsak Nov 7, 2023

Choose a reason for hiding this comment

israel-hdez Nov 7, 2023

Choose a reason for hiding this comment

bartoszmajsak Nov 8, 2023

Choose a reason for hiding this comment

israel-hdez Nov 8, 2023

Choose a reason for hiding this comment

zdtsw Nov 8, 2023

Choose a reason for hiding this comment

bartoszmajsak Nov 13, 2023

Choose a reason for hiding this comment

bartoszmajsak commented Nov 7, 2023 • edited Loading

israel-hdez commented Nov 13, 2023

zdtsw commented Nov 14, 2023

bartoszmajsak commented Nov 14, 2023 • edited Loading

zdtsw commented Nov 14, 2023

zdtsw left a comment

Choose a reason for hiding this comment

zdtsw commented Nov 14, 2023

VaishnaviHire left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 14, 2023

zdtsw commented Nov 18, 2023

bdattoma commented Nov 29, 2023 • edited Loading

israel-hdez commented Nov 29, 2023

israel-hdez commented Nov 2, 2023 •

edited

Loading

zdtsw commented Nov 3, 2023 •

edited

Loading

bartoszmajsak commented Nov 7, 2023 •

edited

Loading

bartoszmajsak commented Nov 14, 2023 •

edited

Loading

bdattoma commented Nov 29, 2023 •

edited

Loading