Feat: Add PVC storage support #267

chinhuang007 · 2022-10-24T16:17:37Z

Motivation

Support PVC as a storage option, to address issue #230

Modifications

Add PVCs to deployment
Add secret watch to ServingRuntime controller to update deployment
Add logic to ServingRuntime controller to create PVCs for predictors if the global setting is true
Create volumes and PVCs based on deployment for ServingRuntimes
Update Predictor controller to validate for PVCs

Result

PVCs are read from the storage secret and created for all ServingRuntimes
PVCs are read from the predictor spec and created for all ServingRuntimes if the global setting is true

Related Issues

Resolves #230

kserve-oss-bot · 2022-10-24T16:17:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chinhuang007
To complete the pull request process, please assign njhill after the PR has been reviewed.
You can assign the PR to them by writing /assign @njhill in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

njhill

Thanks @chinhuang007, please see inline comments.

njhill · 2022-11-11T21:06:25Z

controllers/modelmesh/runtime.go

+		volumeMounts = append([]corev1.VolumeMount{
+			{
+				Name:      pvcName,
+				MountPath: PVCRootDir + "/" + pvcName,
+				ReadOnly:  true,
+			},
+		}, volumeMounts...)


Can be simplified:

Suggested change

volumeMounts = append([]corev1.VolumeMount{

{

Name: pvcName,

MountPath: PVCRootDir + "/" + pvcName,

ReadOnly: true,

},

}, volumeMounts...)

volumeMounts = append(volumeMounts, corev1.VolumeMount{

Name: pvcName,

MountPath: PVCRootDir + "/" + pvcName,

ReadOnly: true,

})

njhill · 2022-11-11T21:07:53Z

controllers/modelmesh/modelmesh.go

