Repository: metal3-io/baremetal-operator
Pull request #741 (2020-12-07 18:11:40)

remove convenience methods from BareMetalHost
It is up to the consumer, not the host, to decide when a host is ready
to have provisioning instructions added. The checks in the Available()
method will be inlined in cluster-api-provider-metal3.

Similarly, the host.Status.ErrorMessage field is the API for determining if a host has an error, so remove HasError().

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/741#" title="Author self-approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
zaneb: /approve
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/741#" title="Author self-approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/741#issuecomment-740370420" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann,zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dhellmann: /test-integration
s3rj1k: @dhellmann I am assuming that PR should be merged also as related https://github.com/metal3-io/cluster-api-provider-metal3/pull/150 is merged already.

Some one needs to trigger `test-integration` for this PR.
dhellmann: > @dhellmann I am assuming that this PR should be merged as related [metal3-io/cluster-api-provider-metal3#150](https://github.com/metal3-io/cluster-api-provider-metal3/pull/150) is merged already.
> 
> Some one needs to trigger `test-integration` for this PR.

Yes, it should be safe to merge this now.

/test-integration

metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
manifestlint | 60f461012c721d7adb38833b57dfc1e028d8749f | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/741/manifestlint/1338412195800682496) | `/test manifestlint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index db492beb..dc279ead 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/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index a1cf3e9c..73fd61b9 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/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index 1835ca47..9954a38b 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 {


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go
index dddb47b6..5a005d5d 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_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 0a550f0a..03df5bbc 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 ba68f96e..0560323f 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 != ""
 		},
 	)
 }