Merge pull request #5 from gmfrasca/dspv2-merge-fixmlmd

Require MLMD to be deployed when using V2 Pipelines
opendatahub-io · Jan 9, 2024 · 6509826 · 6509826
2 parents ae3f49b + 7979f2c
commit 6509826
Show file tree

Hide file tree

Showing 9 changed files with 16 additions and 23 deletions.
diff --git a/api/v1alpha1/dspipeline_types.go b/api/v1alpha1/dspipeline_types.go
@@ -40,8 +40,7 @@ type DSPASpec struct {
 	// ObjectStorage specifies Object Store configurations, used for DS Pipelines artifact passing and storage. Specify either the your own External Storage (e.g. AWS S3), or use the default Minio deployment (unsupported, primarily for development, and testing) .
 	// +kubebuilder:validation:Required
 	*ObjectStorage `json:"objectStorage"`
-	// +kubebuilder:default:={deploy: true}
-	*MLMD `json:"mlmd,omitempty"`
+	*MLMD          `json:"mlmd,omitempty"`
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default:={deploy: false}
 	*CRDViewer `json:"crdviewer"`
@@ -261,7 +260,7 @@ type Minio struct {
 
 type MLMD struct {
 	// Enable DS Pipelines Operator management of MLMD. Setting Deploy to false disables operator reconciliation. Default: true
-	// +kubebuilder:default:=true
+	// +kubebuilder:default:=false
 	// +kubebuilder:validation:Optional
 	Deploy  bool `json:"deploy"`
 	*Envoy  `json:"envoy,omitempty"`

diff --git a/...ses/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml b/...ses/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml
@@ -305,11 +305,9 @@ spec:
                 default: v1
                 type: string
               mlmd:
-                default:
-                  deploy: true
                 properties:
                   deploy:
-                    default: true
+                    default: false
                     description: 'Enable DS Pipelines Operator management of MLMD.
                       Setting Deploy to false disables operator reconciliation. Default:
                       true'

diff --git a/config/internal/apiserver/default/deployment.yaml.tmpl b/config/internal/apiserver/default/deployment.yaml.tmpl
@@ -84,6 +84,7 @@ spec:
             - name: MOVERESULTS_IMAGE
               value: "{{.APIServer.MoveResultsImage}}"
             ## Env Vars to only include if MLMD Deployed ##
+            {{ if .MLMD }}
             {{ if .MLMD.Deploy }}
             - name: METADATA_GRPC_SERVICE_SERVICE_HOST
               value: "ds-pipeline-metadata-grpc-{{.Name}}.{{.Namespace}}.svc.cluster.local"
@@ -92,6 +93,7 @@ spec:
               value: "{{.MLMD.GRPC.Port}}"
             {{ end }}
             {{ end }}
+            {{ end }}
             - name: ML_PIPELINE_SERVICE_HOST
               value: "ds-pipeline-{{.Name}}.{{.Namespace}}.svc.cluster.local"
             - name: ML_PIPELINE_SERVICE_PORT_GRPC

diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go
@@ -384,6 +384,16 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc
 }
 
 func (p *DSPAParams) SetupMLMD(ctx context.Context, dsp *dspa.DataSciencePipelinesApplication, client client.Client, log logr.Logger) error {
+	if p.UsingV2Pipelines(dsp) {
+		if p.MLMD == nil {
+			log.Info("MLMD not specified, but is a required component for V2 Pipelines. Including MLMD with default specs.")
+			p.MLMD = &dspa.MLMD{
+				Deploy: true,
+			}
+		} else {
+			return fmt.Errorf("MLMD explicitly disabled in DSPA, but is a required component for V2 Pipelines")
+		}
+	}
 	if p.MLMD != nil {
 		MlmdEnvoyImagePath := p.GetImageForComponent(dsp, config.MlmdEnvoyImagePath, config.MlmdEnvoyImagePathV2Argo, config.MlmdEnvoyImagePathV2Tekton)
 		MlmdGRPCImagePath := p.GetImageForComponent(dsp, config.MlmdGRPCImagePath, config.MlmdGRPCImagePathV2Argo, config.MlmdGRPCImagePathV2Tekton)

diff --git a/controllers/mlmd.go b/controllers/mlmd.go
@@ -26,7 +26,7 @@ func (r *DSPAReconciler) ReconcileMLMD(dsp *dspav1alpha1.DataSciencePipelinesApp
 
 	log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name)
 
-	if !dsp.Spec.MLMD.Deploy {
+	if (params.MLMD == nil || !params.MLMD.Deploy) && (dsp.Spec.MLMD == nil || !dsp.Spec.MLMD.Deploy) {
 		r.Log.Info("Skipping Application of ML-Metadata (MLMD) Resources")
 		return nil
 	}

diff --git a/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml
@@ -75,10 +75,6 @@ spec:
               value: "ubi-minimal:test0"
             - name: MOVERESULTS_IMAGE
               value: "busybox:test0"
-            - name: METADATA_GRPC_SERVICE_SERVICE_HOST
-              value: "ds-pipeline-metadata-grpc-testdsp0.default.svc.cluster.local"
-            - name: METADATA_GRPC_SERVICE_SERVICE_PORT
-              value: "8080"
             - name: ML_PIPELINE_SERVICE_HOST
               value: ds-pipeline-testdsp0.default.svc.cluster.local
             - name: ML_PIPELINE_SERVICE_PORT_GRPC

diff --git a/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml
@@ -75,10 +75,6 @@ spec:
               value: "ubi-minimal:test2"
             - name: MOVERESULTS_IMAGE
               value: "busybox:test2"
-            - name: METADATA_GRPC_SERVICE_SERVICE_HOST
-              value: "ds-pipeline-metadata-grpc-testdsp2.default.svc.cluster.local"
-            - name: METADATA_GRPC_SERVICE_SERVICE_PORT
-              value: "8080"
             - name: ML_PIPELINE_SERVICE_HOST
               value: ds-pipeline-testdsp2.default.svc.cluster.local
             - name: ML_PIPELINE_SERVICE_PORT_GRPC

diff --git a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml
@@ -75,10 +75,6 @@ spec:
               value: ubi-minimal:test3
             - name: MOVERESULTS_IMAGE
               value: busybox:test3
-            - name: METADATA_GRPC_SERVICE_SERVICE_HOST
-              value: "ds-pipeline-metadata-grpc-testdsp3.default.svc.cluster.local"
-            - name: METADATA_GRPC_SERVICE_SERVICE_PORT
-              value: "8080"
             - name: ML_PIPELINE_SERVICE_HOST
               value: ds-pipeline-testdsp3.default.svc.cluster.local
             - name: ML_PIPELINE_SERVICE_PORT_GRPC

diff --git a/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml
@@ -75,10 +75,6 @@ spec:
               value: "this-ubi-minimal-image-from-cr-should-be-used:test4"
             - name: MOVERESULTS_IMAGE
               value: "this-busybox-image-from-cr-should-be-used:test4"
-            - name: METADATA_GRPC_SERVICE_SERVICE_HOST
-              value: "ds-pipeline-metadata-grpc-testdsp4.default.svc.cluster.local"
-            - name: METADATA_GRPC_SERVICE_SERVICE_PORT
-              value: "8080"
             - name: ML_PIPELINE_SERVICE_HOST
               value: ds-pipeline-testdsp4.default.svc.cluster.local
             - name: ML_PIPELINE_SERVICE_PORT_GRPC