Add unit test for controller reconcile, fixes #17

opendatahub-io · Nov 7, 2023 · 4c04ff7 · 4c04ff7
1 parent 4263845
commit 4c04ff7
Show file tree

Hide file tree

Showing 7 changed files with 251 additions and 167 deletions.
diff --git a/internal/controller/config/defaults.go b/internal/controller/config/defaults.go
@@ -22,112 +22,18 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"text/template"
-	"time"
 )
 
 //go:embed templates/*.yaml.tmpl
 var templateFS embed.FS
 
 const (
-	DefaultImageValue = "MustSetInConfig"
-
-	MLPipelineUIConfigMapPrefix       = "ds-pipeline-ui-configmap-"
-	ArtifactScriptConfigMapNamePrefix = "ds-pipeline-artifact-script-"
-	ArtifactScriptConfigMapKey        = "artifact_script"
-	DSPServicePrefix                  = "ds-pipeline"
-
-	DBSecretNamePrefix = "ds-pipeline-db-"
-	DBSecretKey        = "password"
-
-	MariaDBName        = "mlpipeline"
-	MariaDBHostPrefix  = "mariadb"
-	MariaDBHostPort    = "3306"
-	MariaDBUser        = "mlpipeline"
-	MariaDBNamePVCSize = "10Gi"
-
-	MinioHostPrefix    = "minio"
-	MinioPort          = "9000"
-	MinioScheme        = "http"
-	MinioDefaultBucket = "mlpipeline"
-	MinioPVCSize       = "10Gi"
-
-	ObjectStorageSecretName = "mlpipeline-minio-artifact" // hardcoded in kfp-tekton
-	ObjectStorageAccessKey  = "accesskey"
-	ObjectStorageSecretKey  = "secretkey"
-
 	GrpcImage        = "GRPC_IMAGE"
 	RestImage        = "REST_IMAGE"
 	DefaultGrpcImage = "gcr.io/tfx-oss-public/ml_metadata_store_server:1.14.0"
 	DefaultRestImage = "quay.io/opendatahub/model-registry:latest"
 )
 
