Repository: metal3-io/baremetal-operator
Pull request #571 (2020-07-02 17:20:14)

define the Unmanaged state
Define a new state for newly-created Hosts with no BMC details.

We need the state definition so we can add it to CAPM3 so that does
not break when #569 merges.

See https://github.com/metal3-io/metal3-docs/pull/120 for more
details.


Patch:
diff --git a/docs/baremetalhost-states.md b/docs/baremetalhost-states.md
index 639875e9..9cffc2b1 100644
--- a/docs/baremetalhost-states.md
+++ b/docs/baremetalhost-states.md
@@ -11,10 +11,10 @@ Newly created hosts move immediately to Discovered or Registering. No
 host stays in the Created state while the operator is working
 properly.
 
-## Discovered
+## Unmanaged
 
-A Discovered host is missing either the BMC address or credentials
-secret name, and does not have enough information to access the BMC
+An Unmanaged host is missing both the BMC address and credentials
+secret name, and does not have any information to access the BMC
 for registration.
 
 ## Externally Provisioned
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index 945b7c8d..f419850b 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -119,6 +119,10 @@ const (
 	// StateNone means the state is unknown
 	StateNone ProvisioningState = ""
 
+	// StateUnmanaged means there is insufficient information available to
+	// register the host
+	StateUnmanaged ProvisioningState = "unmanaged"
+
 	// StateRegistrationError means there was an error registering the
 	// host with the backend
 	StateRegistrationError ProvisioningState = "registration error"
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 9590c1f3..c4976212 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -6,6 +6,7 @@ import (
 	"flag"
 	"fmt"
 	"os"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
@@ -220,17 +221,6 @@ func (r *ReconcileBareMetalHost) Reconcile(request reconcile.Request) (result re
 			return reconcile.Result{Requeue: true}, nil
 		}
 		reqLogger.Info("No status cache found")
-	} else {
-		//The status annotation is unneeded, as the status is present, and it will get outdated, so removing it
-		objStatus, err := r.getHostStatusFromAnnotation(host)
-		if err == nil && objStatus != nil {
-			delete(annotations, metal3v1alpha1.StatusAnnotation)
-			errStatus := r.client.Update(context.TODO(), host)
-			if errStatus != nil {
-				return reconcile.Result{}, errors.Wrap(err, "Could not delete status annotation")
-			}
-			return reconcile.Result{Requeue: true}, nil
-		}
 	}
 
 	// NOTE(dhellmann): Handle a few steps outside of the phase
@@ -299,11 +289,14 @@ func (r *ReconcileBareMetalHost) Reconcile(request reconcile.Request) (result re
 		info.log.Info("saving host status",
 			"operational status", host.OperationalStatus(),
 			"provisioning state", host.Status.Provisioning.State)
-		err := r.saveHostStatus(host)
+		requeueNeeded, err := r.saveHostStatus(host)
 		if err != nil {
 			return reconcile.Result{}, errors.Wrap(err,
 				fmt.Sprintf("failed to save host status after %q", initialState))
 		}
+		if requeueNeeded {
+			return reconcile.Result{Requeue: true}, nil
+		}
 		info.log.Info("Updated Status")
 
 		for _, cb := range info.postSaveCallbacks {
@@ -366,8 +359,8 @@ func (r *ReconcileBareMetalHost) credentialsErrorResult(err error, request recon
 			// overwrites our discovered state
 			host.Status.ErrorMessage = err.Error()
 			host.Status.ErrorType = ""
-			saveErr := r.saveHostStatus(host)
-			if saveErr != nil {
+			requeueNeeded, saveErr := r.saveHostStatus(host)
+			if saveErr != nil || requeueNeeded {
 				return reconcile.Result{Requeue: true}, saveErr
 			}
 			// Only publish the event if we do not have an error
@@ -381,11 +374,13 @@ func (r *ReconcileBareMetalHost) credentialsErrorResult(err error, request recon
 	// at some point in the future.
 	case *ResolveBMCSecretRefError:
 		credentialsMissing.Inc()
-		changed, saveErr := r.setErrorCondition(request, host, metal3v1alpha1.RegistrationError, err.Error())
+		changed, requeue, saveErr := r.setErrorCondition(request, host, metal3v1alpha1.RegistrationError, err.Error())
 		if saveErr != nil {
 			return reconcile.Result{Requeue: true}, saveErr
 		} else if !changed {
 			return reconcile.Result{Requeue: true, RequeueAfter: hostErrorRetryDelay}, nil
+		} else if requeue {
+			return reconcile.Result{Requeue: true}, nil
 		}
 		// Only publish the event if we do not have an error
 		// after saving the first time so that we only publish one time, requeue immediately to save the status
@@ -398,11 +393,13 @@ func (r *ReconcileBareMetalHost) credentialsErrorResult(err error, request recon
 	// the host to be reconciled again
 	case *bmc.CredentialsValidationError, *bmc.UnknownBMCTypeError:
 		credentialsInvalid.Inc()
-		changed, saveErr := r.setErrorCondition(request, host, metal3v1alpha1.RegistrationError, err.Error())
+		changed, requeue, saveErr := r.setErrorCondition(request, host, metal3v1alpha1.RegistrationError, err.Error())
 		if saveErr != nil {
 			return reconcile.Result{Requeue: true}, saveErr
 		} else if !changed {
 			return reconcile.Result{}, nil
+		} else if requeue {
+			return reconcile.Result{Requeue: true}, nil
 		}
 		// Only publish the event if we do not have an error
 		// after saving so that we only publish one time. Requeue immediately to save the status
@@ -846,11 +843,61 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo
 	return
 }
 
-func (r *ReconcileBareMetalHost) saveHostStatus(host *metal3v1alpha1.BareMetalHost) error {
+func (r *ReconcileBareMetalHost) saveHostStatus(host *metal3v1alpha1.BareMetalHost) (bool, error) {
 	t := metav1.Now()
 	host.Status.LastUpdated = &t
 
-	return r.client.Status().Update(context.TODO(), host)
+	if err := r.saveHostAnnotation(host); err != nil {
+		if k8serrors.IsConflict(err) {
+			log.Info("Failed to update status annotation because of a conflict, requeuing", "host", host.Name)
+			return true, nil
+		}
+		return false, errors.Wrap(err, "Failed to update Status annotation")
+	}
+
+	if err := r.client.Status().Update(context.TODO(), host); err != nil {
+		if k8serrors.IsConflict(err) {
+			log.Info("Failed to update status because of a conflict, requeuing", "host", host.Name)
+			return true, nil
+		}
+		return false, errors.Wrap(err, "Failed to update Status")
+	}
+	return false, nil
+}
+
+func (r *ReconcileBareMetalHost) saveHostAnnotation(host *metal3v1alpha1.BareMetalHost) error {
+	//Repopulate annotation again
+	objStatus, err := r.getHostStatusFromAnnotation(host)
+	if err != nil {
+		return err
+	}
+
+	if objStatus != nil {
+		// These values are copied to avoid continually updating the annotation
+		objStatus.LastUpdated = host.Status.LastUpdated
+		objStatus.OperationHistory = host.Status.OperationHistory
+		if reflect.DeepEqual(host.Status, *objStatus) {
+			return nil
+		}
+	}
+
+	newAnnotation, err := marshalStatusAnnotation(&host.Status)
+	if err != nil {
+		return err
+	}
+	if host.Annotations == nil {
+		host.Annotations = make(map[string]string)
+	}
+	host.Annotations[metal3v1alpha1.StatusAnnotation] = string(newAnnotation)
+	return r.client.Update(context.TODO(), host.DeepCopy())
+}
+
+func marshalStatusAnnotation(status *metal3v1alpha1.BareMetalHostStatus) ([]byte, error) {
+	newAnnotation, err := json.Marshal(status)
+	if err != nil {
+		return []byte{}, errors.Wrap(err, "failed to marshall status annotation")
+	}
+	return newAnnotation, nil
 }
 
 func unmarshalStatusAnnotation(content []byte) (*metal3v1alpha1.BareMetalHostStatus, error) {
@@ -875,7 +922,7 @@ func (r *ReconcileBareMetalHost) getHostStatusFromAnnotation(host *metal3v1alpha
 	return objStatus, nil
 }
 
-func (r *ReconcileBareMetalHost) setErrorCondition(request reconcile.Request, host *metal3v1alpha1.BareMetalHost, errType metal3v1alpha1.ErrorType, message string) (changed bool, err error) {
+func (r *ReconcileBareMetalHost) setErrorCondition(request reconcile.Request, host *metal3v1alpha1.BareMetalHost, errType metal3v1alpha1.ErrorType, message string) (changed, requeue bool, err error) {
 	reqLogger := log.WithValues("Request.Namespace",
 		request.Namespace, "Request.Name", request.Name)
 
@@ -886,7 +933,7 @@ func (r *ReconcileBareMetalHost) setErrorCondition(request reconcile.Request, ho
 			"message", message,
 		)
 		// We might need to requeue if we failed to update the status
-		err = r.saveHostStatus(host)
+		requeue, err = r.saveHostStatus(host)
 		if err != nil {
 			err = errors.Wrap(err, "failed to update error message")
 		}


Test patch:
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 798f0a05..df47234b 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -3,8 +3,8 @@ package baremetalhost
 import (
 	goctx "context"
 	"encoding/base64"
-	"encoding/json"
 	"fmt"
+	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -220,7 +220,7 @@ func TestStatusAnnotation_EmptyStatus(t *testing.T) {
 }
 
 // TestStatusAnnotation_StatusPresent tests that if status is present
-// status annotation is ignored and deleted.
+// status annotation is ignored.
 func TestStatusAnnotation_StatusPresent(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
@@ -234,8 +234,7 @@ func TestStatusAnnotation_StatusPresent(t *testing.T) {
 
 	tryReconcile(t, r, host,
 		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
-			_, found := host.Annotations[metal3v1alpha1.StatusAnnotation]
-			if host.Status.HardwareProfile != "StatusProfile" && host.Status.Provisioning.Image.URL == "foo" && !found {
+			if host.Status.HardwareProfile != "StatusProfile" && host.Status.Provisioning.Image.URL == "foo" {
 				return true
 			}
 			return false
@@ -255,7 +254,7 @@ func TestStatusAnnotation_Partial(t *testing.T) {
 		return
 	}
 	unpackedStatus.LastUpdated = nil
-	packedStatus, err := json.Marshal(unpackedStatus)
+	packedStatus, err := marshalStatusAnnotation(unpackedStatus)
 	if err != nil {
 		t.Fatal(err)
 		return
@@ -280,6 +279,37 @@ func TestStatusAnnotation_Partial(t *testing.T) {
 	)
 }
 
+// TestStatusAnnotation tests if statusAnnotation is populated correctly
+func TestStatusAnnotation(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Spec.Online = true
+	host.Spec.Image = &metal3v1alpha1.Image{URL: "foo", Checksum: "123"}
+	bmcSecret := newBMCCredsSecret(defaultSecretName, "User", "Pass")
+	r := newTestReconciler(host, bmcSecret)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			if utils.StringInList(host.Finalizers, metal3v1alpha1.BareMetalHostFinalizer) {
+				return true
+			}
+			return false
+		},
+	)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			objStatus, _ := r.getHostStatusFromAnnotation(host)
+			objStatus.LastUpdated = host.Status.LastUpdated
+
+			if reflect.DeepEqual(host.Status, *objStatus) {
+				return true
+			}
+			return false
+		},
+	)
+
+}
+
 // TestPause ensures that the requeue happens when the pause annotation is there.
 func TestPause(t *testing.T) {
 	host := newDefaultHost(t)