Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1d7055f
Implement installation and configuration of KServe prerequisites
israel-hdez Nov 1, 2023
b971312
Fix feedback and finish TODOs
israel-hdez Nov 7, 2023
82b9ba5
Fix more feedback
israel-hdez Nov 7, 2023
4be6c25
Merge branch 'incubation' into kserve-mesh-serverless-prereq
bartoszmajsak Nov 8, 2023
470168d
chore: updates apis/dscinitialization/v1/servicemesh_types.go
bartoszmajsak Nov 8, 2023
cbc6d32
fix: renames imports after merge
bartoszmajsak Nov 8, 2023
6e7642b
Run make lint after merge with incubation
israel-hdez Nov 8, 2023
394b575
Fix more feedback
israel-hdez Nov 8, 2023
882ea26
Add unit tests for serverless feature
israel-hdez Nov 9, 2023
343f666
Merge branch 'incubation' into kserve-mesh-serverless-prereq
israel-hdez Nov 9, 2023
f77c763
Fix linter errors
israel-hdez Nov 9, 2023
4b5b005
Add unit tests for service mesh feature
israel-hdez Nov 10, 2023
9b75b63
wip: serverless as part of kserve
bartoszmajsak Nov 10, 2023
d753b4d
chore: removes identitytype from CRD
bartoszmajsak Nov 10, 2023
a13df5b
feat: ensures that serverless config is removed when KServe is enabled
bartoszmajsak Nov 10, 2023
b089fd6
chore: checks if service mesh is configured when serving is enabled
bartoszmajsak Nov 10, 2023
d384afe
fix: returns error on fail to remove serverless
bartoszmajsak Nov 10, 2023
2805355
chore(dsci): attempts to reduce test flakyness
bartoszmajsak Nov 10, 2023
bbeef9e
chore: removes misleading TODO
bartoszmajsak Nov 10, 2023
364b743
Fix Dockerfile
israel-hdez Nov 11, 2023
61fa9ff
Merge branch 'incubation' into kserve-mesh-serverless-prereq
israel-hdez Nov 11, 2023
a04fda3
chore: regenerates bundle
bartoszmajsak Nov 13, 2023
036abed
Merge branch 'incubation' into kserve-mesh-serverless-prereq
bartoszmajsak Nov 13, 2023
723b9d6
Feedback: fix zdtsw comments
israel-hdez Nov 13, 2023
503e2d8
Merge branch 'incubation' into kserve-mesh-serverless-prereq
bartoszmajsak Nov 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion apis/dscinitialization/v1/dscinitialization_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,22 @@ type DSCInitializationSpec struct {
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=2
// +optional
Monitoring Monitoring `json:"monitoring,omitempty"`
// Configures Service Mesh as networking layer for Data Science Clusters components.
// The Service Mesh is a mandatory prerequisite for single model serving (KServe) and
// you should review this configuration if you are planning to use KServe.
// For other components, it enhances user experience; e.g. it provides unified
// authentication giving a Single Sign On experience.
bartoszmajsak marked this conversation as resolved.
Show resolved Hide resolved
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=3
// +optional
ServiceMesh ServiceMeshSpec `json:"serviceMesh,omitempty"`
// Configures Serverless (KNative Serving). This is a prerequisite for single model
// serving (KServe) and you should review this configuration if you are planning to use KServe.
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=4
// +optional
Serverless ServerlessSpec `json:"serverless,omitempty"`
bartoszmajsak marked this conversation as resolved.
Show resolved Hide resolved
// Internal development useful field to test customizations.
// This is not recommended to be used in production environment.
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=3
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=5
// +optional
DevFlags DevFlags `json:"devFlags,omitempty"`
}
Expand Down
68 changes: 68 additions & 0 deletions apis/dscinitialization/v1/serverless_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package v1

import operatorv1 "github.com/openshift/api/operator/v1"

// ServerlessSpec configures KNative components used in Open Data Hub. Specifically,
// KNative is used to enable single model serving (KServe).
type ServerlessSpec struct {
// +kubebuilder:validation:Enum=Managed;Removed
// +kubebuilder:default=Removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.google.com/document/d/1A3la5Zg93RcvYk6HW08VKxHbtbEoDZNablEOUwahPmo/edit#heading=h.eb02e5dvc38p according to this doc,
for clean install of operator, we will get kserve by default Managaed,
so i think we should have Serverless and ServiceMesh by default set to Managed than Removed
FYI @israel-hdez @bartoszmajsak

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very in favor of enabling these components by default, since these can have an important impact on the cluster.

Yet, if the consensus tells that it would be safe, I think we can do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zdtsw I think default here refers to if nothing is set, it will be set to Removed. This is same as all other components. We can update our default DSCI CR to set these components to Managed

ManagementState operatorv1.ManagementState `json:"managementState,omitempty"`
// Serving configures the KNative-Serving stack used for model serving. A Service
// Mesh (Istio) is prerequisite, since it is used as networking layer.
Serving ServingSpec `json:"serving,omitempty"`
}