-// DSPO Config File Paths
-const (
-	APIServerImagePath            = "Images.ApiServer"
-	APIServerArtifactImagePath    = "Images.Artifact"
-	PersistenceAgentImagePath     = "Images.PersistentAgent"
-	ScheduledWorkflowImagePath    = "Images.ScheduledWorkflow"
-	APIServerCacheImagePath       = "Images.Cache"
-	APIServerMoveResultsImagePath = "Images.MoveResultsImage"
-	MariaDBImagePath              = "Images.MariaDB"
-	OAuthProxyImagePath           = "Images.OAuthProxy"
-	MlmdEnvoyImagePath            = "Images.MlmdEnvoy"
-	MlmdGRPCImagePath             = "Images.MlmdGRPC"
-	MlmdWriterImagePath           = "Images.MlmdWriter"
-)
-
-// ModelRegistry Status Condition Types
-const (
-	DatabaseAvailable      = "DatabaseAvailable"
-	ObjectStoreAvailable   = "ObjectStoreAvailable"
-	APIServerReady         = "APIServerReady"
-	PersistenceAgentReady  = "PersistenceAgentReady"
-	ScheduledWorkflowReady = "ScheduledWorkflowReady"
-	CrReady                = "Ready"
-)
-
-// ModelRegistry Ready Status Condition Reasons
-// As per k8s api convention: Reason is intended
-// to be used in concise output, such as one-line
-// kubectl get output, and in summarizing
-// occurrences of causes
-const (
-	MinimumReplicasAvailable    = "MinimumReplicasAvailable"
-	FailingToDeploy             = "FailingToDeploy"
-	Deploying                   = "Deploying"
-	ComponentDeploymentNotFound = "ComponentDeploymentNotFound"
-)
-
-// Any required Configmap paths can be added here,
-// they will be automatically included for required
-// validation check
-var requiredFields = []string{
-	APIServerImagePath,
-	APIServerArtifactImagePath,
-	PersistenceAgentImagePath,
-	ScheduledWorkflowImagePath,
-	APIServerCacheImagePath,
-	APIServerMoveResultsImagePath,
-	MariaDBImagePath,
-	OAuthProxyImagePath,
-}
-
-// DefaultDBConnectionTimeout is the default DB storage healthcheck timeout
-const DefaultDBConnectionTimeout = time.Second * 15
-
-// DefaultObjStoreConnectionTimeout is the default Object storage healthcheck timeout
-const DefaultObjStoreConnectionTimeout = time.Second * 15
-
-const DefaultMaxConcurrentReconciles = 10
-
-const MlmdGrpcPort = 8080
-
-const MlmdRestPort = 9090
-
-func GetConfigRequiredFields() []string {
-	return requiredFields
-}
-
 // Default ResourceRequirements
 var (
 	MlmdRestResourceRequirements = createResourceRequirement(resource.MustParse("100m"), resource.MustParse("256Mi"), resource.MustParse("100m"), resource.MustParse("256Mi"))

diff --git a/internal/controller/config/templates/deployment.yaml.tmpl b/internal/controller/config/templates/deployment.yaml.tmpl
@@ -6,21 +6,18 @@ metadata:
   labels:
     app: {{.Name}}
     component: model-registry
-    registry: {{.Name}}
 spec:
   replicas: 1
   revisionHistoryLimit: 0
   selector:
     matchLabels:
       app: {{.Name}}
       component: model-registry
-      registry: {{.Name}}
   template:
     metadata:
       labels:
         app: {{.Name}}
         component: model-registry
-        registry: {{.Name}}
     spec:
       containers:
         - args:

diff --git a/internal/controller/config/templates/service.yaml.tmpl b/internal/controller/config/templates/service.yaml.tmpl
@@ -6,7 +6,6 @@ metadata:
   labels:
     app: {{.Name}}
     component: model-registry
-    registry: {{.Name}}
 spec:
   ports:
     - name: grpc-api
@@ -18,5 +17,4 @@ spec:
   selector:
     app: {{.Name}}
     component: model-registry
-    registry: {{.Name}}
   type: ClusterIP
diff --git a/internal/controller/config/templates/serviceaccount.yaml.tmpl b/internal/controller/config/templates/serviceaccount.yaml.tmpl
@@ -6,4 +6,3 @@ metadata:
   labels:
     app: {{.Name}}
     component: model-registry
-    registry: {{.Name}}
diff --git a/internal/controller/modelregistry_controller.go b/internal/controller/modelregistry_controller.go
@@ -259,6 +259,70 @@ func (r *ModelRegistryReconciler) updateRegistryResources(ctx context.Context, p
 	return result, nil
 }
 
+func (r *ModelRegistryReconciler) setRegistryStatus(ctx context.Context, req ctrl.Request, operationResult OperationResult) error {
+	log := klog.FromContext(ctx)
+
+	modelRegistry := &modelregistryv1alpha1.ModelRegistry{}
+	if err := r.Get(ctx, req.NamespacedName, modelRegistry); err != nil {
+		log.Error(err, "Failed to re-fetch modelRegistry")
+		return err
+	}
+
+	status := metav1.ConditionTrue
+	reason := ReasonCreated
+	message := "Deployment for custom resource %s was successfully created"
+	switch operationResult {
+	case ResourceCreated:
+		status = metav1.ConditionFalse
+		reason = ReasonCreating
+		message = "Creating deployment for custom resource %s"
+	case ResourceUpdated:
+		status = metav1.ConditionFalse
+		reason = ReasonUpdating
+		message = "Updating deployment for custom resource %s"
+	}
+
+	meta.SetStatusCondition(&modelRegistry.Status.Conditions, metav1.Condition{Type: ConditionTypeProgressing,
+		Status: status, Reason: reason,
+		Message: fmt.Sprintf(message, modelRegistry.Name)})
+
+	// determine registry available condition
+	deployment := &appsv1.Deployment{}
+	if err := r.Get(ctx, req.NamespacedName, deployment); err != nil {
+		log.Error(err, "Failed to get modelRegistry deployment", "name", req.NamespacedName)
+		return err
+	}
+	log.V(10).Info("Found service deployment", "name", len(deployment.Name))
+
+	// check deployment availability
+	available := false
+	for _, c := range deployment.Status.Conditions {
+		if c.Type == appsv1.DeploymentAvailable {
+			available = c.Status == corev1.ConditionTrue
+			break
+		}
+	}
+
+	if available {
+		status = metav1.ConditionTrue
+		reason = ReasonAvailable
+		message = "Deployment for custom resource %s is available"
+	} else {
+		status = metav1.ConditionFalse
+		reason = ReasonUnavailable
+		message = "Deployment for custom resource %s is not available"
+	}
+	meta.SetStatusCondition(&modelRegistry.Status.Conditions, metav1.Condition{Type: ConditionTypeAvailable,
+		Status: status, Reason: reason,
+		Message: fmt.Sprintf(message, modelRegistry.Name)})
+
+	if err := r.Status().Update(ctx, modelRegistry); err != nil {
+		log.Error(err, "Failed to update modelRegistry status")
+		return err
+	}
+	return nil
+}
+
 func (r *ModelRegistryReconciler) createOrUpdateDeployment(ctx context.Context, params *ModelRegistryParams,
 	registry *modelregistryv1alpha1.ModelRegistry, templateName string) (result OperationResult, err error) {
 	result = ResourceUnchanged
@@ -311,7 +375,6 @@ func (r *ModelRegistryReconciler) createOrUpdateServiceAccount(ctx context.Conte
 }
 
 //go:generate go-enum -type=OperationResult
-
 type OperationResult int
 
 const (
@@ -346,8 +409,10 @@ func (r *ModelRegistryReconciler) createOrUpdate(ctx context.Context, currObj cl
 		return result, err
 	}
 
+	// hack: envtest is missing typemeta for some reason, hence the ignores for apiVersion and kind!!!
 	// create a patch by comparing objects
-	patchResult, err := patch.DefaultPatchMaker.Calculate(currObj, newObj, patch.IgnoreStatusFields())
+	patchResult, err := patch.DefaultPatchMaker.Calculate(currObj, newObj, patch.IgnoreStatusFields(),
+		patch.IgnoreField("apiVersion"), patch.IgnoreField("kind"))
 	if err != nil {
 		return result, err
 	}
@@ -420,68 +485,3 @@ func (r *ModelRegistryReconciler) logResultAsEvent(registry *modelregistryv1alph
 				registry.Namespace))
 	}
 }