@@ -53,6 +53,7 @@ type Deployment struct {
 	RESTProxyImage     string
 	RESTProxyResources *corev1.ResourceRequirements
 	RESTProxyPort      uint16
+	PVCs               map[string]struct{}


I think this could just be a []string?

njhill · 2022-11-11T21:34:19Z

controllers/predictor_controller.go

+		s := &corev1.Secret{}
+		if err := pr.Client.Get(context.TODO(), types.NamespacedName{
+			Name:      modelmesh.StorageSecretName,
+			Namespace: predictor.Namespace,
+		}, s); err != nil {
+			return "Could not get the storage secret"
+		}
+		var storageConfig map[string]string
+		for _, storageData := range s.Data {
+			if err := json.Unmarshal(storageData, &storageConfig); err != nil {
+				return "Could not parse storage configuration json"
+			}
+			if storageConfig["type"] == StoragePVCType && storageConfig["name"] == storage.PersistentVolumeClaim.ClaimName {
+				return ""
+			}
+		}
+
+		return "PersistentVolumeClaim name is not in the storage config secret"


We don't want this here. That field in the CRD is deprecated, we decided against "hard coded" storage types so it will just be included as the "type" parameter in the StorageSpec.

We could change the returned error message however to indicate this e.g. "spec.storage.PersistentVolumeClaim is not supported, use 'pvc' type storage in the storageSpec instead"

njhill · 2022-11-11T21:37:10Z

controllers/modelmesh/puller.go

+			{
+				Name:      PVCMount,
+				MountPath: PullerPVCPath,
+				ReadOnly:  true,
+			},


As discussed, we should add all the same PVC mounts here as done for the other containers below. Will need to add a new arg to this func and the one that calls it to pass through the list of PVCs.

njhill · 2022-11-11T23:28:46Z

controllers/modelmesh/const.go

@@ -29,6 +29,7 @@ const (
 	SocketVolume    = "domain-socket"

 	ConfigStorageMount = "storage-config"
+	PVCMount           = "pvc-mount"


I don't think this is needed.

njhill · 2022-11-11T23:46:03Z

main.go

+		// default is true
+		envVarVal, _ := os.LookupEnv(envVar)
+		if envVarVal != FalseString {
+			err = cl.Get(context.Background(), client.ObjectKey{Name: "foo", Namespace: ControllerNamespace}, resourceObject)


Might be best to just check doing a Get on a secret called storage-config

njhill · 2022-11-11T23:47:17Z

pkg/predictor_source/inferenceservice_registry.go

+	if parameters["type"] == pvcStorageType {
+		p.Spec.Storage.PersistentVolumeClaim = &corev1.PersistentVolumeClaimVolumeSource{}
+		p.Spec.Storage.PersistentVolumeClaim.ClaimName = parameters["name"]
+		p.Spec.Storage.PersistentVolumeClaim.ReadOnly = true
+	}


Remove this per previous comment

njhill · 2022-11-11T23:47:41Z

pkg/predictor_source/inferenceservice_registry.go

+		case pvcStorageType:
+			modelPath = strings.TrimPrefix(u.Path, "/")
+			uriParameters["type"] = pvcStorageType
+			uriParameters["name"] = u.Host


njhill · 2022-11-11T23:48:06Z

pkg/predictor_source/inferenceservice_registry.go

@@ -39,6 +40,7 @@ const (
 	runtimeAnnotation    = "serving.kserve.io/servingRuntime"

 	azureBlobHostSuffix = "blob.core.windows.net"
+	pvcStorageType      = "pvc"


I don't think a constant is needed here, best to keep it consistent with the others.

njhill · 2022-11-11T23:50:23Z

controllers/servingruntime_controller.go

@@ -623,3 +676,16 @@ func (r *ServingRuntimeReconciler) clusterServingRuntimeRequests(csr *kserveapi.

 	return requests
 }
+
+func (r *ServingRuntimeReconciler) secretRequests(secret *corev1.Secret, namespace string) []reconcile.Request {


Rename to storageSecretRequests? (otherwise a bit generic)

Also don't need a separate namespace arg, can just get it from the secret inside this func.

chinhuang007 · 2022-11-19T23:07:28Z

@njhill thanks for the detailed review as always! Just made a few changes so please take a look again.

njhill

Thanks @chinhuang007 it's getting closer :)

One of the things is nontrivial and I'll aim to add some more detail for that.

njhill · 2022-11-24T00:10:45Z

controllers/servingruntime_controller.go

+	}
+	// add pvcs for predictors when the global config is enabled
+	if r.DeployPVCForPredictor {
+		list := &v1beta1.InferenceServiceList{}


I think we still need to change this per prior comment, to support different predictor "sources" (could be predictor or inferencservice). I will aim to write an example of what I mean.

@chinhuang007 here is what I was thinking of, sorry again for the delay:

if cfg.AllowAnyPVC { restProxyEnabled := cfg.RESTProxy.Enabled f := func(p *api.Predictor) bool { if runtimeSupportsPredictor(rt, p, restProxyEnabled, req.Name) && p.Spec.Storage != nil && p.Spec.Storage.Parameters != nil { params := *p.Spec.Storage.Parameters if stype := params["type"]; stype == "pvc" { if name := params["name"]; name != "" { pvcsMap[name] = struct{}{} } } } return false } for _, pr := range r.RegistryMap { if found, err := pr.Find(ctx, req.Namespace, f); found || err != nil { return nil, err } } }

njhill · 2022-11-24T00:13:45Z

controllers/predictor_controller.go

+				if err := json.Unmarshal(storageData, &storageConfig); err != nil {
+					return "Could not parse storage configuration json"
+				}
+				if storageConfig["type"] == StoragePVCType && storageConfig["name"] == parameters["name"] {


We should check for the existence of the "name" key in both cases... with a wrong configuration this could panic.

njhill · 2022-11-24T00:15:14Z

controllers/predictor_controller.go

@@ -239,6 +240,33 @@ func validatePredictor(predictor *api.Predictor) string {
 		return "Only one of spec.schemaPath and spec.storage.schemaPath can be specified"
 	}

+	if storage.StorageSpec.Parameters != nil {


I actually think we can probably omit this whole section. If the PVC isn't present it will cause the predictor load to fail (with an informative message from the puller).

njhill · 2022-11-24T00:17:33Z

main.go

+		ClusterScope:          clusterScopeMode,
+		EnableCSRWatch:        enableCSRWatch,
+		EnableSecretWatch:     enableSecretWatch,
+		DeployPVCForPredictor: conf.EnableDeployPVCForPredictor,


Getting the value here means it won't be dynamic. We can just grab it from the config where we use it.

njhill · 2022-11-24T00:19:59Z

controllers/servingruntime_controller.go

+		}
+	}
+	// add pvcs for predictors when the global config is enabled
+	if r.DeployPVCForPredictor {


Per other comment this doesn't need to be a reconciler field, just pass cfg into this function and get the value from there.

njhill · 2022-11-24T00:22:14Z

pkg/config/config.go

-	ModelMeshEndpoint string // For dev use only
+	EtcdSecretName              string // DEPRECATED - should be removed in the future
+	ModelMeshEndpoint           string // For dev use only
+	EnableDeployPVCForPredictor bool


I think we should name this differently, how about AllowAnyPVC?

We should also add to the docs ... the config params table and also in the storage part of the docs explain how this parameter works.

njhill · 2022-11-29T20:37:18Z

controllers/servingruntime_controller.go

+			if _, exists := storageConfig["name"]; !exists {
+				return nil, fmt.Errorf("Missing PVC name in storage configuration")
+			}
+			pvcsMap[storageConfig["name"]] = struct{}{}


~~Can avoid double lookup:~~ Edit: ignore this, see follow-on comment

Suggested change

if _, exists := storageConfig["name"]; !exists {

return nil, fmt.Errorf("Missing PVC name in storage configuration")

}

pvcsMap[storageConfig["name"]] = struct{}{}

if name, exists := storageConfig["name"]; exists {

pvcsMap[name] = struct{}{}

} else {

return nil, fmt.Errorf("Missing PVC name in storage configuration")

}

Actually on second thoughts, simplest would just be this. I also think we shouldn't return a failure for this, best to just log a warning.

Suggested change

if _, exists := storageConfig["name"]; !exists {

return nil, fmt.Errorf("Missing PVC name in storage configuration")

}

pvcsMap[storageConfig["name"]] = struct{}{}

if name := storageConfig["name"]; name != "" {

pvcsMap[name] = struct{}{}

} else {

// Log warning here

}

njhill · 2022-11-29T20:41:21Z

controllers/servingruntime_controller.go

+		}
+	}
+	// add pvcs for predictors when the global config is enabled
+	if r.ConfigProvider.GetConfig().AllowAnyPVC {


So that we use a consistent snapshot of the config for the whole reconciliation, it would be better to pass the already retrieved config struct into this function instead of getting via the provider. Can just add a cfg arg to getPVCs().

njhill · 2022-11-30T17:01:14Z

pkg/predictor_source/inferenceservice_registry.go

-		if p != nil && predicate(p) {
+		inferenceService := &list.Items[i]
+		nname := types.NamespacedName{Name: inferenceService.Name, Namespace: inferenceService.Namespace}
+		p, err := BuildBasePredictorFromInferenceService(inferenceService)


@chinhuang007 we should probably check for p == nil here like before.

Add PVC support according to the design and discussions captured in the issue, kserve#230 Signed-off-by: Chin Huang <[email protected]>

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

fOO223Fr · 2023-01-10T16:40:10Z

@pvaneck @ckadner is there any update on this Feature? this would be a great addition for our use case. Thank you

njhill · 2023-01-18T23:15:07Z

@fOO223Fr yes this is the next thing we are focused on completing. It didn't make the 0.10 release but is mostly complete. The main thing remaining to do is some tests, which @ckadner is working on.

Jooho · 2023-02-07T19:45:35Z

controllers/servingruntime_controller.go

+	s := &corev1.Secret{}
+	if err := r.Client.Get(ctx, types.NamespacedName{
+		Name:      modelmesh.StorageSecretName,
+		Namespace: req.Namespace,
+	}, s); err != nil {
+		return nil, fmt.Errorf("Could not get the storage secret: %w", err)
+	}


To use PVC, why do we need to check StorageSecretName The default "storage-config" secret usually has credentials for accessing cloud storage such as S3. However, PVC does not need credentials to attach to the pod.
With this code block, the controller keeps checking if the secret exists in a namespace. So although it is not needed, it must exist in the namespace to deploy an inference pod.

This goes back to some of the design decisions for the implementation, i.e. ...

#230 (comment)

The controller will watch the storage config secret in each namespace and keep the pvcs mounted to runtime deployments based on any pvc type entries
Predictors can always reference one of these PVCs for storage

#230 (comment)

The entry type will be checked and the name used to create volumes and volume mounts for runtime deployments, while the keys are irrelevant.

so even though users want to use PVC only, creating storage-config Secret with dummy data is required, am I right?

If you use the allowAnyPVC config parameter, and don't want to deploy the minio-examples image, I suppose you would not need it
Certainly it should not be required to create it with dummy data, so I think we should tolerate it not being there. @njhill

@ckadner yes as discussed offline we should change it so that in that mode the storage-config secret isn't required.

njhill

Thanks @chinhuang007 @ckadner!

njhill · 2023-03-01T09:56:04Z

/lgtm

Related: kserve#230, kserve#267 Signed-off-by: Christian Kadner <[email protected]>

* Add PVC support Add PVC support according to the design and discussions captured in the issue, kserve#230 Signed-off-by: Chin Huang <[email protected]> * add predictor controller login Signed-off-by: Chin Huang <[email protected]> * code restructure, cleanup based on review Signed-off-by: Chin Huang <[email protected]> * fix addPullerSidecar to include all pvcs Signed-off-by: Chin Huang <[email protected]> * restructure and simplify code, use global configmap rather than env var Signed-off-by: Chin Huang <[email protected]> * make AllowAnyPVC dynamic, update docs Signed-off-by: Chin Huang <[email protected]> * add runtimeSupportsPredictor check Signed-off-by: Chin Huang <[email protected]> * use PredictorRegistry and add storage to find() Signed-off-by: Chin Huang <[email protected]> --------- Signed-off-by: Chin Huang <[email protected]>

Motivation Address PVC follow-up work items outlined in #337 for PVC storage introduced in #267 Modifications Code changes: - Sort PVC mounts on serving runtime specs to avoid unstable repeated runtime rollouts as Kubernetes treat two otherwise identical deployment specs as different if the same set of volume mounts are in different order - Don't add non-existent PVCs from predictor/ISVC when allowAnyPVC is enabled as this would cause all serving pods for that runtime to stay in - Pending state with unbound (pending) volumes - Tolerate missing storage-config secret when allowAnyPVC is enabled - Lint: fix "io/ioutil" deprecations FVT changes: - Add Storage test suite - Add helper methods to add PVC to storage-config during FVT - Allow for additional time in WaitForReadyDeployStatus but allow early abort on success - Check if pod still running before gRPC/REST requests, reconnect if necessary - Only choose "Ready" runtime pod for port-forwards - Include ISVC tests in Predictor test suite to ensure "serial" execution of TLS tests Resolves #337 Signed-off-by: Christian Kadner <[email protected]>

Motivation Address PVC follow-up work items outlined in kserve#337 for PVC storage introduced in opendatahub-io#267 Modifications Code changes: - Sort PVC mounts on serving runtime specs to avoid unstable repeated runtime rollouts as Kubernetes treat two otherwise identical deployment specs as different if the same set of volume mounts are in different order - Don't add non-existent PVCs from predictor/ISVC when allowAnyPVC is enabled as this would cause all serving pods for that runtime to stay in - Pending state with unbound (pending) volumes - Tolerate missing storage-config secret when allowAnyPVC is enabled - Lint: fix "io/ioutil" deprecations FVT changes: - Add Storage test suite - Add helper methods to add PVC to storage-config during FVT - Allow for additional time in WaitForReadyDeployStatus but allow early abort on success - Check if pod still running before gRPC/REST requests, reconnect if necessary - Only choose "Ready" runtime pod for port-forwards - Include ISVC tests in Predictor test suite to ensure "serial" execution of TLS tests Resolves kserve#337 Signed-off-by: Christian Kadner <[email protected]>

kserve-oss-bot requested review from ckadner and pvaneck October 24, 2022 16:17

chinhuang007 marked this pull request as draft October 24, 2022 16:17

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

chinhuang007 changed the title ~~Add pvc~~ (WIP) Add pvc Oct 24, 2022

chinhuang007 force-pushed the add-pvc branch from 50661b7 to 26468e7 Compare October 24, 2022 17:03

chinhuang007 force-pushed the add-pvc branch from 26468e7 to e6b8d8b Compare November 3, 2022 21:26

njhill reviewed Nov 11, 2022

View reviewed changes

njhill changed the title ~~(WIP) Add pvc~~ Feat: Add PVC storage support Nov 12, 2022

chinhuang007 force-pushed the add-pvc branch 6 times, most recently from 457c64e to ea63c61 Compare November 18, 2022 21:24

chinhuang007 marked this pull request as ready for review November 19, 2022 00:52

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

njhill reviewed Nov 24, 2022

View reviewed changes

chinhuang007 force-pushed the add-pvc branch from cdf26b6 to 080d438 Compare November 28, 2022 22:10

njhill reviewed Nov 29, 2022

View reviewed changes

chinhuang007 force-pushed the add-pvc branch from e7d58bf to dc49a3b Compare November 30, 2022 07:15

njhill reviewed Nov 30, 2022

View reviewed changes

chinhuang007 force-pushed the add-pvc branch from dc49a3b to 75c0303 Compare November 30, 2022 18:11

chinhuang007 added 5 commits December 14, 2022 12:09

Add PVC support

b81d5f9

Add PVC support according to the design and discussions captured in the issue, kserve#230 Signed-off-by: Chin Huang <[email protected]>

add predictor controller login

b75ba47

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

code restructure, cleanup based on review

6ced006

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

fix addPullerSidecar to include all pvcs

7577c3f

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

restructure and simplify code, use global configmap rather than env var

2ae47a4

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

chinhuang007 added 3 commits December 14, 2022 12:09

make AllowAnyPVC dynamic, update docs

5bce95a

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

add runtimeSupportsPredictor check

62db4b0

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

use PredictorRegistry and add storage to find()

537344b

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

chinhuang007 force-pushed the add-pvc branch from 75c0303 to 537344b Compare December 14, 2022 20:11

This was referenced Jan 31, 2023

PVC storage support #230

Closed

feat: Add pullman PVC storage provider kserve/modelmesh-runtime-adapter#36

Merged

Jooho reviewed Feb 7, 2023

View reviewed changes

ckadner self-assigned this Feb 16, 2023

ckadner added this to the v0.11.0 milestone Feb 24, 2023

njhill approved these changes Mar 1, 2023

View reviewed changes

njhill merged commit f794ffc into kserve:main Mar 1, 2023

ckadner mentioned this pull request Mar 1, 2023

PVC support follow up items #337

Closed

6 tasks

ckadner added a commit to ckadner/modelmesh-serving that referenced this pull request Mar 6, 2023

test: Add FVT tests for PVC storage

72d0e81

Related: kserve#230, kserve#267 Signed-off-by: Christian Kadner <[email protected]>

ckadner mentioned this pull request Mar 6, 2023

Bug fixes and E2E tests for PVC storage #340

Merged

ckadner added a commit to ckadner/modelmesh-serving that referenced this pull request Mar 6, 2023

test: Add FVT tests for PVC storage

d3f4f92

Related: kserve#230, kserve#267 Signed-off-by: Christian Kadner <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add PVC storage support #267

Feat: Add PVC storage support #267

chinhuang007 commented Oct 24, 2022 •

edited by ckadner

Loading

kserve-oss-bot commented Oct 24, 2022

njhill left a comment

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

njhill Nov 11, 2022

chinhuang007 commented Nov 19, 2022

njhill left a comment

njhill Nov 24, 2022

njhill Nov 30, 2022

njhill Nov 24, 2022

njhill Nov 24, 2022

njhill Nov 24, 2022

njhill Nov 24, 2022

njhill Nov 24, 2022

njhill Nov 29, 2022 •

edited

Loading

njhill Nov 30, 2022

njhill Nov 29, 2022

njhill Nov 30, 2022

fOO223Fr commented Jan 10, 2023

njhill commented Jan 18, 2023

Jooho Feb 7, 2023

ckadner Feb 7, 2023

Jooho Feb 7, 2023

ckadner Feb 7, 2023

njhill Mar 1, 2023

njhill left a comment

njhill commented Mar 1, 2023

-			if _, exists := storageConfig["name"]; !exists {
-				return nil, fmt.Errorf("Missing PVC name in storage configuration")
-			}
-			pvcsMap[storageConfig["name"]] = struct{}{}
+			if name := storageConfig["name"]; name != "" {
+				pvcsMap[name] = struct{}{}
+			} else {
+				// Log warning here
+			}

Feat: Add PVC storage support #267

Feat: Add PVC storage support #267

Conversation

chinhuang007 commented Oct 24, 2022 • edited by ckadner Loading

Motivation

Modifications

Result

Related Issues

kserve-oss-bot commented Oct 24, 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

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 Nov 19, 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

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

njhill Nov 29, 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

fOO223Fr commented Jan 10, 2023

njhill commented Jan 18, 2023

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

njhill left a comment

Choose a reason for hiding this comment

njhill commented Mar 1, 2023

chinhuang007 commented Oct 24, 2022 •

edited by ckadner

Loading

njhill Nov 29, 2022 •

edited

Loading