// ServingSpec specifies the configuration for the KNative Serving components and their
// bindings with the Service Mesh.
type ServingSpec struct {
// Name specifies the name of the KNativeServing resource that is going to be
// created to instruct the KNative Operator to deploy KNative serving components.
// +kubebuilder:default=knative-serving
Name string `json:"name,omitempty"`
// Namespace specifies the namespace where the KNativeServing resource is going
// to be created.
// +kubebuilder:default=knative-serving
Namespace string `json:"namespace,omitempty"`
// LocalGatewayServiceName allows to customize the name of the Kubernetes Service that
// is going to be created for intra-cluster requests. The service is created in the
// Service Mesh namespace.
// +kubebuilder:default=knative-local-gateway
LocalGatewayServiceName string `json:"localGatewayServiceName,omitempty"`
// IngressGateway allows to customize some parameters for the Istio Ingress Gateway
// that is bound to KNative-Serving.
IngressGateway IngressGatewaySpec `json:"ingressGateway,omitempty"`
}

// IngressGatewaySpec represents the configuration of the KNative Ingress Gateway.
type IngressGatewaySpec struct {
// GatewaySelector specifies the label selector to choose the Istio Ingress Gateway to use
// for intercepting incoming requests. If unset, the selector knative=ingressgateway is used.
// GatewaySelector map[string]string `json:"selector,omitempty"`

// Domain specifies the DNS name for intercepting ingress requests coming from
// outside the cluster. Most likely, you will want to use a wildcard name,
// like *.example.com. If not set, the domain of the OpenShift Ingress is used.
// If you choose to generate a certificate, this is the domain used for the certificate request.
Domain string `json:"domain,omitempty"`
// Certificate specifies configuration about the location of the TLS certificate and
// if a certificate would be generated.
Certificate CertificateSpec `json:"certificate,omitempty"`
israel-hdez marked this conversation as resolved.
Show resolved Hide resolved
}

// CertificateSpec represents the specification of the certificate securing communications of
// the Istio Ingress Gateway for the KNative network.
type CertificateSpec struct {
// SecretName specifies the name of the Kubernetes Secret resource that contains a
// TLS certificate secure HTTP communications for the KNative network.
// +kubebuilder:default=knative-serving-cert
SecretName string `json:"secretName,omitempty"`
// Generate specifies if the TLS certificate should be generated automatically using an own private
// key. The private key is going to be stored in a secret with the same name as the
// TLS certificate plus the "-key" suffix (e.g. knative-serving-cert-key).
// If this value is set to None, pre-existence of the TLS Secret (SecretName) with a
// valid certificate is assumed.
// +kubebuilder:validation:Enum=SelfSigned;None
// +kubebuilder:default=SelfSigned
Generate string `json:"generate,omitempty"`
israel-hdez marked this conversation as resolved.
Show resolved Hide resolved
}
28 changes: 28 additions & 0 deletions apis/dscinitialization/v1/servicemesh_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package v1

import operatorv1 "github.com/openshift/api/operator/v1"

// ServiceMeshSpec configures Service Mesh.
type ServiceMeshSpec struct {
// +kubebuilder:validation:Enum=Managed;Removed
// +kubebuilder:default=Removed
ManagementState operatorv1.ManagementState `json:"managementState,omitempty"`
// Mesh holds configuration of Service Mesh used by Opendatahub.
Mesh MeshSpec `json:"mesh,omitempty"`
israel-hdez marked this conversation as resolved.
Show resolved Hide resolved
}

