From 54ea85ecd6100bd556e309ef1cfe1ad136960f4d Mon Sep 17 00:00:00 2001 From: Anish Asthana Date: Thu, 2 Jan 2025 15:38:20 +0530 Subject: [PATCH] Refactor reconcilor so each component returns status Signed-off-by: Anish Asthana --- controllers/apiserver.go | 18 ++++++------ controllers/common.go | 10 +++---- controllers/dspipeline_controller.go | 28 ++++++++++--------- .../dspipeline_controller_func_test.go | 3 +- controllers/mlmd.go | 20 ++++++------- controllers/mlpipeline_ui.go | 10 +++---- controllers/persistence_agent.go | 10 +++---- controllers/scheduled_workflow.go | 10 +++---- controllers/workflow_controller.go | 10 +++---- 9 files changed, 61 insertions(+), 58 deletions(-) diff --git a/controllers/apiserver.go b/controllers/apiserver.go index 929ede232..5a88760b0 100644 --- a/controllers/apiserver.go +++ b/controllers/apiserver.go @@ -39,31 +39,31 @@ var samplePipelineTemplates = map[string]string{ "sample-config": "apiserver/sample-pipeline/sample-config.yaml.tmpl", } -func (r *DSPAReconciler) ReconcileAPIServer(ctx context.Context, dsp *dspav1.DataSciencePipelinesApplication, params *DSPAParams) error { +func (r *DSPAReconciler) ReconcileAPIServer(ctx context.Context, dsp *dspav1.DataSciencePipelinesApplication, params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) if !dsp.Spec.APIServer.Deploy { r.Log.Info("Skipping Application of APIServer Resources") - return nil + return "Skipped Application of APIServer Resources", nil } log.Info("Applying APIServer Resources") - err := r.ApplyDir(dsp, params, apiServerTemplatesDir) + err = r.ApplyDir(dsp, params, apiServerTemplatesDir) if err != nil { - return err + return "Failed to apply APIServer Resources", err } if dsp.Spec.APIServer.EnableRoute { err := r.Apply(dsp, params, serverRoute) if err != nil { - return err + return "Failed to apply APIServer route", err } } else { route := &v1.Route{} namespacedNamed := types.NamespacedName{Name: "ds-pipeline-" + dsp.Name, Namespace: dsp.Namespace} err := r.DeleteResourceIfItExists(ctx, route, namespacedNamed) if err != nil { - return err + return "Failed to delete APIServer route", err } } @@ -71,18 +71,18 @@ func (r *DSPAReconciler) ReconcileAPIServer(ctx context.Context, dsp *dspav1.Dat if dsp.Spec.APIServer.EnableSamplePipeline { err := r.Apply(dsp, params, template) if err != nil { - return err + return "Failed to apply sample pipeline", err } } else { cm := &corev1.ConfigMap{} namespacedNamed := types.NamespacedName{Name: cmName + "-" + dsp.Name, Namespace: dsp.Namespace} err := r.DeleteResourceIfItExists(ctx, cm, namespacedNamed) if err != nil { - return err + return "Failed to delete sample pipeline", err } } } log.Info("Finished applying APIServer Resources") - return nil + return "APIServer Resources Applied", nil } diff --git a/controllers/common.go b/controllers/common.go index 86a07db2b..87729b678 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -23,21 +23,21 @@ var commonTemplatesDir = "common/default" const commonCusterRolebindingTemplate = "common/no-owner/clusterrolebinding.yaml.tmpl" -func (r *DSPAReconciler) ReconcileCommon(dsp *dspav1.DataSciencePipelinesApplication, params *DSPAParams) error { +func (r *DSPAReconciler) ReconcileCommon(dsp *dspav1.DataSciencePipelinesApplication, params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) log.Info("Applying Common Resources") - err := r.ApplyDir(dsp, params, commonTemplatesDir) + err = r.ApplyDir(dsp, params, commonTemplatesDir) if err != nil { - return err + return "Error Applying Common Resources", err } err = r.ApplyWithoutOwner(params, commonCusterRolebindingTemplate) if err != nil { - return err + return "Error Applying clusterrolebinding", err } log.Info("Finished applying Common Resources") - return nil + return "Common Resources Applied", nil } func (r *DSPAReconciler) CleanUpCommon(params *DSPAParams) error { diff --git a/controllers/dspipeline_controller.go b/controllers/dspipeline_controller.go index 8f29d8da9..c80c5d850 100644 --- a/controllers/dspipeline_controller.go +++ b/controllers/dspipeline_controller.go @@ -19,6 +19,7 @@ package controllers import ( "context" "fmt" + "github.com/opendatahub-io/data-science-pipelines-operator/controllers/dspastatus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -286,55 +287,55 @@ func (r *DSPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. if dspaPrereqsReady { // Manage Common Manifests - err = r.ReconcileCommon(dspa, params) + var statusMessage string + _, err = r.ReconcileCommon(dspa, params) if err != nil { return ctrl.Result{}, err } - err = r.ReconcileAPIServer(ctx, dspa, params) + statusMessage, err = r.ReconcileAPIServer(ctx, dspa, params) if err != nil { r.setStatusAsNotReady(config.APIServerReady, err, dspaStatus.SetApiServerStatus) return ctrl.Result{}, err } else { - r.setStatus(ctx, params.APIServerDefaultResourceName, config.APIServerReady, dspa, + r.setStatus(ctx, params.APIServerDefaultResourceName, config.APIServerReady, statusMessage, dspa, dspaStatus.SetApiServerStatus, log) } - err = r.ReconcilePersistenceAgent(dspa, params) + statusMessage, err = r.ReconcilePersistenceAgent(dspa, params) if err != nil { r.setStatusAsNotReady(config.PersistenceAgentReady, err, dspaStatus.SetPersistenceAgentStatus) return ctrl.Result{}, err } else { - r.setStatus(ctx, params.PersistentAgentDefaultResourceName, config.PersistenceAgentReady, dspa, + r.setStatus(ctx, params.PersistentAgentDefaultResourceName, config.PersistenceAgentReady, statusMessage, dspa, dspaStatus.SetPersistenceAgentStatus, log) } - err = r.ReconcileScheduledWorkflow(dspa, params) + statusMessage, err = r.ReconcileScheduledWorkflow(dspa, params) if err != nil { r.setStatusAsNotReady(config.ScheduledWorkflowReady, err, dspaStatus.SetScheduledWorkflowStatus) return ctrl.Result{}, err } else { - r.setStatus(ctx, params.ScheduledWorkflowDefaultResourceName, config.ScheduledWorkflowReady, dspa, + r.setStatus(ctx, params.ScheduledWorkflowDefaultResourceName, config.ScheduledWorkflowReady, statusMessage, dspa, dspaStatus.SetScheduledWorkflowStatus, log) } - err = r.ReconcileUI(dspa, params) + _, err = r.ReconcileUI(dspa, params) if err != nil { return ctrl.Result{}, err } - err = r.ReconcileWorkflowController(dspa, params) + _, err = r.ReconcileWorkflowController(dspa, params) if err != nil { return ctrl.Result{}, err } - // MLMD should be the last to reconcile because it can cause an early exit due to the lack of the TLS secret, which may not have been created yet. - err = r.ReconcileMLMD(ctx, dspa, params) + statusMessage, err = r.ReconcileMLMD(ctx, dspa, params) if err != nil { r.setStatusAsNotReady(config.MLMDProxyReady, err, dspaStatus.SetMLMDProxyStatus) return ctrl.Result{}, err } else { - r.setStatus(ctx, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady, dspa, + r.setStatus(ctx, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady, statusMessage, dspa, dspaStatus.SetMLMDProxyStatus, log) } } @@ -374,10 +375,11 @@ func (r *DSPAReconciler) setStatusAsUnsupported(conditionType string, err error, setStatus(condition) } -func (r *DSPAReconciler) setStatus(ctx context.Context, resourceName string, conditionType string, +func (r *DSPAReconciler) setStatus(ctx context.Context, resourceName string, conditionType string, statusMessage string, dspa *dspav1.DataSciencePipelinesApplication, setStatus func(metav1.Condition), log logr.Logger) { condition, err := r.evaluateCondition(ctx, dspa, resourceName, conditionType) + condition.Message = statusMessage setStatus(condition) if err != nil { log.Error(err, fmt.Sprintf("Encountered error when creating the %s readiness condition", conditionType)) diff --git a/controllers/dspipeline_controller_func_test.go b/controllers/dspipeline_controller_func_test.go index 68659e240..26cd5a170 100644 --- a/controllers/dspipeline_controller_func_test.go +++ b/controllers/dspipeline_controller_func_test.go @@ -20,12 +20,13 @@ package controllers import ( "fmt" + "testing" + mfc "github.com/manifestival/controller-runtime-client" mf "github.com/manifestival/manifestival" util "github.com/opendatahub-io/data-science-pipelines-operator/controllers/testutil" "github.com/spf13/viper" "github.com/stretchr/testify/assert" - "testing" ) var uc = util.UtilContext{} diff --git a/controllers/mlmd.go b/controllers/mlmd.go index a70d272a7..72de1062c 100644 --- a/controllers/mlmd.go +++ b/controllers/mlmd.go @@ -17,7 +17,7 @@ package controllers import ( "context" - "errors" + dspav1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1" ) @@ -29,47 +29,47 @@ const ( ) func (r *DSPAReconciler) ReconcileMLMD(ctx context.Context, dsp *dspav1.DataSciencePipelinesApplication, - params *DSPAParams) error { + params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) 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 + return "MLMD Resource Application Skipped", nil } log.Info("Applying ML-Metadata (MLMD) Resources") // We need to create the service first so OpenShift creates the certificate that we'll use later. - err := r.ApplyDir(dsp, params, mlmdTemplatesDir+"/"+mlmdGrpcService) + err = r.ApplyDir(dsp, params, mlmdTemplatesDir+"/"+mlmdGrpcService) if err != nil { - return err + return "MLMD Service Failed to create", err } if params.PodToPodTLS { var certificatesExist bool certificatesExist, err = params.LoadMlmdCertificates(ctx, r.Client) if err != nil { - return err + return "Failed to load MLMD Certificate", err } if !certificatesExist { - return errors.New("secret containing the certificate for MLMD gRPC Server was not created yet") + return "Secret containing the certificate for MLMD gRPC Server was not created yet", nil } } err = r.ApplyDir(dsp, params, mlmdTemplatesDir) if err != nil { - return err + return "Failed to apply MLMD Resources", err } if dsp.Spec.MLMD == nil || dsp.Spec.MLMD.Envoy == nil || dsp.Spec.MLMD.Envoy.DeployRoute { err = r.Apply(dsp, params, mlmdEnvoyRoute) if err != nil { - return err + return "Failed to apply MLMD Envoy Route", err } } log.Info("Finished applying MLMD Resources") - return nil + return "MLMD Resources Applied", nil } diff --git a/controllers/mlpipeline_ui.go b/controllers/mlpipeline_ui.go index 4cec9dcbe..2421066eb 100644 --- a/controllers/mlpipeline_ui.go +++ b/controllers/mlpipeline_ui.go @@ -23,21 +23,21 @@ import ( var mlPipelineUITemplatesDir = "mlpipelines-ui" func (r *DSPAReconciler) ReconcileUI(dsp *dspav1.DataSciencePipelinesApplication, - params *DSPAParams) error { + params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) if dsp.Spec.MlPipelineUI == nil || !dsp.Spec.MlPipelineUI.Deploy { log.Info("Skipping Application of MlPipelineUI Resources") - return nil + return "Skipped application of MlPipelineUI Resources", nil } log.Info("Applying MlPipelineUI Resources") - err := r.ApplyDir(dsp, params, mlPipelineUITemplatesDir) + err = r.ApplyDir(dsp, params, mlPipelineUITemplatesDir) if err != nil { - return err + return "Failed to apply MlPipelineUI Resources", err } log.Info("Finished applying MlPipelineUI Resources") - return nil + return "MlPipelineUI Resources Applied", nil } diff --git a/controllers/persistence_agent.go b/controllers/persistence_agent.go index 5d5d7de79..33e89aaaa 100644 --- a/controllers/persistence_agent.go +++ b/controllers/persistence_agent.go @@ -25,22 +25,22 @@ var persistenceAgentTemplatesDir = "persistence-agent" const persistenceAgentDefaultResourceNamePrefix = "ds-pipeline-persistenceagent-" func (r *DSPAReconciler) ReconcilePersistenceAgent(dsp *dspav1.DataSciencePipelinesApplication, - params *DSPAParams) error { + params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) if !dsp.Spec.PersistenceAgent.Deploy { log.Info("Skipping Application of PersistenceAgent Resources") - return nil + return "Skipped Application PersistenceAgent Resources", nil } log.Info("Applying PersistenceAgent Resources") - err := r.ApplyDir(dsp, params, persistenceAgentTemplatesDir) + err = r.ApplyDir(dsp, params, persistenceAgentTemplatesDir) if err != nil { - return err + return "Failed to apply PersistenceAgent Resources", err } log.Info("Finished applying PersistenceAgent Resources") - return nil + return "PersistenceAgent Resources Applied", nil } diff --git a/controllers/scheduled_workflow.go b/controllers/scheduled_workflow.go index ce0a06d19..7f480906f 100644 --- a/controllers/scheduled_workflow.go +++ b/controllers/scheduled_workflow.go @@ -25,22 +25,22 @@ var scheduledWorkflowTemplatesDir = "scheduled-workflow" const scheduledWorkflowDefaultResourceNamePrefix = "ds-pipeline-scheduledworkflow-" func (r *DSPAReconciler) ReconcileScheduledWorkflow(dsp *dspav1.DataSciencePipelinesApplication, - params *DSPAParams) error { + params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) if !dsp.Spec.ScheduledWorkflow.Deploy { log.Info("Skipping Application of ScheduledWorkflow Resources") - return nil + return "Skipped Application of ScheduledWorkflow Resources", nil } log.Info("Applying ScheduledWorkflow Resources") - err := r.ApplyDir(dsp, params, scheduledWorkflowTemplatesDir) + err = r.ApplyDir(dsp, params, scheduledWorkflowTemplatesDir) if err != nil { - return err + return "Failed to apply ScheduledWorkflow Resources", err } log.Info("Finished applying ScheduledWorkflow Resources") - return nil + return "ScheduledWorkflow Resources Applied", nil } diff --git a/controllers/workflow_controller.go b/controllers/workflow_controller.go index 2d9a2c4b2..12b86b5be 100644 --- a/controllers/workflow_controller.go +++ b/controllers/workflow_controller.go @@ -23,22 +23,22 @@ import ( var workflowControllerTemplatesDir = "workflow-controller" func (r *DSPAReconciler) ReconcileWorkflowController(dsp *dspav1.DataSciencePipelinesApplication, - params *DSPAParams) error { + params *DSPAParams) (status string, err error) { log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) if dsp.Spec.WorkflowController == nil || !dsp.Spec.WorkflowController.Deploy { log.Info("Skipping Application of WorkflowController Resources") - return nil + return "Skipped application of WorkflowController Resources", nil } log.Info("Applying WorkflowController Resources") - err := r.ApplyDir(dsp, params, workflowControllerTemplatesDir) + err = r.ApplyDir(dsp, params, workflowControllerTemplatesDir) if err != nil { - return err + return "Failed to apply WorkflowController Resources", err } log.Info("Finished applying WorkflowController Resources") - return nil + return "WorkflowController Resources Applied", nil }