Add Volumes to ServingRuntimePodSpec; allow other built-in ServerTypes (

kubeflow#2147)

* Add Volumes to ServingRuntimePodSpec; allow other built-in ServerTypes

Update unit tests to cover merging volumes, and improve environment variable merging.

Signed-off-by: Nick Hill <nickhill@us.ibm.com>

* Address some review comments

Signed-off-by: Nick Hill <nickhill@us.ibm.com>

* Change to use strategic merge patch from k8s apimachinery

Signed-off-by: Nick Hill <nickhill@us.ibm.com>

* Change container args merge behaviour to concatenate

Also rename a coreContainer var to mergedContainer.

Signed-off-by: Nick Hill <nickhill@us.ibm.com>

hack/violation_exceptions.list

@@ -1,6 +1,7 @@
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,BuiltInAdapter,Env
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,ServingRuntimePodSpec,Containers
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,ServingRuntimePodSpec,Tolerations
+API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,ServingRuntimePodSpec,Volumes
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,ServingRuntimeSpec,ProtocolVersions
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,ServingRuntimeSpec,SupportedModelFormats
 API rule violation: list_type_missing,./pkg/apis/serving/v1alpha1,TrainedModelList,Items

pkg/apis/serving/v1alpha1/servingruntime_types.go

@@ -54,6 +54,13 @@ type ServingRuntimePodSpec struct {
 	// +patchStrategy=merge
 	Containers []corev1.Container `json:"containers" patchStrategy:"merge" patchMergeKey:"name" validate:"required"`
 
+	// List of volumes that can be mounted by containers belonging to the pod.
+	// More info: https://kubernetes.io/docs/concepts/storage/volumes
+	// +optional
+	// +patchMergeKey=name
+	// +patchStrategy=merge,retainKeys
+	Volumes []corev1.Volume `json:"volumes,omitempty" patchStrategy:"merge,retainKeys" patchMergeKey:"name" protobuf:"bytes,1,rep,name=volumes"`
+
 	// NodeSelector is a selector which must be true for the pod to fit on a node.
 	// Selector which must match a node's labels for the pod to be scheduled on that node.
 	// More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
@@ -131,20 +138,22 @@ type ServingRuntimeStatus struct {
 
 // ServerType constant for specifying the runtime name
 // +k8s:openapi-gen=true
-// +kubebuilder:validation:Enum=triton;mlserver
 type ServerType string
 
-// ServerType Enum
+// Built-in ServerTypes (others may be supported)
 const (
 	// Model server is Triton
 	Triton ServerType = "triton"
 	// Model server is MLServer
 	MLServer ServerType = "mlserver"
+	// Model server is OpenVino Model Server
+	OVMS ServerType = "ovms"
 )
 
 // +k8s:openapi-gen=true
 type BuiltInAdapter struct {
-	// ServerType can be one of triton/mlserver and the runtime's container must have the same name
+	// ServerType must be one of the supported built-in types such as "triton" or "mlserver",
+	// and the runtime's container must have the same name
 	ServerType ServerType `json:"serverType,omitempty"`
 	// Port which the runtime server listens for model management requests
 	RuntimeManagementPort int `json:"runtimeManagementPort,omitempty"`

pkg/apis/serving/v1beta1/swagger.json

@@ -34,7 +34,7 @@
           "format": "int32"
         },
         "serverType": {
-          "description": "ServerType can be one of triton/mlserver and the runtime's container must have the same name",
+          "description": "ServerType must be one of the supported built-in types such as \"triton\" or \"mlserver\", and the runtime's container must have the same name",
           "type": "string"
         }
       }
@@ -208,6 +208,16 @@
             "default": {},
             "$ref": "#/definitions/v1.Toleration"
           }
+        },
+        "volumes": {
+          "description": "List of volumes that can be mounted by containers belonging to the pod. More info: https://kubernetes.io/docs/concepts/storage/volumes",
+          "type": "array",
+          "items": {
+            "default": {},
+            "$ref": "#/definitions/v1.Volume"
+          },
+          "x-kubernetes-patch-merge-key": "name",
+          "x-kubernetes-patch-strategy": "merge,retainKeys"
         }
       }
     },
@@ -296,6 +306,16 @@
             "default": {},
             "$ref": "#/definitions/v1.Toleration"
           }
+        },
+        "volumes": {
+          "description": "List of volumes that can be mounted by containers belonging to the pod. More info: https://kubernetes.io/docs/concepts/storage/volumes",
+          "type": "array",
+          "items": {
+            "default": {},
+            "$ref": "#/definitions/v1.Volume"
+          },
+          "x-kubernetes-patch-merge-key": "name",
+          "x-kubernetes-patch-strategy": "merge,retainKeys"
         }
       }
     },