type MeshSpec struct {
// Name is a name Service Mesh Control Plane. Defaults to "minimal".
bartoszmajsak marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:default=data-science-smcp
zdtsw marked this conversation as resolved.
Show resolved Hide resolved
israel-hdez marked this conversation as resolved.
Show resolved Hide resolved
Name string `json:"name,omitempty"`
// Namespace is a namespace where Service Mesh is deployed. Defaults to "istio-system".
// +kubebuilder:default=istio-system
Namespace string `json:"namespace,omitempty"`
// MetricsCollection specifies if metrics from components on the Mesh namespace
// should be collected. Setting the value to "Istio" will collect metrics from the
// control plane and any proxies on the Mesh namespace (like gateway pods). Setting
// to "None" will disable metrics collection.
// +kubebuilder:validation:Enum=Istio;None
// +kubebuilder:default=Istio
MetricsCollection string `json:"monitoring,omitempty"`
israel-hdez marked this conversation as resolved.
Show resolved Hide resolved
}
96 changes: 96 additions & 0 deletions apis/dscinitialization/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,124 @@ spec:
description: Namespace for monitoring if it is enabled
type: string
type: object
serverless:
description: Configures Serverless (KNative Serving). This is a prerequisite
for single model serving (KServe) and you should review this configuration
if you are planning to use KServe.
properties:
managementState:
default: Removed
enum:
- Managed
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VaishnaviHire @zdtsw it applies to most of the things here so it's not a comment for this particular PR... the way we use states now makes enum and pattern not matching each other. I wonder which takes precedence here and if we shouldn't align it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old plan was to not have enum in the end when we implemented Unmanaged (not sure if we will need Force or not in long term) but since we do not want user be able to put Unmanaged or Force for the time being, we added enum to force with 2 options.
My personal opinion: once logic for Unmanaged is done, we can remove enum

type: string
serving:
description: Serving configures the KNative-Serving stack used
for model serving. A Service Mesh (Istio) is prerequisite, since
it is used as networking layer.
properties:
ingressGateway:
description: IngressGateway allows to customize some parameters
for the Istio Ingress Gateway that is bound to KNative-Serving.
properties:
certificate:
description: Certificate specifies configuration about
the location of the TLS certificate and if a certificate
would be generated.
properties:
generate:
default: SelfSigned
description: Generate specifies if the TLS certificate
should be generated automatically using an own private
key. The private key is going to be stored in a
secret with the same name as the TLS certificate
plus the "-key" suffix (e.g. knative-serving-cert-key).
If this value is set to None, pre-existence of the
TLS Secret (SecretName) with a valid certificate
is assumed.
enum:
- SelfSigned
- None
type: string
secretName:
default: knative-serving-cert
description: SecretName specifies the name of the
Kubernetes Secret resource that contains a TLS certificate
secure HTTP communications for the KNative network.
type: string
type: object
domain:
description: Domain specifies the DNS name for intercepting
ingress requests coming from outside the cluster. Most
likely, you will want to use a wildcard name, like *.example.com.
If not set, the domain of the OpenShift Ingress is used.
If you choose to generate a certificate, this is the
domain used for the certificate request.
type: string
type: object
localGatewayServiceName:
default: knative-local-gateway
description: LocalGatewayServiceName allows to customize the
name of the Kubernetes Service that is going to be created
for intra-cluster requests. The service is created in the
Service Mesh namespace.
type: string
name:
default: knative-serving
description: Name specifies the name of the KNativeServing
resource that is going to be created to instruct the KNative
Operator to deploy KNative serving components.
type: string
namespace:
default: knative-serving
description: Namespace specifies the namespace where the KNativeServing
resource is going to be created.
type: string
type: object
type: object
serviceMesh:
description: Configures Service Mesh as networking layer for Data
Science Clusters components. The Service Mesh is a mandatory prerequisite
for single model serving (KServe) and you should review this configuration
if you are planning to use KServe. For other components, it enhances
user experience; e.g. it provides unified authentication giving
a Single Sign On experience.
properties:
managementState:
default: Removed
enum:
- Managed
- Removed
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
mesh:
description: Mesh holds configuration of Service Mesh used by
Opendatahub.
properties:
monitoring:
default: Istio
description: MetricsCollection specifies if metrics from components
on the Mesh namespace should be collected. Setting the value
to "Istio" will collect metrics from the control plane and
any proxies on the Mesh namespace (like gateway pods). Setting
to "None" will disable metrics collection.
enum:
- Istio
- None
type: string
name:
default: data-science-smcp
description: Name is a name Service Mesh Control Plane. Defaults
to "minimal".
type: string
namespace:
default: istio-system
description: Namespace is a namespace where Service Mesh is
deployed. Defaults to "istio-system".
type: string
type: object
type: object
required:
- applicationsNamespace
type: object
Expand Down
Loading
Loading