Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Refactor reconcilor so each component is returning status #765

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −58
Open

WIP: Refactor reconcilor so each component is returning status #765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions controllers/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,50 +39,50 @@ 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
}
}

for cmName, template := range samplePipelineTemplates {
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
}
10 changes: 5 additions & 5 deletions controllers/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
28 changes: 15 additions & 13 deletions controllers/dspipeline_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if overwriting the Message here is the right call. The reason I'm wondering that is that https://github.com/opendatahub-io/data-science-pipelines-operator/blob/main/controllers/dspipeline_controller.go#L406 has a number of instances in which it is setting the Message field itself. Thoughts @hbelmiro ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually proposed replacing

r.setStatus(ctx, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady, statusMessage, dspa,
				dspaStatus.SetMLMDProxyStatus, log)

with

			condition, err := r.evaluateCondition(ctx, dspa, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady)
			condition.Message = status
			if err != nil {
				r.setStatusAsNotReady(config.MLMDProxyReady, err, dspaStatus.SetMLMDProxyStatus)
				return ctrl.Result{}, err
			}
			dspaStatus.SetMLMDProxyStatus(condition)

here.

setStatus(condition)
if err != nil {
log.Error(err, fmt.Sprintf("Encountered error when creating the %s readiness condition", conditionType))
Expand Down
3 changes: 2 additions & 1 deletion controllers/dspipeline_controller_func_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
20 changes: 10 additions & 10 deletions controllers/mlmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package controllers

import (
"context"
"errors"

dspav1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1"
)

Expand All @@ -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
}
10 changes: 5 additions & 5 deletions controllers/mlpipeline_ui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
10 changes: 5 additions & 5 deletions controllers/persistence_agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
10 changes: 5 additions & 5 deletions controllers/scheduled_workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
10 changes: 5 additions & 5 deletions controllers/workflow_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Loading