pkg/controller/v1beta1/inferenceservice/utils/utils.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/json"
 	"html/template"
+	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"regexp"
 	"strings"
 
@@ -75,61 +76,53 @@ func GetDeploymentMode(annotations map[string]string, deployConfig *v1beta1api.D
 // MergeRuntimeContainers Merge the predictor Container struct with the runtime Container struct, allowing users
 // to override runtime container settings from the predictor spec.
 func MergeRuntimeContainers(runtimeContainer *v1.Container, predictorContainer *v1.Container) (*v1.Container, error) {
-	// Default container configuration from the runtime.
-	coreContainer := v1.Container{
-		Args:            runtimeContainer.Args,
-		Command:         runtimeContainer.Command,
-		Env:             runtimeContainer.Env,
-		Image:           runtimeContainer.Image,
-		Name:            runtimeContainer.Name,
-		Resources:       runtimeContainer.Resources,
-		ImagePullPolicy: runtimeContainer.ImagePullPolicy,
-		WorkingDir:      runtimeContainer.WorkingDir,
-		LivenessProbe:   runtimeContainer.LivenessProbe,
-	}
 	// Save runtime container name, as the name can be overridden as empty string during the Unmarshal below
 	// since the Name field does not have the 'omitempty' struct tag.
 	runtimeContainerName := runtimeContainer.Name
 
-	// Args and Env will be combined instead of overridden.
-	argCopy := make([]string, len(coreContainer.Args))
-	copy(argCopy, coreContainer.Args)
-
-	envCopy := make([]v1.EnvVar, len(coreContainer.Env))
-	copy(envCopy, coreContainer.Env)
+	// Use JSON Marshal/Unmarshal to merge Container structs using strategic merge patch
+	runtimeContainerJson, err := json.Marshal(runtimeContainer)
+	if err != nil {
+		return nil, err
+	}
 
-	// Use JSON Marshal/Unmarshal to merge Container structs.
 	overrides, err := json.Marshal(predictorContainer)
 	if err != nil {
 		return nil, err
 	}
 
-	if err := json.Unmarshal(overrides, &coreContainer); err != nil {
+	mergedContainer := v1.Container{}
+	jsonResult, err := strategicpatch.StrategicMergePatch(runtimeContainerJson, overrides, mergedContainer)
+	if err != nil {
 		return nil, err
 	}
 
-	if coreContainer.Name == "" {
-		coreContainer.Name = runtimeContainerName
+	if err := json.Unmarshal(jsonResult, &mergedContainer); err != nil {
+		return nil, err
 	}
 
-	argCopy = append(argCopy, predictorContainer.Args...)
-	envCopy = append(envCopy, predictorContainer.Env...)
-
-	coreContainer.Args = argCopy
-	coreContainer.Env = envCopy
+	if mergedContainer.Name == "" {
+		mergedContainer.Name = runtimeContainerName
+	}
 
-	return &coreContainer, nil
+	// Strategic merge patch will replace args but more useful behaviour here is to concatenate
+	mergedContainer.Args = append(append([]string{}, runtimeContainer.Args...), predictorContainer.Args...)
 
+	return &mergedContainer, nil
 }
 
 // MergePodSpec Merge the predictor PodSpec struct with the runtime PodSpec struct, allowing users
 // to override runtime PodSpec settings from the predictor spec.
 func MergePodSpec(runtimePodSpec *v1alpha1.ServingRuntimePodSpec, predictorPodSpec *v1beta1.PodSpec) (*v1.PodSpec, error) {
 
-	corePodSpec := v1.PodSpec{
+	runtimePodSpecJson, err := json.Marshal(v1.PodSpec{
 		NodeSelector: runtimePodSpec.NodeSelector,
 		Affinity:     runtimePodSpec.Affinity,
 		Tolerations:  runtimePodSpec.Tolerations,
+		Volumes:      runtimePodSpec.Volumes,
+	})
+	if err != nil {
+		return nil, err
 	}
 
 	// Use JSON Marshal/Unmarshal to merge PodSpec structs.
@@ -138,12 +131,17 @@ func MergePodSpec(runtimePodSpec *v1alpha1.ServingRuntimePodSpec, predictorPodSp
 		return nil, err
 	}
 
-	if err := json.Unmarshal(overrides, &corePodSpec); err != nil {
+	corePodSpec := v1.PodSpec{}
+	jsonResult, err := strategicpatch.StrategicMergePatch(runtimePodSpecJson, overrides, corePodSpec)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := json.Unmarshal(jsonResult, &corePodSpec); err != nil {
 		return nil, err
 	}
 
 	return &corePodSpec, nil
-
 }
 
 // GetServingRuntime Get a ServingRuntime by name. First, ServingRuntimes in the given namespace will be checked.