Improve readability:

Return in each switch case. Make function for patching status. Reorganize where some conditions were being set. Replace !errors.IsNotFound with ctrlclient.IgnoreNotFound in order to improve readability. Signed-off-by: Jacob Weinstock <[email protected]>
tinkerbell · Sep 29, 2024 · ff45778 · ff45778
1 parent 7084d48
commit ff45778
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 75 deletions.
diff --git a/docs/Workflow.md b/docs/Workflow.md
@@ -0,0 +1,28 @@
+# The Workflow custom resource
+
+This doc provides details for different parts of the Workflow custom resource.
+
+## Spec
+
+### BootOptions
+
+The `spec.bootOptions` object contains optional functionality that will run before a Workflow and triggers handling of different Hardware booting capabilities.
+
+## Status
+
+### State
+
+There are several states that a Workflow can be in:
+
+`STATE_WAITING` -
+`STATE_PENDING` -
+`STATE_RUNNING` -
+`STATE_SUCCESS` -
+`STATE_FAILED`  -
+`STATE_TIMEOUT` -
+
+### OneTimeNetboot
+
+### TemplateRendering
+
+### Conditions
diff --git a/internal/deprecated/workflow/bootops.go b/internal/deprecated/workflow/bootops.go
@@ -7,7 +7,6 @@ import (
 
 	rufio "github.com/tinkerbell/rufio/api/v1alpha1"
 	"github.com/tinkerbell/tink/api/v1alpha1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -25,13 +24,13 @@ func handleExistingJob(ctx context.Context, cc client.Client, wf *v1alpha1.Workf
 	}
 	name := fmt.Sprintf(bmcJobName, wf.Spec.HardwareRef)
 	namespace := wf.Namespace
-	if err := cc.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, &rufio.Job{}); (err != nil && !errors.IsNotFound(err)) || err == nil {
+	if err := cc.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, &rufio.Job{}); client.IgnoreNotFound(err) != nil || err == nil {
 		existingJob := &rufio.Job{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}}
 		opts := []client.DeleteOption{
 			client.GracePeriodSeconds(0),
 			client.PropagationPolicy(metav1.DeletePropagationForeground),
 		}
