feat(operator): Cluster wide operator for V2 #4847

ukclivecox · 2023-05-17T06:36:06Z

Provides a cluster wide operator.

Adds SeldonConfig and SeldonRuntime custom resources
create a new seldon-core-v2-runtime Helm chart that install components via a new SeldonRuntime into a namespace
Tests
Docs for helm and uprading

ImagePullSecrets

Updates also imagePullSecrets yaml processing to replace the existing single imagePullSecret in v2 branch which caused issues for handling StatefulSet statehandling via Banzai as that was adding an empty list element which failed the 3-way merge creation. The update allows a full list or nothing to be added via Helm. This will be improved when we remove Kustomize from the creation of Helm chart and instread use the Helm chart as the source of truth.

…supported in CRDs - also trace endpoint has namespace added

…ents-sc

review-notebook-app · 2023-05-17T06:36:10Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

k8s/kustomize/componentsNoConfig/kustomization.yaml

ukclivecox · 2023-05-19T16:08:56Z

kafka/strimzi/values.yaml

@@ -1,3 +1,5 @@
+watchAnyNamespace: true


Check whether we need this still. Secrets are copied.

has this been checked?

operator/config/serverconfigs/kustomization.yaml

operator/scheduler/client.go

sakoush

Here are some comments so far.

sakoush · 2023-06-08T17:26:15Z

operator/apis/mlops/v1alpha1/seldonconfig_types.go

+	DataflowEngineName  = "dataflow-engine"
+	HodometerName       = "hodometer"


nit: I understand this is not part of this PR but these 2 services do not have seldon- prefix

future pr then?

sakoush · 2023-06-08T17:34:56Z

operator/controllers/reconcilers/common/patch_utils.go

@@ -0,0 +1,56 @@
+package common


insert copyright. perhaps also check that all new files have copyrights? it is probably useful at some point in a future PR to add a check for the existence of copyrights.

sakoush · 2023-06-08T17:38:40Z

operator/controllers/reconcilers/seldon/statefulset_reconciler.go