-
-func (r *ModelRegistryReconciler) setRegistryStatus(ctx context.Context, req ctrl.Request, operationResult OperationResult) error {
-	log := klog.FromContext(ctx)
-
-	modelRegistry := &modelregistryv1alpha1.ModelRegistry{}
-	if err := r.Get(ctx, req.NamespacedName, modelRegistry); err != nil {
-		log.Error(err, "Failed to re-fetch modelRegistry")
-		return err
-	}
-
-	status := metav1.ConditionTrue
-	reason := ReasonCreated
-	message := "Deployment for custom resource %s was successfully created"
-	if operationResult != ResourceUnchanged {
-		status = metav1.ConditionFalse
-	}
-	if operationResult == ResourceCreated {
-		reason = ReasonCreating
-		message = "Creating deployment for custom resource %s"
-	}
-	if operationResult == ResourceUpdated {
-		reason = ReasonUpdating
-		message = "Updating deployment for custom resource %s"
-	}
-
-	meta.SetStatusCondition(&modelRegistry.Status.Conditions, metav1.Condition{Type: ConditionTypeProgressing,
-		Status: status, Reason: reason,
-		Message: fmt.Sprintf(message, modelRegistry.Name)})
-
-	// determine registry available condition
-	deployment := &appsv1.Deployment{}
-	if err := r.Get(ctx, req.NamespacedName, deployment); err != nil {
-		log.Error(err, "Failed to get modelRegistry deployment", "name", req.NamespacedName)
-		return err
-	}
-	log.V(10).Info("Found service deployment", "name", len(deployment.Name))
-
-	// check deployment availability
-	available := false
-	for _, c := range deployment.Status.Conditions {
-		if c.Type == appsv1.DeploymentAvailable {
-			available = c.Status == corev1.ConditionTrue
-			break
-		}
-	}
-
-	if available {
-		status = metav1.ConditionTrue
-		reason = ReasonAvailable
-		message = "Deployment for custom resource %s is available"
-	} else {
-		status = metav1.ConditionFalse
-		reason = ReasonUnavailable
-		message = "Deployment for custom resource %s is not available"
-	}
-	meta.SetStatusCondition(&modelRegistry.Status.Conditions, metav1.Condition{Type: ConditionTypeAvailable,
-		Status: status, Reason: reason,
-		Message: fmt.Sprintf(message, modelRegistry.Name)})
-
-	if err := r.Status().Update(ctx, modelRegistry); err != nil {
-		log.Error(err, "Failed to update modelRegistry status")
-		return err
-	}
-	return nil
-}