-		if err := cc.Delete(ctx, existingJob, opts...); err != nil {
+		if err := cc.Delete(ctx, existingJob, opts...); client.IgnoreNotFound(err) != nil {
 			return reconcile.Result{}, fmt.Errorf("error deleting job.bmc.tinkerbell.org object: %w", err)
 		}
 		return reconcile.Result{Requeue: true}, nil
@@ -49,8 +48,8 @@ func handleJobCreation(ctx context.Context, cc client.Client, wf *v1alpha1.Workf
 			return reconcile.Result{Requeue: true}, nil
 		}
 		hw := &v1alpha1.Hardware{ObjectMeta: metav1.ObjectMeta{Name: wf.Spec.HardwareRef, Namespace: wf.Namespace}}
-		if gerr := cc.Get(ctx, client.ObjectKey{Name: wf.Spec.HardwareRef, Namespace: wf.Namespace}, hw); gerr != nil {
-			return reconcile.Result{}, fmt.Errorf("error getting hardware %s: %w", wf.Spec.HardwareRef, gerr)
+		if err := cc.Get(ctx, client.ObjectKey{Name: wf.Spec.HardwareRef, Namespace: wf.Namespace}, hw); err != nil {
+			return reconcile.Result{}, fmt.Errorf("error getting hardware %s: %w", wf.Spec.HardwareRef, err)
 		}
 		if hw.Spec.BMCRef == nil {
 			return reconcile.Result{}, fmt.Errorf("hardware %s does not have a BMC, cannot perform one time netboot", hw.Name)
@@ -72,7 +71,6 @@ func handleJobCreation(ctx context.Context, cc client.Client, wf *v1alpha1.Workf
 			Message: "job created",
 			Time:    &metav1.Time{Time: metav1.Now().UTC()},
 		})
-
 		return reconcile.Result{Requeue: true}, nil
 	}
 

diff --git a/internal/deprecated/workflow/reconciler.go b/internal/deprecated/workflow/reconciler.go
@@ -2,6 +2,7 @@ package workflow
 
 import (
 	"context"
+	serrors "errors"
 	"fmt"
 	"time"
 
@@ -57,91 +58,83 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 	if !stored.DeletionTimestamp.IsZero() {
 		return reconcile.Result{}, nil
 	}
+
 	wflow := stored.DeepCopy()
 
-	var (
-		resp reconcile.Result
-		err  error
-	)
 	switch wflow.Status.State {
 	case "":
-		resp, err = r.processNewWorkflow(ctx, logger, wflow)
-	case v1alpha1.WorkflowStateRunning:
-		resp = r.processRunningWorkflow(ctx, wflow)
-		// set the current action in the status
-		ca := runningAction(wflow)
-		if ca != "" && wflow.Status.CurrentAction != ca {
-			wflow.Status.CurrentAction = ca
-		}
+		resp, err := r.processNewWorkflow(ctx, logger, wflow)
+
+		return resp, serrors.Join(err, mergePatchsStatus(ctx, r.client, stored, wflow))
 	case v1alpha1.WorkflowStateWaiting:
 		// make sure any existing job is deleted
 		if !wflow.Status.BootOptions.OneTimeNetboot.ExistingJobDeleted {
 			rc, err := handleExistingJob(ctx, r.client, wflow)
-			// Patch any changes, regardless of errors
-			if !equality.Semantic.DeepEqual(wflow, stored) {
-				if perr := r.client.Status().Patch(ctx, wflow, ctrlclient.MergeFrom(stored)); perr != nil {
-					err = fmt.Errorf("error patching workflow %s, %w", wflow.Name, perr)
-				}
-			}
-			return rc, err
+
+			return rc, serrors.Join(err, mergePatchsStatus(ctx, r.client, stored, wflow))
 		}
 
 		// create a new job
 		if wflow.Status.BootOptions.OneTimeNetboot.UID == "" && wflow.Status.BootOptions.OneTimeNetboot.ExistingJobDeleted {
 			rc, err := handleJobCreation(ctx, r.client, wflow)
-			// Patch any changes, regardless of errors
-			if !equality.Semantic.DeepEqual(wflow, stored) {
-				if perr := r.client.Status().Patch(ctx, wflow, ctrlclient.MergeFrom(stored)); perr != nil {
-					err = fmt.Errorf("error patching workflow %s, %w", wflow.Name, perr)
-				}
-			}
-			return rc, err
+
+			return rc, serrors.Join(err, mergePatchsStatus(ctx, r.client, stored, wflow))
 		}
 
 		// check if the job is complete
 		if !wflow.Status.BootOptions.OneTimeNetboot.Complete && wflow.Status.BootOptions.OneTimeNetboot.UID != "" && wflow.Status.BootOptions.OneTimeNetboot.ExistingJobDeleted {
 			rc, err := handleJobComplete(ctx, r.client, wflow)
-			// Patch any changes, regardless of errors
-			if !equality.Semantic.DeepEqual(wflow, stored) {
-				if perr := r.client.Status().Patch(ctx, wflow, ctrlclient.MergeFrom(stored)); perr != nil {
-					err = fmt.Errorf("error patching workflow %s, %w", wflow.Name, perr)
-				}
-			}
-			return rc, err
+
+			return rc, serrors.Join(err, mergePatchsStatus(ctx, r.client, stored, wflow))
+		}
+	case v1alpha1.WorkflowStateRunning:
+		r.processRunningWorkflow(wflow)
+		// set the current action in the status
+		ca := runningAction(wflow)
+		if ca != "" && wflow.Status.CurrentAction != ca {
+			wflow.Status.CurrentAction = ca
 		}
+
+		return reconcile.Result{}, mergePatchsStatus(ctx, r.client, stored, wflow)
+	case v1alpha1.WorkflowStatePending, v1alpha1.WorkflowStateTimeout, v1alpha1.WorkflowStateFailed:
+		return reconcile.Result{}, nil
 	case v1alpha1.WorkflowStateSuccess:
 		if wflow.Spec.BootOptions.ToggleAllowNetboot && !wflow.Status.HasCondition(v1alpha1.ToggleAllowNetbootFalse, metav1.ConditionTrue) {
 			// handle updating hardware allowPXE to false
-			wflow.Status.SetCondition(v1alpha1.WorkflowCondition{
-				Type:    v1alpha1.ToggleAllowNetbootFalse,
-				Status:  metav1.ConditionTrue,
-				Reason:  "Complete",
-				Message: "set allowPXE to false",
-				Time:    &metav1.Time{Time: metav1.Now().UTC()},
-			})
-			if gerr := handleHardwareAllowPXE(ctx, r.client, wflow, nil, false); gerr != nil {
+			if err := handleHardwareAllowPXE(ctx, r.client, wflow, nil, false); err != nil {
 				stored.Status.SetCondition(v1alpha1.WorkflowCondition{
 					Type:    v1alpha1.ToggleAllowNetbootFalse,
 					Status:  metav1.ConditionTrue,
 					Reason:  "Error",
-					Message: fmt.Sprintf("error setting Allow PXE: %v", gerr),
+					Message: fmt.Sprintf("error setting Allow PXE: %v", err),
 					Time:    &metav1.Time{Time: metav1.Now().UTC()},
 				})
-				err = gerr
+				return reconcile.Result{}, serrors.Join(err, mergePatchsStatus(ctx, r.client, stored, wflow))
 			}
+			wflow.Status.SetCondition(v1alpha1.WorkflowCondition{
+				Type:    v1alpha1.ToggleAllowNetbootFalse,
+				Status:  metav1.ConditionTrue,
+				Reason:  "Complete",
+				Message: "set allowPXE to false",
+				Time:    &metav1.Time{Time: metav1.Now().UTC()},
+			})
+
+			return reconcile.Result{}, mergePatchsStatus(ctx, r.client, stored, wflow)
 		}
-	default:
-		return resp, nil
 	}
 
+	return reconcile.Result{}, nil
+}
+
+// mergePatchsStatus merges an updated Workflow with an original Workflow and patches the Status object via the client (cc).
+func mergePatchsStatus(ctx context.Context, cc ctrlclient.Client, original, updated *v1alpha1.Workflow) error {
 	// Patch any changes, regardless of errors
-	if !equality.Semantic.DeepEqual(wflow, stored) {
-		if perr := r.client.Status().Patch(ctx, wflow, ctrlclient.MergeFrom(stored)); perr != nil {
-			err = fmt.Errorf("error patching workflow %s, %w", wflow.Name, perr)
+	if !equality.Semantic.DeepEqual(updated, original) {
+		if err := cc.Status().Patch(ctx, updated, ctrlclient.MergeFrom(original)); err != nil {
+			return fmt.Errorf("error patching status of workflow: %s, error: %w", updated.Name, err)
 		}
 	}
-
-	return resp, err
+	return nil
 }
 
 func runningAction(wf *v1alpha1.Workflow) string {
@@ -187,14 +180,9 @@ func (r *Reconciler) processNewWorkflow(ctx context.Context, logger logr.Logger,
 		return reconcile.Result{}, err
 	}
 
-	data := make(map[string]interface{})
-	for key, val := range stored.Spec.HardwareMap {
-		data[key] = val
-	}
-
 	var hardware v1alpha1.Hardware
 	err := r.client.Get(ctx, ctrlclient.ObjectKey{Name: stored.Spec.HardwareRef, Namespace: stored.Namespace}, &hardware)
-	if err != nil && !errors.IsNotFound(err) {
+	if ctrlclient.IgnoreNotFound(err) != nil {
 		logger.Error(err, "error getting Hardware object in processNewWorkflow function")
 		stored.Status.TemplateRendering = v1alpha1.TemplateRenderingFailed
 		stored.Status.SetCondition(v1alpha1.WorkflowCondition{
@@ -224,10 +212,12 @@ func (r *Reconciler) processNewWorkflow(ctx context.Context, logger logr.Logger,
 		)
 	}
 
-	if err == nil {
-		contract := toTemplateHardwareData(hardware)
-		data["Hardware"] = contract
+	data := make(map[string]interface{})
+	for key, val := range stored.Spec.HardwareMap {
+		data[key] = val
 	}
+	contract := toTemplateHardwareData(hardware)
+	data["Hardware"] = contract
 
 	tinkWf, err := renderTemplateHardware(stored.Name, ptr.StringValue(tpl.Spec.Data), data)
 	if err != nil {
@@ -255,13 +245,6 @@ func (r *Reconciler) processNewWorkflow(ctx context.Context, logger logr.Logger,
 
 	// set hardware allowPXE if requested.
 	if stored.Spec.BootOptions.ToggleAllowNetboot {
-		stored.Status.SetCondition(v1alpha1.WorkflowCondition{
-			Type:    v1alpha1.ToggleAllowNetbootTrue,
-			Status:  metav1.ConditionTrue,
-			Reason:  "Complete",
-			Message: "set allowPXE to true",
-			Time:    &metav1.Time{Time: metav1.Now().UTC()},
-		})
 		if err := handleHardwareAllowPXE(ctx, r.client, stored, &hardware, true); err != nil {
 			stored.Status.SetCondition(v1alpha1.WorkflowCondition{
 				Type:    v1alpha1.ToggleAllowNetbootTrue,
@@ -272,6 +255,13 @@ func (r *Reconciler) processNewWorkflow(ctx context.Context, logger logr.Logger,
 			})
 			return reconcile.Result{}, err
 		}
+		stored.Status.SetCondition(v1alpha1.WorkflowCondition{
+			Type:    v1alpha1.ToggleAllowNetbootTrue,
+			Status:  metav1.ConditionTrue,
+			Reason:  "Complete",
+			Message: "set allowPXE to true",
+			Time:    &metav1.Time{Time: metav1.Now().UTC()},
+		})
 	}
 
 	// netboot the hardware if requested
@@ -315,7 +305,7 @@ func toTemplateHardwareData(hardware v1alpha1.Hardware) templateHardwareData {
 	return contract
 }
 
-func (r *Reconciler) processRunningWorkflow(_ context.Context, stored *v1alpha1.Workflow) reconcile.Result { //nolint:unparam // This is the way controller runtime works.
+func (r *Reconciler) processRunningWorkflow(stored *v1alpha1.Workflow) {
 	// Check for global timeout expiration
 	if r.nowFunc().After(stored.GetStartTime().Add(time.Duration(stored.Status.GlobalTimeout) * time.Second)) {
 		stored.Status.State = v1alpha1.WorkflowStateTimeout
@@ -336,6 +326,4 @@ func (r *Reconciler) processRunningWorkflow(_ context.Context, stored *v1alpha1.
 			}
 		}
 	}
-
-	return reconcile.Result{}
 }