@@ -0,0 +1,236 @@
+/*
+Copyright 2022 Seldon Technologies Ltd.


change to 2023?

separate PR to update date, or add a range. Not actually sure if this is needed or not - will need to check

sakoush · 2023-06-08T17:40:42Z

operator/controllers/reconcilers/seldon/statefulset_reconciler.go

+const (
+	StatefulSetReadyReason    = "StatefulSet replicas matches desired replicas"
+	StatefulSetNotReadyReason = "StatefulSet replicas does not match desired replicas"
+)


should this section be at the start of the document?

sakoush · 2023-06-08T17:53:37Z

operator/scheduler/experiment.go

 	req := &scheduler.StartExperimentRequest{
 		Experiment: experiment.AsSchedulerExperimentRequest(),
 	}
 	logger.Info("Start", "experiment name", experiment.Name)
-	_, err := grcpClient.StartExperiment(ctx, req, grpc_retry.WithMax(2))
+	_, err = grcpClient.StartExperiment(ctx, req, grpc_retry.WithMax(2))


why specifically 2 retries? also extract as const?

will refactor to const

sakoush · 2023-06-08T17:57:49Z

operator/scheduler/server.go

 	if err != nil {
 		return err
 	}
 	return nil
 }

-func (s *SchedulerClient) SubscribeServerEvents(ctx context.Context) error {
+func (s *SchedulerClient) SubscribeServerEvents(ctx context.Context, namespace string, conn *grpc.ClientConn) error {


are we using namespace?

will remove

sakoush · 2023-06-08T17:58:57Z

operator/scheduler/model.go

 	if err != nil {
 		return err, s.checkErrorRetryable(model.Kind, model.Name, err)
 	}

 	return nil, false
 }

-func (s *SchedulerClient) SubscribeModelEvents(ctx context.Context) error {
+func (s *SchedulerClient) SubscribeModelEvents(ctx context.Context, namespace string, conn *grpc.ClientConn) error {


will remove

sakoush · 2023-06-08T17:59:21Z

operator/scheduler/experiment.go

 	return err, s.checkErrorRetryable(experiment.Kind, experiment.Name, err)
 }

-func (s *SchedulerClient) SubscribeExperimentEvents(ctx context.Context) error {
+func (s *SchedulerClient) SubscribeExperimentEvents(ctx context.Context, namespace string, conn *grpc.ClientConn) error {


will remove

sakoush · 2023-06-08T18:03:05Z

operator/scheduler/client.go

+		if err != nil {
+			return nil, err
+		}
+		err = s.smokeTestConnection(conn)


perhaps add a note to describe why we do a smoke test to scheduler? Make sure it is up before proceeding further?

sakoush · 2023-06-08T18:06:44Z

scheduler/pkg/tracing/provider_test.go

 			expectedConfig: &TracingConfig{
 				Enable:               true,
 				OtelExporterEndpoint: "0.0.0.0:1234",
-				Ratio:                0.5,
+				Ratio:                "0.5",


add test for invalid ratio?

added test to loading config and added go test for this check

sakoush

Here are the rest of my comments.

sakoush · 2023-06-09T09:11:01Z

operator/main.go

+	watchNamespace := namespace
+	if clusterwide {
+		watchNamespace = ""
+	}
+	setupLog.Info("Starting manager", "clusterwide", clusterwide)


It is slightly not clear to me why this is enough to enable clusterwide perhaps add a note with a description?
can we have clusterwide and per namespace install?

If its namespaced then the controller won't watch for changes outside its namespace

sakoush · 2023-06-09T09:21:22Z

operator/controllers/reconcilers/seldon/service_reconciler.go

+				{
+					Port:       DefaultXdsPort,
+					TargetPort: intstr.FromString(DefaultXdsPortName),
+					Name:       fmt.Sprintf("%s%s", serviceConfig.GrpcServicePrefix, DefaultXdsPortName),


is GrpcServicePrefix something new in this PR?

its part of the original Helm setup of services which we need to include when creating V1 SVCs to allow proper operation in some service meshes such as istio

sakoush · 2023-06-09T09:22:59Z

operator/controllers/reconcilers/seldon/service_reconciller_test.go

+				annotator)
+			g.Expect(err).To(BeNil())
+			err = sr.Reconcile()
+			if test.error {


I dont think we have test cases with errors? perhaps add if relevant or simplify the test code.

removed err check as only k8s errors would happen and this won't happen with FakeClient

sakoush · 2023-06-09T09:23:33Z

operator/controllers/reconcilers/seldon/runtime_reconciler.go

@@ -0,0 +1,142 @@
+package server


no copyrights

sakoush · 2023-06-09T09:31:28Z

k8s/helm-charts/seldon-core-v2-setup/values.yaml


 opentelemetry:
-  endpoint: seldon-collector:4317
+  endpoint: seldon-collector.seldon-mesh:4317


would this work still with per namespace install? I am assuming that users would need to provide another value instead?

The assumption its in the manager install namespace but agree this needs to be changed to be based on install namespace

will change so it uses namespace of install

actually, the reason its hardwired is we don't know where you will install opentelemetry or kafka so its up to you to change it really.

sakoush · 2023-06-09T09:52:39Z

operator/controllers/reconcilers/seldon/configmap_reconciler_test.go

+				meta)
+			g.Expect(err).To(BeNil())
+			err = sr.Reconcile()
+			if test.error {


no test cases with errors

sakoush · 2023-06-09T09:53:54Z

operator/controllers/reconcilers/seldon/deployment_reconciler_test.go

+		error            bool
+	}
+
+	tests := []test{


do we want to add the other components here as extra test cases?

sakoush · 2023-06-09T09:54:24Z

operator/controllers/reconcilers/seldon/deployment_reconciler_test.go

+				annotator)
+			g.Expect(err).To(BeNil())
+			err = sr.Reconcile()
+			if test.error {


no test cases with errors

sakoush · 2023-06-09T09:54:58Z

operator/controllers/reconcilers/seldon/rbac_reconciller_test.go

+			if test.error {
+				g.Expect(err).ToNot(BeNil())


sakoush · 2023-06-09T09:57:50Z

docs/source/contents/getting-started/kubernetes-installation/helm.md

@@ -4,7 +4,7 @@ We provide several Helm charts.

 * `seldon-core-v2-crds` : cluster wide install of custom resources


do we want to add docs on how to install clusterwide and the implications?

…ate commands

sakoush

LGTM.

One minor comment whether we need to fix the raw yaml manifests to seldon-mesh namespace. Although I think they need all to be recreated and checked in?

sakoush · 2023-06-15T11:57:03Z

k8s/Makefile

 	mv ${HELM_SERVERS_BASE}/.seldon-v2-servers.yaml ${HELM_SERVERS_BASE}/seldon-v2-servers.yaml

 .PHONY: create-yaml
 create-yaml:
-	helm template seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs > yaml/certs.yaml
+	helm template seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs --namespace seldon-mesh > yaml/certs.yaml


Alternatively I have removed the namespace check this PR for more info.

do we want the raw yaml to have seldon-mesh namespace hardcoded in?

I think the PR needs to be checked further as not sure how best to handle Roles which I think require namespaces but maybe not.

sakoush · 2023-06-15T11:59:34Z

k8s/yaml/components.yaml

@@ -28,20 +28,6 @@ metadata:
 ---
 # Source: seldon-core-v2-setup/templates/seldon-v2-components.yaml
 apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding


do we need to recreate all yaml manifests?

…rther docs for helm and new docs for upgrading

sakoush

Left some comments regarding the creation of raw yaml manifests.

sakoush · 2023-06-19T07:57:32Z

k8s/Makefile

 	mv ${HELM_SERVERS_BASE}/.seldon-v2-servers.yaml ${HELM_SERVERS_BASE}/seldon-v2-servers.yaml

 .PHONY: create-yaml
 create-yaml:
-	helm template seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs > yaml/certs.yaml
+	helm template seldon-core-v2-certs ./helm-charts/seldon-core-v2-certs --namespace seldon-mesh > yaml/certs.yaml


do we want the raw yaml to have seldon-mesh namespace hardcoded in?

sakoush · 2023-06-19T07:58:32Z

k8s/Makefile

-	helm template seldon-core-v2-components ./helm-charts/seldon-core-v2-setup > yaml/components.yaml
-	helm template seldon-core-v2-servers ./helm-charts/seldon-core-v2-servers > yaml/servers.yaml
+	helm template seldon-core-v2-components ./helm-charts/seldon-core-v2-setup --namespace seldon-mesh > yaml/components.yaml
+	helm template seldon-core-v2-servers ./helm-charts/seldon-core-v2-runtime --namespace seldon-mesh > yaml/servers.yaml


I think this needs to be reflected in the new change of having runtime and servers?

docs/source/contents/upgrading/index.md

Co-authored-by: Sherif Akoush <sherif.akoush@gmail.com>

sakoush

LGTM

ukclivecox added 17 commits May 5, 2023 13:40

Add new SeldonRuntime CRD

f72d00f

Add namespace to reconciler struct and mods to runtime overrides

4ca9ff9

remove namespace and slim down seldonruntime CR

6ce2c27

initial operator work

5d7ef39

initial tests worked for seldon-mesh namespace. Need to add configmaps

bd97746

add configmaps

fccea3a

configmaps working with trace ratio changed to string as float64 not …

e759b26

…supported in CRDs - also trace endpoint has namespace added

multi namespace initially working

dabfb3b

TLS tests

220c421

rename helm chart seldon-core-v2-servers to seldon-core-v2-runtime

2badd0f

fixes from tests

3a0567b

remove kustomize helm-components folder as is replaced by helm-compon…

bfff591

…ents-sc

Merge branch 'v2' into cluster-wide-operator-v2

87ed29b

Kafka mtls demo and move configuration to one struct

1436980

Allow overrrides for config settings

dc4b83e

Merge branch 'v2' into cluster-wide-operator-v2

14ae4cc

Add initial tests

5b6e5d7

ukclivecox mentioned this pull request May 17, 2023

How to enable cluster wide operator? [v2 version] #4735

Closed

RafalSkolasinski requested review from RafalSkolasinski and agrski May 17, 2023 08:51

ukclivecox added 3 commits May 18, 2023 09:27

initial lint and testing fixes

f419b4b

revert tls cert file defaulting

414aa76

further tests and docs

2d752c5

ukclivecox changed the title ~~DRAFT : Cluster wide operator for V2~~ Cluster wide operator for V2 May 19, 2023

lint

1afb09f

ukclivecox requested a review from sakoush May 19, 2023 14:28

ukclivecox commented May 19, 2023

View reviewed changes

k8s/kustomize/componentsNoConfig/kustomization.yaml Outdated Show resolved Hide resolved

ukclivecox commented May 19, 2023

View reviewed changes

operator/config/serverconfigs/kustomization.yaml Outdated Show resolved Hide resolved

ukclivecox commented May 19, 2023

View reviewed changes

operator/scheduler/client.go Outdated Show resolved Hide resolved

ukclivecox commented May 19, 2023

View reviewed changes

operator/scheduler/client.go Show resolved Hide resolved

initial review fixes

5d50fe2

ukclivecox added the v2 label May 24, 2023

ukclivecox added 3 commits May 31, 2023 19:25

Merge branch 'v2' into cluster-wide-operator-v2

b0c97d7

Fix imagepullsecrets translation to helm templates

8979186

Merge branch 'v2' into cluster-wide-operator-v2

6e47765

ukclivecox changed the title ~~Cluster wide operator for V2~~ feat[operator]: Cluster wide operator for V2 Jun 7, 2023

Merge branch 'v2' into cluster-wide-operator-v2

f3ef257

sakoush reviewed Jun 8, 2023

View reviewed changes

sakoush reviewed Jun 9, 2023

View reviewed changes

ukclivecox added 2 commits June 13, 2023 18:37

Merge branch 'v2' into cluster-wide-operator-v2

6bf194c

Review fixes

e5df737

ukclivecox changed the title ~~feat[operator]: Cluster wide operator for V2~~ feat(operator): Cluster wide operator for V2 Jun 14, 2023

ukclivecox added 3 commits June 15, 2023 06:25

allow Role/RoleBIndings when non clusterwide

bab43b7

Update docs for cluster wide helm usage

26bd848

ensure clusterRoleBindinf is excluded and add namespace to helm templ…

d86adfe

…ate commands

sakoush approved these changes Jun 15, 2023

View reviewed changes

Merge branch 'v2' into cluster-wide-operator-v2

be6c822

ukclivecox mentioned this pull request Jun 17, 2023

Make rclone config_secrets configurable in values.yaml #4915

Merged

Recreate servers helm chart with just the seldon servers in it and fu…

ede8567

…rther docs for helm and new docs for upgrading

sakoush reviewed Jun 19, 2023

View reviewed changes

ukclivecox and others added 4 commits June 19, 2023 09:46

regenerate static yaml

c1e5217

Merge branch 'v2' into cluster-wide-operator-v2

bc2198b

remove namespace from static yamls

b201034

Update docs/source/contents/upgrading/index.md

db74a15

Co-authored-by: Sherif Akoush <sherif.akoush@gmail.com>

sakoush approved these changes Jun 19, 2023

View reviewed changes

ukclivecox merged commit 2391429 into SeldonIO:v2 Jun 19, 2023

Kolajik mentioned this pull request Jun 23, 2023

Core v2 deployment without dataflow-engine is not possible #4939

Closed

		DataflowEngineName = "dataflow-engine"
		HodometerName = "hodometer"

		@@ -4,7 +4,7 @@ We provide several Helm charts.

		* `seldon-core-v2-crds` : cluster wide install of custom resources

feat(operator): Cluster wide operator for V2 #4847

feat(operator): Cluster wide operator for V2 #4847

Conversation

ukclivecox commented May 17, 2023 • edited Loading

ImagePullSecrets

review-notebook-app bot commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakoush left a comment

Choose a reason for hiding this comment

ukclivecox commented May 17, 2023 •

edited

Loading