diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go index db492bebd2..dc279ead63 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_types.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go @@ -608,48 +608,6 @@ func (host *BareMetalHost) BootMode() BootMode { return mode } -// Available returns true if the host is available to be provisioned. -func (host *BareMetalHost) Available() bool { - if host.Spec.ConsumerRef != nil { - return false - } - if host.GetDeletionTimestamp() != nil { - return false - } - if host.HasError() { - return false - } - return true -} - -// SetErrorMessage updates the ErrorMessage in the host Status struct -// and increases the ErrorCount -func (host *BareMetalHost) SetErrorMessage(errType ErrorType, message string) { - host.Status.OperationalStatus = OperationalStatusError - host.Status.ErrorType = errType - host.Status.ErrorMessage = message - host.Status.ErrorCount++ -} - -// ClearError removes any existing error message. -func (host *BareMetalHost) ClearError() (dirty bool) { - dirty = host.SetOperationalStatus(OperationalStatusOK) - var emptyErrType ErrorType = "" - if host.Status.ErrorType != emptyErrType { - host.Status.ErrorType = emptyErrType - dirty = true - } - if host.Status.ErrorMessage != "" { - host.Status.ErrorMessage = "" - dirty = true - } - if host.Status.ErrorCount != 0 { - host.Status.ErrorCount = 0 - dirty = true - } - return dirty -} - // setLabel updates the given label when necessary and returns true // when a change is made or false when no change is made. func (host *BareMetalHost) setLabel(name, value string) bool { @@ -713,12 +671,6 @@ func (host *BareMetalHost) OperationalStatus() OperationalStatus { return host.Status.OperationalStatus } -// HasError returns a boolean indicating whether there is an error -// set for the host. -func (host *BareMetalHost) HasError() bool { - return host.Status.ErrorMessage != "" -} - // CredentialsKey returns a NamespacedName suitable for loading the // Secret containing the credentials associated with the host. func (host *BareMetalHost) CredentialsKey() types.NamespacedName { diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go index dddb47b607..5a005d5d8b 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go @@ -2,7 +2,6 @@ package v1alpha1 import ( "testing" - "time" "github.com/stretchr/testify/assert" @@ -10,71 +9,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestHostAvailable(t *testing.T) { - hostWithError := BareMetalHost{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myhost", - Namespace: "myns", - }, - } - hostWithError.SetErrorMessage(RegistrationError, "oops something went wrong") - - testCases := []struct { - Host BareMetalHost - Expected bool - FailMessage string - }{ - { - Host: BareMetalHost{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myhost", - Namespace: "myns", - }, - }, - Expected: true, - FailMessage: "available host returned not available", - }, - { - Host: hostWithError, - Expected: false, - FailMessage: "host with error returned as available", - }, - { - Host: BareMetalHost{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myhost", - Namespace: "myns", - }, - Spec: BareMetalHostSpec{ - ConsumerRef: &corev1.ObjectReference{ - Name: "mymachine", - Namespace: "myns", - }, - }, - }, - Expected: false, - FailMessage: "host with consumerref returned as available", - }, - { - Host: BareMetalHost{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myhost", - Namespace: "myns", - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - }, - }, - Expected: false, - FailMessage: "deleted host returned as available", - }, - } - - for _, tc := range testCases { - if tc.Host.Available() != tc.Expected { - t.Error(tc.FailMessage) - } - } -} - func TestHostNeedsHardwareInspection(t *testing.T) { testCases := []struct { @@ -554,39 +488,3 @@ func TestBootMode(t *testing.T) { }) } } - -func TestErrorCountIncrementsAlways(t *testing.T) { - - b := &BareMetalHost{} - assert.Equal(t, b.Status.ErrorCount, 0) - - b.SetErrorMessage(RegistrationError, "An error message") - assert.Equal(t, b.Status.ErrorCount, 1) - - b.SetErrorMessage(InspectionError, "Another error message") - assert.Equal(t, b.Status.ErrorCount, 2) -} - -func TestClearErrorCount(t *testing.T) { - - b := &BareMetalHost{ - Status: BareMetalHostStatus{ - ErrorCount: 5, - }, - } - - assert.True(t, b.ClearError()) - assert.Equal(t, 0, b.Status.ErrorCount) -} - -func TestClearErrorCountOnlyIfNotZero(t *testing.T) { - - b := &BareMetalHost{ - Status: BareMetalHostStatus{ - ErrorCount: 5, - }, - } - - assert.True(t, b.ClearError()) - assert.False(t, b.ClearError()) -} diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index a1cf3e9c9d..73fd61b9d6 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -277,7 +277,7 @@ func logResult(info *reconcileInfo, result ctrl.Result) { func recordActionFailure(info *reconcileInfo, errorType metal3v1alpha1.ErrorType, errorMessage string) actionFailed { - info.host.SetErrorMessage(errorType, errorMessage) + setErrorMessage(info.host, errorType, errorMessage) eventType := map[metal3v1alpha1.ErrorType]string{ metal3v1alpha1.RegistrationError: "RegistrationError", @@ -358,6 +358,34 @@ func clearRebootAnnotations(host *metal3v1alpha1.BareMetalHost) (dirty bool) { return } +// clearError removes any existing error message. +func clearError(host *metal3v1alpha1.BareMetalHost) (dirty bool) { + dirty = host.SetOperationalStatus(metal3v1alpha1.OperationalStatusOK) + var emptyErrType metal3v1alpha1.ErrorType = "" + if host.Status.ErrorType != emptyErrType { + host.Status.ErrorType = emptyErrType + dirty = true + } + if host.Status.ErrorMessage != "" { + host.Status.ErrorMessage = "" + dirty = true + } + if host.Status.ErrorCount != 0 { + host.Status.ErrorCount = 0 + dirty = true + } + return dirty +} + +// setErrorMessage updates the ErrorMessage in the host Status struct +// and increases the ErrorCount +func setErrorMessage(host *metal3v1alpha1.BareMetalHost, errType metal3v1alpha1.ErrorType, message string) { + host.Status.OperationalStatus = metal3v1alpha1.OperationalStatusError + host.Status.ErrorType = errType + host.Status.ErrorMessage = message + host.Status.ErrorCount++ +} + // Manage deletion of the host func (r *BareMetalHostReconciler) actionDeleting(prov provisioner.Provisioner, info *reconcileInfo) actionResult { info.log.Info( @@ -424,7 +452,7 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner if provResult.Dirty { info.log.Info("host not ready", "wait", provResult.RequeueAfter) - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -435,7 +463,7 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner registeredNewCreds := !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret) info.host.UpdateGoodCredentials(*info.bmcCredsSecret) info.log.Info("clearing previous error message") - info.host.ClearError() + clearError(info.host) if registeredNewCreds { info.publishEvent("BMCAccessValidated", "Verified access to BMC") @@ -457,7 +485,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner, return recordActionFailure(info, metal3v1alpha1.InspectionError, provResult.ErrorMessage) } - info.host.ClearError() + clearError(info.host) if provResult.Dirty || details == nil { return actionContinue{provResult.RequeueAfter} @@ -507,7 +535,7 @@ func (r *BareMetalHostReconciler) actionMatchProfile(prov provisioner.Provisione info.publishEvent("ProfileSet", fmt.Sprintf("Hardware profile set: %s", hardwareProfile)) } - info.host.ClearError() + clearError(info.host) return actionComplete{} } @@ -541,7 +569,7 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione // Go back into the queue and wait for the Provision() method // to return false, indicating that it has no more work to // do. - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -574,7 +602,7 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage) } if provResult.Dirty { - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -590,7 +618,7 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio } if provResult.Dirty { - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -624,7 +652,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner, } if provResult.Dirty { - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -684,7 +712,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner, } powerChangeAttempts.With(metricLabels).Inc() }) - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -708,7 +736,7 @@ func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provi return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage) } if provResult.Dirty { - info.host.ClearError() + clearError(info.host) return actionContinue{provResult.RequeueAfter} } @@ -729,7 +757,7 @@ func (r *BareMetalHostReconciler) actionManageReady(prov provisioner.Provisioner if dirty { info.log.Info("updating host provisioning settings") } - info.host.ClearError() + clearError(info.host) return actionComplete{} } return r.manageHostPower(prov, info) @@ -792,7 +820,7 @@ func (r *BareMetalHostReconciler) getHostStatusFromAnnotation(host *metal3v1alph func (r *BareMetalHostReconciler) setErrorCondition(request ctrl.Request, host *metal3v1alpha1.BareMetalHost, errType metal3v1alpha1.ErrorType, message string) (err error) { reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName) - host.SetErrorMessage(errType, message) + setErrorMessage(host, errType, message) reqLogger.Info( "adding error message", diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go index 0a550f0ae2..03df5bbc4e 100644 --- a/controllers/metal3.io/baremetalhost_controller_test.go +++ b/controllers/metal3.io/baremetalhost_controller_test.go @@ -169,7 +169,7 @@ func waitForError(t *testing.T, r *BareMetalHostReconciler, host *metal3v1alpha1 tryReconcile(t, r, host, func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool { t.Logf("Waiting for error: %q", host.Status.ErrorMessage) - return host.HasError() + return host.Status.ErrorMessage != "" }, ) } @@ -178,7 +178,7 @@ func waitForNoError(t *testing.T, r *BareMetalHostReconciler, host *metal3v1alph tryReconcile(t, r, host, func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool { t.Logf("Waiting for no error message: %q", host.Status.ErrorMessage) - return !host.HasError() + return host.Status.ErrorMessage == "" }, ) } @@ -1298,3 +1298,39 @@ func TestUpdateEventHandler(t *testing.T) { }) } } + +func TestErrorCountIncrementsAlways(t *testing.T) { + + b := &metal3v1alpha1.BareMetalHost{} + assert.Equal(t, b.Status.ErrorCount, 0) + + setErrorMessage(b, metal3v1alpha1.RegistrationError, "An error message") + assert.Equal(t, b.Status.ErrorCount, 1) + + setErrorMessage(b, metal3v1alpha1.InspectionError, "Another error message") + assert.Equal(t, b.Status.ErrorCount, 2) +} + +func TestClearErrorCount(t *testing.T) { + + b := &metal3v1alpha1.BareMetalHost{ + Status: metal3v1alpha1.BareMetalHostStatus{ + ErrorCount: 5, + }, + } + + assert.True(t, clearError(b)) + assert.Equal(t, 0, b.Status.ErrorCount) +} + +func TestClearErrorCountOnlyIfNotZero(t *testing.T) { + + b := &metal3v1alpha1.BareMetalHost{ + Status: metal3v1alpha1.BareMetalHostStatus{ + ErrorCount: 5, + }, + } + + assert.True(t, clearError(b)) + assert.False(t, clearError(b)) +} diff --git a/controllers/metal3.io/demo_test.go b/controllers/metal3.io/demo_test.go index ba68f96e22..0560323fd9 100644 --- a/controllers/metal3.io/demo_test.go +++ b/controllers/metal3.io/demo_test.go @@ -45,7 +45,7 @@ func TestDemoRegistrationError(t *testing.T) { host.Status.Provisioning.State, host.Status.ErrorMessage, ) - return host.HasError() + return host.Status.ErrorMessage != "" }, ) } @@ -168,7 +168,7 @@ func TestDemoValidationError(t *testing.T) { host.Status.Provisioning.State, host.Status.ErrorMessage, ) - return host.HasError() + return host.Status.ErrorMessage != "" }, ) } diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go index 1835ca4761..9954a38b9e 100644 --- a/controllers/metal3.io/host_state_machine.go +++ b/controllers/metal3.io/host_state_machine.go @@ -290,7 +290,7 @@ func (hsm *hostStateMachine) handleReady(info *reconcileInfo) actionResult { } func (hsm *hostStateMachine) provisioningCancelled() bool { - if hsm.Host.HasError() { + if hsm.Host.Status.ErrorMessage != "" { return true } if hsm.Host.Spec.Image == nil {