From aa6476ce31b67baf1298fd957eaf94bc8058da55 Mon Sep 17 00:00:00 2001 From: Georgi Chulkov Date: Mon, 29 Apr 2024 16:52:33 +0200 Subject: [PATCH] Remove Unknown state and stop creating unknown OOBs --- api/v1alpha1/oob_types.go | 13 +++-- config/crd/bases/metal.ironcore.dev_oobs.yaml | 4 +- internal/controller/oob_controller.go | 54 +++++++++---------- internal/controller/oob_controller_test.go | 15 ++---- 4 files changed, 38 insertions(+), 48 deletions(-) diff --git a/api/v1alpha1/oob_types.go b/api/v1alpha1/oob_types.go index 8b77938e..139d9982 100644 --- a/api/v1alpha1/oob_types.go +++ b/api/v1alpha1/oob_types.go @@ -78,7 +78,7 @@ type OOBStatus struct { // +optional FirmwareVersion string `json:"firmwareVersion,omitempty"` - // +kubebuilder:validation:Enum=Ready;Unready;Ignored;Unknown;Error + // +kubebuilder:validation:Enum=Ready;InProgress;NoEndpoint;Ignored;Error // +optional State OOBState `json:"state,omitempty"` @@ -99,11 +99,11 @@ const ( type OOBState string const ( - OOBStateReady OOBState = "Ready" - OOBStateUnready OOBState = "Unready" - OOBStateIgnored OOBState = "Ignored" - OOBStateUnknown OOBState = "Unknown" - OOBStateError OOBState = "Error" + OOBStateReady OOBState = "Ready" + OOBStateInProgress OOBState = "InProgress" + OOBStateNoEndpoint OOBState = "NoEndpoint" + OOBStateIgnored OOBState = "Ignored" + OOBStateError OOBState = "Error" ) const ( @@ -112,7 +112,6 @@ const ( OOBConditionReasonInProgress = "InProgress" OOBConditionReasonNoEndpoint = "NoEndpoint" OOBConditionReasonIgnored = "Ignored" - OOBConditionReasonUnknown = "Unknown" OOBConditionReasonError = "Error" ) diff --git a/config/crd/bases/metal.ironcore.dev_oobs.yaml b/config/crd/bases/metal.ironcore.dev_oobs.yaml index ff4e3b37..ef4e30e6 100644 --- a/config/crd/bases/metal.ironcore.dev_oobs.yaml +++ b/config/crd/bases/metal.ironcore.dev_oobs.yaml @@ -208,9 +208,9 @@ spec: state: enum: - Ready - - Unready + - InProgress + - NoEndpoint - Ignored - - Unknown - Error type: string type: diff --git a/internal/controller/oob_controller.go b/internal/controller/oob_controller.go index 70a58b86..c02d1e64 100644 --- a/internal/controller/oob_controller.go +++ b/internal/controller/oob_controller.go @@ -282,10 +282,9 @@ func (r *OOBReconciler) reconcile(ctx context.Context, oob *metalv1alpha1.OOB) ( } ctx, advance, err = r.runPhase(ctx, oob, oobRecPhase{ - name: "Credentials", - run: r.processCredentials, - errType: OOBErrorBadCredentials, - readyReasons: []string{metalv1alpha1.OOBConditionReasonUnknown}, + name: "Credentials", + run: r.processCredentials, + errType: OOBErrorBadCredentials, }) if !advance { return ctrl.Result{}, err @@ -400,7 +399,7 @@ func (r *OOBReconciler) processIgnoreAnnotation(ctx context.Context, oob *metalv Reason: metalv1alpha1.OOBConditionReasonIgnored, }) } else if oob.Status.State == metalv1alpha1.OOBStateIgnored { - return r.setCondition(ctx, oob, nil, nil, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, nil, nil, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -426,7 +425,7 @@ func (r *OOBReconciler) processInitial(ctx context.Context, oob *metalv1alpha1.O _, ok := ssa.GetCondition(oob.Status.Conditions, metalv1alpha1.OOBConditionTypeReady) if oob.Status.State == "" || !ok { - return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -511,7 +510,7 @@ func (r *OOBReconciler) processEndpoint(ctx context.Context, oob *metalv1alpha1. apply = apply.WithSpec(util.Ensure(apply.Spec). WithEndpointRef(*oob.Spec.EndpointRef)) - ctx, apply, status, err = r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateUnready, metav1.Condition{ + ctx, apply, status, err = r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -523,7 +522,7 @@ func (r *OOBReconciler) processEndpoint(ctx context.Context, oob *metalv1alpha1. break } if !found { - return r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateNoEndpoint, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonNoEndpoint, @@ -555,7 +554,7 @@ func (r *OOBReconciler) processEndpoint(ctx context.Context, oob *metalv1alpha1. if oob.Status.State == metalv1alpha1.OOBStateError { cond, _ := ssa.GetCondition(oob.Status.Conditions, metalv1alpha1.OOBConditionTypeReady) if strings.HasPrefix(cond.Message, OOBErrorBadEndpoint+": ") { - return r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, apply, status, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -640,7 +639,7 @@ func (r *OOBReconciler) processCredentials(ctx context.Context, oob *metalv1alph apply = apply.WithSpec(util.Ensure(apply.Spec). WithSecretRef(*oob.Spec.SecretRef)) - return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -651,11 +650,7 @@ func (r *OOBReconciler) processCredentials(ctx context.Context, oob *metalv1alph if oob.Spec.Protocol == nil || (creds.Username == "" && creds.Password == "") { a, ok := r.macDB.Get(oob.Spec.MACAddress) if !ok { - return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateUnknown, metav1.Condition{ - Type: metalv1alpha1.OOBConditionTypeReady, - Status: metav1.ConditionFalse, - Reason: metalv1alpha1.OOBConditionReasonUnknown, - }) + return r.setError(ctx, oob, apply, nil, OOBErrorBadCredentials, fmt.Errorf("cannot find MAC address in MAC DB: %s", oob.Spec.MACAddress)) } if a.Ignore && !metav1.HasAnnotation(oob.ObjectMeta, OOBIgnoreAnnotation) { @@ -804,7 +799,7 @@ func (r *OOBReconciler) processCredentials(ctx context.Context, oob *metalv1alph if oob.Status.State == metalv1alpha1.OOBStateError { cond, _ := ssa.GetCondition(oob.Status.Conditions, metalv1alpha1.OOBConditionTypeReady) if strings.HasPrefix(cond.Message, OOBErrorBadCredentials+": ") { - return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateUnready, metav1.Condition{ + return r.setCondition(ctx, oob, apply, nil, metalv1alpha1.OOBStateInProgress, metav1.Condition{ Type: metalv1alpha1.OOBConditionTypeReady, Status: metav1.ConditionFalse, Reason: metalv1alpha1.OOBConditionReasonInProgress, @@ -880,18 +875,21 @@ func (r *OOBReconciler) enqueueOOBFromIP(ctx context.Context, obj client.Object) } if len(oobList.Items) == 0 && ip.Status.State == ipamv1alpha1.CFinishedIPState && ip.Status.Reserved != nil { - oob := metalv1alpha1.OOB{ - ObjectMeta: metav1.ObjectMeta{ - Name: mac, - }, - } - apply := metalv1alpha1apply.OOB(oob.Name, oob.Namespace). - WithFinalizers(OOBFinalizer). - WithSpec(metalv1alpha1apply.OOBSpec(). - WithMACAddress(mac)) - err = r.Patch(ctx, &oob, ssa.Apply(apply), client.FieldOwner(OOBFieldManager), client.ForceOwnership) - if err != nil { - log.Error(ctx, fmt.Errorf("cannot apply OOB: %w", err)) + _, ok = r.macDB.Get(mac) + if ok { + oob := metalv1alpha1.OOB{ + ObjectMeta: metav1.ObjectMeta{ + Name: mac, + }, + } + apply := metalv1alpha1apply.OOB(oob.Name, oob.Namespace). + WithFinalizers(OOBFinalizer). + WithSpec(metalv1alpha1apply.OOBSpec(). + WithMACAddress(mac)) + err = r.Patch(ctx, &oob, ssa.Apply(apply), client.FieldOwner(OOBFieldManager), client.ForceOwnership) + if err != nil { + log.Error(ctx, fmt.Errorf("cannot apply OOB: %w", err)) + } } } diff --git a/internal/controller/oob_controller_test.go b/internal/controller/oob_controller_test.go index 2e4354dd..72b60ce6 100644 --- a/internal/controller/oob_controller_test.go +++ b/internal/controller/oob_controller_test.go @@ -186,7 +186,7 @@ var _ = Describe("OOB Controller", func() { By("Expecting the OOB to have no endpoint") Eventually(Object(oob)).Should(SatisfyAll( HaveField("Spec.EndpointRef", BeNil()), - HaveField("Status.State", metalv1alpha1.OOBStateUnready), + HaveField("Status.State", metalv1alpha1.OOBStateNoEndpoint), WithTransform(readyReason, Equal(metalv1alpha1.OOBConditionReasonNoEndpoint)), )) @@ -368,7 +368,7 @@ var _ = Describe("OOB Controller", func() { )) }) - It("should set the OOB to unknown if the MAC is unknown", func(ctx SpecContext) { + It("should not create an oob if the MAC is unknown", func(ctx SpecContext) { By("Creating an IP") ip := &ipamv1alpha1.IP{ ObjectMeta: metav1.ObjectMeta{ @@ -399,16 +399,9 @@ var _ = Describe("OOB Controller", func() { Name: ip.Labels[OOBIPMacLabel], }, } - DeferCleanup(func(ctx SpecContext) { - Expect(k8sClient.Delete(ctx, oob)).To(Succeed()) - Eventually(Get(oob)).Should(Satisfy(errors.IsNotFound)) - }) - By("Expecting OOB to be unknown") - Eventually(Object(oob)).Should(SatisfyAll( - HaveField("Status.State", metalv1alpha1.OOBStateUnknown), - WithTransform(readyReason, Equal(metalv1alpha1.OOBConditionReasonUnknown)), - )) + By("Expecting OOB not to exist") + Eventually(Get(oob)).Should(Satisfy(errors.IsNotFound)) }) It("should handle a bad credentials secret", func(ctx SpecContext) {