From b55317e853bded62534372872f8b2a71f7ae7e77 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Fri, 20 Sep 2024 15:56:12 -0600 Subject: [PATCH] Simplify allowPXE handling: Push the boolean info to the caller so that handleHardwareAllowPXE can be simpler. Signed-off-by: Jacob Weinstock --- internal/deprecated/workflow/reconciler.go | 65 ++++++++----------- .../deprecated/workflow/reconciler_test.go | 6 +- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/internal/deprecated/workflow/reconciler.go b/internal/deprecated/workflow/reconciler.go index ce00b45b7..62137a40e 100644 --- a/internal/deprecated/workflow/reconciler.go +++ b/internal/deprecated/workflow/reconciler.go @@ -94,15 +94,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{Requeue: true}, nil } case v1alpha1.WorkflowStateSuccess: - // handle updating hardware allowPXE to false - var hw v1alpha1.Hardware - err := r.client.Get(ctx, ctrlclient.ObjectKey{Name: wflow.Spec.HardwareRef, Namespace: wflow.Namespace}, &hw) - if err != nil && !errors.IsNotFound(err) { - logger.Error(err, "error getting Hardware object for WorkflowStateSuccess processing") - return reconcile.Result{}, err - } - if err := handleHardwareAllowPXE(ctx, r.client, wflow, &hw); err != nil { - return resp, err + if wflow.Spec.BootOpts.ToggleAllowNetboot { + // handle updating hardware allowPXE to false + if err := handleHardwareAllowPXE(ctx, r.client, wflow, nil, false); err != nil { + return reconcile.Result{}, err + } } default: return resp, nil @@ -117,40 +113,32 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return resp, err } -func handleHardwareAllowPXE(ctx context.Context, client ctrlclient.Client, stored *v1alpha1.Workflow, hardware *v1alpha1.Hardware) error { - // We need to set allowPXE to true before a workflow runs. - // We need to set allowPXE to false after a workflow completes successfully. +// handleHardwareAllowPXE sets the allowPXE field on the hardware interfaces to true before a workflow runs and false after a workflow completes successfully. +// If hardware is nil then it will be retrieved using the client. +func handleHardwareAllowPXE(ctx context.Context, client ctrlclient.Client, stored *v1alpha1.Workflow, hardware *v1alpha1.Hardware, allowPXE bool) error { + if stored == nil { + return fmt.Errorf("cannot handle hardware allowPXE without a workflow") + } - // before workflow case - if stored.Status.ToggleAllowNetboot == nil || (stored.Status.ToggleAllowNetboot != nil && stored.Status.ToggleAllowNetboot.Status == "" && stored.Status.State == "" || stored.Status.State == v1alpha1.WorkflowStatePending) { - status := &v1alpha1.Status{Status: v1alpha1.StatusSuccess, Message: "allowPXE set to true"} - for _, iface := range hardware.Spec.Interfaces { - iface.Netboot.AllowPXE = ptr.Bool(true) - } - if err := client.Update(ctx, hardware); err != nil { - status.Status = v1alpha1.StatusFailure - status.Message = fmt.Sprintf("error setting allowPXE: %v", err) - stored.Status.ToggleAllowNetboot = status - return err + if hardware == nil { + hardware = &v1alpha1.Hardware{} + if err := client.Get(ctx, ctrlclient.ObjectKey{Name: stored.Spec.HardwareRef, Namespace: stored.Namespace}, hardware); err != nil { + stored.Status.ToggleAllowNetboot = &v1alpha1.Status{Status: v1alpha1.StatusFailure, Message: fmt.Sprintf("error getting hardware: %v", err)} + return fmt.Errorf("hardware not found: name=%v; namespace=%v, error: %w", stored.Spec.HardwareRef, stored.Namespace, err) } - stored.Status.ToggleAllowNetboot = status } - // after workflow case - if stored.Status.State == v1alpha1.WorkflowStateSuccess { - status := &v1alpha1.Status{Status: v1alpha1.StatusSuccess, Message: "allowPXE set to false"} - for _, iface := range hardware.Spec.Interfaces { - iface.Netboot.AllowPXE = ptr.Bool(false) - } - if err := client.Update(ctx, hardware); err != nil { - status.Status = v1alpha1.StatusFailure - status.Message = fmt.Sprintf("error setting allowPXE: %v", err) - stored.Status.ToggleAllowNetboot = status - return err - } - stored.Status.ToggleAllowNetboot = status + for _, iface := range hardware.Spec.Interfaces { + iface.Netboot.AllowPXE = ptr.Bool(allowPXE) } + if err := client.Update(ctx, hardware); err != nil { + stored.Status.ToggleAllowNetboot = &v1alpha1.Status{Status: v1alpha1.StatusFailure, Message: fmt.Sprintf("error setting allowPXE: %v", err)} + return err + } + + stored.Status.ToggleAllowNetboot = &v1alpha1.Status{Status: v1alpha1.StatusSuccess, Message: fmt.Sprintf("allowPXE set to %v", allowPXE)} + return nil } @@ -288,8 +276,7 @@ func (r *Reconciler) processNewWorkflow(ctx context.Context, logger logr.Logger, // set hardware allowPXE if requested. if stored.Spec.BootOpts.ToggleAllowNetboot { - if err := handleHardwareAllowPXE(ctx, r.client, stored, &hardware); err != nil { - stored.Status.ToggleAllowNetboot = &v1alpha1.Status{Status: v1alpha1.StatusFailure, Message: fmt.Sprintf("error setting allowPXE: %v", err)} + if err := handleHardwareAllowPXE(ctx, r.client, stored, &hardware, true); err != nil { return reconcile.Result{}, err } } diff --git a/internal/deprecated/workflow/reconciler_test.go b/internal/deprecated/workflow/reconciler_test.go index dbe57f057..573662171 100644 --- a/internal/deprecated/workflow/reconciler_test.go +++ b/internal/deprecated/workflow/reconciler_test.go @@ -94,6 +94,7 @@ func TestHandleHardwareAllowPXE(t *testing.T) { OriginalWorkflow *v1alpha1.Workflow WantWorkflow *v1alpha1.Workflow WantError error + AllowPXE bool }{ "before workflow": { OriginalHardware: &v1alpha1.Hardware{ @@ -141,6 +142,7 @@ func TestHandleHardwareAllowPXE(t *testing.T) { Message: "allowPXE set to true", }, }}, + AllowPXE: true, }, "after workflow": { OriginalHardware: &v1alpha1.Hardware{ @@ -166,7 +168,7 @@ func TestHandleHardwareAllowPXE(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "machine1", Namespace: "default", - ResourceVersion: "1002", + ResourceVersion: "1001", }, Spec: v1alpha1.HardwareSpec{ Interfaces: []v1alpha1.Interface{ @@ -199,7 +201,7 @@ func TestHandleHardwareAllowPXE(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { fakeClient := GetFakeClientBuilder().WithRuntimeObjects(tt.OriginalHardware).Build() - err := handleHardwareAllowPXE(context.Background(), fakeClient, tt.OriginalWorkflow, tt.OriginalHardware) + err := handleHardwareAllowPXE(context.Background(), fakeClient, tt.OriginalWorkflow, tt.OriginalHardware, tt.AllowPXE) if diff := cmp.Diff(tt.WantError, err, cmp.Comparer(func(a, b error) bool { return a.Error() == b.Error()