feat: Update install to support ClusterServingRuntime #252

chinhuang007 · 2022-09-29T18:22:04Z

Update install and config to support ClusterServingRuntime

update install script to setup SRs or CSRs
add rbac to access CSRs
add and update install yamls
update kserve ServingRuntimes and ClusterServingRuntimes yaml
update fvt to work with either cluster or namespace scope mode
update unit tests to work with updated yamls

We will need a separate PR to cover more FVT scenarios for the cluster scope mode with ClusterServingRuntimes

Signed-off-by: Chin Huang [email protected]

njhill

Thanks @chinhuang007 this looks pretty good, just a few comments.

njhill · 2022-10-03T18:14:34Z

controllers/servingruntime_controller.go

@@ -138,7 +138,7 @@ func (r *ServingRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Reque

 	// Delete etcd secret when there is no ServingRuntimes in a namespace
 	etcdSecretName := cfg.GetEtcdSecretName()
-	if len(runtimes.Items) == 0 {
+	if len(srSpecs) == 0 {


Good catch!

@chinhuang007 this is really a fix to the prior PR, we could just open a separate PR with just this and get it merged first.

njhill · 2022-10-04T00:04:35Z

fvt/predictor/predictor_suite_test.go

+	// confirm 3 serving runtimes exist in namespace scope mode
 	list, err := FVTClientInstance.ListServingRuntimes(metav1.ListOptions{})
 	Expect(err).ToNot(HaveOccurred())
-	Expect(list.Items).To(HaveLen(3))
+	if NameSpaceScopeMode {
+		Expect(list.Items).To(HaveLen(3))
+	} else {
+		Expect(list.Items).To(HaveLen(0))
+	}


For these, wouldn't it be better to list either SRs or CSRs depending on namespace/cluster mode? I.e. the list should still contain 3 in either case.

As mentioned in commit comments, I try to minimize the FVT changes to test CSRs, for which I suggest to create a new PR.

njhill · 2022-10-04T00:06:29Z

config/cluster-runtimes/kustomization.yaml

+    patch: |-
+      - op: replace
+        path: /kind
+        value: ClusterServingRuntime


If cluster scope is the default install mode, I'm wondering whether we should invert this - change the SRs in runtimes to be CSRs, and have this overlay change them to SRs for the namespace scope install case?

Again, due to existing FVTs and unit test depend on these to be SR, I try to minimize the changes. A separate PR should address your concern.

njhill · 2022-10-04T00:08:34Z

scripts/deploy/iks/test-fvt.sh

  fi
-


I don't completely understand the changes in this file, are they temporary?

Yes, this is temporarily to make the test only for the controller namespace in the namespace scope mode, until we have the FVTs all figured out for CSRs in cluster scope mode.

njhill · 2022-10-04T00:11:57Z

config/crd/kustomization.yaml

@@ -18,8 +18,7 @@ resources:
  # Including creation of Service here for now, will later be done automatically
  - bases/serving.kserve.io_predictors.yaml
  - bases/serving.kserve.io_inferenceservices.yaml
-  # ClusterServingRuntime not yet supported by modelmesh-serving
-  #  - bases/serving.kserve.io_clusterservingruntimes.yaml
+  - bases/serving.kserve.io_clusterservingruntimes.yaml


I'm wondering whether we should include this in namespace scope case.

Good question. Since we won't do anything with CSRs in namespace scope mode, probably can exclude it during install.

@chinhuang007 WDYT about excluding this for namespace mode? We could just have a separate cluster_crd kustomization which includes this one + the CSR CRD

chinhuang007 · 2022-10-10T23:23:21Z

@njhill I made some changes to address comments. Please take one more look.

njhill

Thanks @chinhuang007 it's looking pretty good, just a few more comments.

njhill · 2022-10-26T19:04:35Z

config/crd/kustomization.yaml

@@ -18,8 +18,7 @@ resources:
  # Including creation of Service here for now, will later be done automatically
  - bases/serving.kserve.io_predictors.yaml
  - bases/serving.kserve.io_inferenceservices.yaml
-  # ClusterServingRuntime not yet supported by modelmesh-serving
-  #  - bases/serving.kserve.io_clusterservingruntimes.yaml
+  - bases/serving.kserve.io_clusterservingruntimes.yaml


@chinhuang007 WDYT about excluding this for namespace mode? We could just have a separate cluster_crd kustomization which includes this one + the CSR CRD

njhill · 2022-10-26T19:17:18Z

controllers/servingruntime_controller_test.go

-		"config/runtimes/ovms-1.x.yaml",
+		"controllers/testdata/mlserver-0.x.yaml",
+		"controllers/testdata/triton-2.x.yaml",
+		"controllers/testdata/ovms-1.x.yaml",


I think it would be a lot cleaner for these to convert it from CSR to SR programmatically. Then we won't need to copy files around.

And it should be easy with manifestival, you can just have a top level function like

func convertToServingRuntime(resource *unstructured.Unstructured) error { if resource.GetKind() == "ClusterServingRuntime" { resource.SetKind("ServingRuntime") } return nil }

and then just add it to the existing Transform() call where each of the CRs are loaded, i.e.:

By("create the runtime") m, err = mf.ManifestFrom(mf.Path(filepath.Join("..", sampleFilename))) m.Client = mfc.NewClient(k8sClient) Expect(err).ToNot(HaveOccurred()) m, err = m.Transform(convertToServingRuntime, mf.InjectNamespace(namespace)) Expect(err).ToNot(HaveOccurred()) // ...

@chinhuang007 WDYT about excluding this for namespace mode? We could just have a separate cluster_crd kustomization which includes this one + the CSR CRD

I think having a separate cluster_crd kustomization will complicate the installation. Currently everything under crd folder is taken as a whole, as documented below. We would need to duplicate kustomization.yaml except one line and make a few other changes to make it to work. So I would suggest to stay with the current simple solution.

# This kustomization.yaml is not intended to be run by itself, # since it depends on service name and namespace that are out of this kustomize package. # It should be run by config/default

OK, it just feels like this is a bit more hacky/brittle.

fvt/predictor/predictor_suite_test.go

njhill · 2022-10-26T19:33:17Z

fvt/scaleToZero/scaleToZero_suite_test.go

+	if NameSpaceScopeMode {
+		f = FVTClientInstance.ListServingRuntimes
+	}
+	list, err := f(metav1.ListOptions{})


Same comment as above, except err is already declared here

njhill · 2022-10-26T19:36:34Z

scripts/deploy/iks/test-fvt.sh

@@ -62,6 +61,9 @@ run_fvt() {
    fi
  fi

+  if [[ $(grep "Test Suite Passed" fvt.out) ]]; then
+      REV=0
+  fi


I don't understand why this is added here?

njhill · 2022-10-26T19:37:17Z

scripts/install.sh

@@ -239,6 +239,7 @@ if [[ $delete == "true" ]]; then
  info "Deleting any previous ModelMesh Serving instances and older CRD with serving.kserve.io api group name"
  kubectl delete crd/predictors.serving.kserve.io --ignore-not-found=true
  kubectl delete crd/servingruntimes.serving.kserve.io --ignore-not-found=true
+  kubectl delete crd/clusterservingruntimes.serving.kserve.io --ignore-not-found=true


Should move this into the namespace_scope != true block below I think?

njhill · 2022-10-26T19:39:28Z

scripts/install.sh

@@ -296,6 +298,7 @@ fi
 if [[ $namespace_scope_mode == "true" ]]; then
  info "Enabling namespace scope mode"
  kubectl set env deploy/modelmesh-controller NAMESPACE_SCOPE=true
+  sed -i 's/#- bases\/serving.kserve.io_clusterservingruntimes.yaml/- bases\/serving.kserve.io_clusterservingruntimes.yaml/g' crd/kustomization.yaml


I don't think this is needed here, it's already done above?

This is to reset crd/kustomization.yaml back to original in case the user wants to delete the namespaced installation and deploy a cluster scoped installation using the same set of files.

Ah ok sorry I didn't notice that this was reverting.

njhill · 2022-10-26T19:41:23Z

scripts/install.sh

@@ -281,6 +282,7 @@ kubectl get secret storage-config
 info "Installing ModelMesh Serving RBACs (namespace_scope_mode=$namespace_scope_mode)"
 if [[ $namespace_scope_mode == "true" ]]; then
  kustomize build rbac/namespace-scope | kubectl apply -f -
+  sed -i 's/- bases\/serving.kserve.io_clusterservingruntimes.yaml/#- bases\/serving.kserve.io_clusterservingruntimes.yaml/g' crd/kustomization.yaml


Ah I see, you didn't make a separate kustomization for this. I guess this is ok. Maybe add a comment above this line saying that we won't install the CSR CRD in namespace mode?

scripts/install.sh

Update install and config to support ClusterServingRuntime - update install script to setup SRs or CSRs - add rbac to access CSRs - add and update install yamls - update kserve ServingRuntimes and ClusterServingRuntimes yaml - update fvt, to use the namespace scope mode for now - fix an issue in SR controller We will need a separate PR to cover FVTs for the cluster scope mode with ClusterServingRuntimes Signed-off-by: Chin Huang <[email protected]>

Signed-off-by: Chin Huang <[email protected]>

chinhuang007 · 2022-10-28T00:17:05Z

@njhill Thanks for the review. One more commit is pushed. Please take a look.

Signed-off-by: Nick Hill <[email protected]>

njhill

Thanks @chinhuang007, LGTM. I pushed a couple more small changes to avoid another review roundtrip :)

kserve-oss-bot · 2022-10-29T21:13:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chinhuang007, njhill

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

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

Signed-off-by: Nick Hill <[email protected]>

njhill · 2022-10-29T22:04:40Z

/lgtm

kserve-oss-bot requested review from animeshsingh and Tomcli September 29, 2022 18:22

chinhuang007 marked this pull request as draft September 29, 2022 18:22

kserve-oss-bot added the do-not-merge/work-in-progress label Sep 29, 2022

chinhuang007 force-pushed the install-csr branch 4 times, most recently from f4aca3f to 1949cff Compare September 30, 2022 21:38

chinhuang007 marked this pull request as ready for review September 30, 2022 22:07

kserve-oss-bot removed the do-not-merge/work-in-progress label Sep 30, 2022

njhill reviewed Oct 4, 2022

View reviewed changes

njhill marked this pull request as draft October 5, 2022 17:34

kserve-oss-bot added the do-not-merge/work-in-progress label Oct 5, 2022

chinhuang007 force-pushed the install-csr branch 2 times, most recently from 83c74df to 279d02a Compare October 10, 2022 22:30

chinhuang007 marked this pull request as ready for review October 10, 2022 23:15

kserve-oss-bot removed the do-not-merge/work-in-progress label Oct 10, 2022

chinhuang007 changed the title ~~Update install to support ClusterServingRuntime~~ feat: Update install to support ClusterServingRuntime Oct 10, 2022

njhill reviewed Oct 26, 2022

View reviewed changes

chinhuang007 added 6 commits October 27, 2022 09:05

switch to cluster scope for FVT

2c77451

Signed-off-by: Chin Huang <[email protected]>

make CSR as default runtimes

9ff1097

Signed-off-by: Chin Huang <[email protected]>

fix unit tests to use ServingRuntimes

8f3391f

Signed-off-by: Chin Huang <[email protected]>

make e2e test to run in cluster scope mode

e3ec525

Signed-off-by: Chin Huang <[email protected]>

update fvt and install to address comments

af0baee

Signed-off-by: Chin Huang <[email protected]>

chinhuang007 force-pushed the install-csr branch from 279d02a to af0baee Compare October 27, 2022 16:10

code update, restructure and cleanup based on review comments

8e4a032

Signed-off-by: Chin Huang <[email protected]>

A couple more minor changes from review

2319240

Signed-off-by: Nick Hill <[email protected]>

njhill approved these changes Oct 29, 2022

View reviewed changes

kserve-oss-bot added the approved label Oct 29, 2022

Linting fix 🤦

164e26a

Signed-off-by: Nick Hill <[email protected]>

kserve-oss-bot assigned njhill Oct 29, 2022

kserve-oss-bot added the lgtm label Oct 29, 2022

njhill merged commit 5e016de into kserve:main Oct 31, 2022

ckadner mentioned this pull request Jan 13, 2023

Update ModelMesh version to v0.10.0 kserve/kserve#2645

Merged

4 tasks

rafvasq mentioned this pull request Jan 19, 2024

ClusterServingRuntime support #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update install to support ClusterServingRuntime #252

feat: Update install to support ClusterServingRuntime #252

chinhuang007 commented Sep 29, 2022 •

edited by njhill

Loading

njhill left a comment

njhill Oct 3, 2022

njhill Oct 4, 2022

njhill Oct 4, 2022

chinhuang007 Oct 4, 2022

njhill Oct 4, 2022

chinhuang007 Oct 4, 2022

njhill Oct 4, 2022

chinhuang007 Oct 4, 2022

njhill Oct 4, 2022

chinhuang007 Oct 4, 2022

njhill Oct 26, 2022

chinhuang007 commented Oct 10, 2022

njhill left a comment

njhill Oct 26, 2022

njhill Oct 26, 2022

chinhuang007 Oct 27, 2022 •

edited

Loading

njhill Oct 29, 2022

njhill Oct 26, 2022

njhill Oct 26, 2022

njhill Oct 26, 2022

njhill Oct 26, 2022

chinhuang007 Oct 27, 2022

njhill Oct 28, 2022

njhill Oct 26, 2022

chinhuang007 commented Oct 28, 2022

njhill left a comment

kserve-oss-bot commented Oct 29, 2022

njhill commented Oct 29, 2022

feat: Update install to support ClusterServingRuntime #252

feat: Update install to support ClusterServingRuntime #252

Conversation

chinhuang007 commented Sep 29, 2022 • edited by njhill Loading

njhill 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

chinhuang007 commented Oct 10, 2022

njhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinhuang007 Oct 27, 2022 • edited Loading

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

chinhuang007 commented Oct 28, 2022

njhill left a comment

Choose a reason for hiding this comment

kserve-oss-bot commented Oct 29, 2022

njhill commented Oct 29, 2022

chinhuang007 commented Sep 29, 2022 •

edited by njhill

Loading

chinhuang007 Oct 27, 2022 •

edited

Loading