remove convenience methods from BareMetalHost

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 {

apis/metal3.io/v1alpha1/baremetalhost_types_test.go

@@ -2,79 +2,13 @@ package v1alpha1
 
 import (
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 
 	corev1 "k8s.io/api/core/v1"
 	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())
-}

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",

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))
+}