Repository: metal3-io/baremetal-operator
Pull request #546 (2020-06-04 17:45:04)

remove infinite reconcile loop when fetching status annotation
The problem won't be caused by the operator's version of the annotation, but we're using the annotation to load just the hardware details portion of the status and that is causing reconciliation to continuously copy the status from the annotation to the host and never let the host move past that point.


Patch:
diff --git a/cmd/get-hardware-details/main.go b/cmd/get-hardware-details/main.go
index 5a80be57..066adebb 100644
--- a/cmd/get-hardware-details/main.go
+++ b/cmd/get-hardware-details/main.go
@@ -23,7 +23,7 @@ func main() {
 			IronicInspectorEndpoint: os.Args[1],
 		})
 	if err != nil {
-		fmt.Printf("could not get inpsector client: %s", err)
+		fmt.Printf("could not get inspector client: %s", err)
 		os.Exit(1)
 	}
 
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 382bdae9..101bea83 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -207,6 +207,13 @@ func (r *ReconcileBareMetalHost) Reconcile(request reconcile.Request) (result re
 		objStatus, err := r.getHostStatusFromAnnotation(host)
 		if err == nil && objStatus != nil {
 			host.Status = *objStatus
+			if host.Status.LastUpdated.IsZero() {
+				// Ensure the LastUpdated timestamp in set to avoid
+				// infinite loops if the annotation only contained
+				// part of the status information.
+				t := metav1.Now()
+				host.Status.LastUpdated = &t
+			}
 			errStatus := r.client.Status().Update(context.TODO(), host)
 			if errStatus != nil {
 				return reconcile.Result{}, errors.Wrap(err, "Could not restore status from annotation")
@@ -866,7 +873,7 @@ func (r *ReconcileBareMetalHost) saveHostAnnotation(host *metal3v1alpha1.BareMet
 	}
 
 	delete(host.Annotations, metal3v1alpha1.StatusAnnotation)
-	newAnnotation, err := json.Marshal(host.Status)
+	newAnnotation, err := marshalStatusAnnotation(&host.Status)
 	if err != nil {
 		return err
 	}
@@ -877,6 +884,22 @@ func (r *ReconcileBareMetalHost) saveHostAnnotation(host *metal3v1alpha1.BareMet
 	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) {
+	objStatus := &metal3v1alpha1.BareMetalHostStatus{}
+	if err := json.Unmarshal(content, objStatus); err != nil {
+		return nil, errors.Wrap(err, "Failed to fetch Status from annotation")
+	}
+	return objStatus, nil
+}
+
 // extract host from Status annotation
 func (r *ReconcileBareMetalHost) getHostStatusFromAnnotation(host *metal3v1alpha1.BareMetalHost) (*metal3v1alpha1.BareMetalHostStatus, error) {
 	annotations := host.GetAnnotations()
@@ -884,9 +907,9 @@ func (r *ReconcileBareMetalHost) getHostStatusFromAnnotation(host *metal3v1alpha
 	if annotations[metal3v1alpha1.StatusAnnotation] == "" {
 		return nil, nil
 	}
-	objStatus := &metal3v1alpha1.BareMetalHostStatus{}
-	if err := json.Unmarshal(content, objStatus); err != nil {
-		return nil, errors.Wrap(err, "Failed to fetch Status from annotation")
+	objStatus, err := unmarshalStatusAnnotation(content)
+	if err != nil {
+		return nil, err
 	}
 	return objStatus, nil
 }


Test patch:
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 8fd7b8db..df47234b 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -30,7 +30,7 @@ import (
 const (
 	namespace         string = "test-namespace"
 	defaultSecretName string = "bmc-creds-valid"
-	statusAnnotaion   string = `{"operationalStatus":"OK","lastUpdated":"2020-04-15T15:00:50Z","hardwareProfile":"StatusProfile","hardware":{"systemVendor":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true},{"name":"eth1","model":"0x1af4  0x0001","mac":"00:b7:8b:bb:3d:f8","ip":"192.168.111.20","speedGbps":0,"vlanId":0,"pxe":false}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["aes","apic","arat","avx","clflush","cmov","constant_tsc","cx16","cx8","de","eagerfpu","ept","erms","f16c","flexpriority","fpu","fsgsbase","fxsr","hypervisor","lahf_lm","lm","mca","mce","mmx","msr","mtrr","nopl","nx","pae","pat","pclmulqdq","pge","pni","popcnt","pse","pse36","rdrand","rdtscp","rep_good","sep","smep","sse","sse2","sse4_1","sse4_2","ssse3","syscall","tpr_shadow","tsc","tsc_adjust","tsc_deadline_timer","vme","vmx","vnmi","vpid","x2apic","xsave","xsaveopt","xtopology"],"count":4},"hostname":"node-0"},"provisioning":{"state":"provisioned","ID":"8a0ede17-7b87-44ac-9293-5b7d50b94b08","image":{"url":"bar","checksum":""}},"goodCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"triedCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"errorMessage":"","poweredOn":true,"operationHistory":{"register":{"start":"2020-04-15T12:06:26Z","end":"2020-04-15T12:07:12Z"},"inspect":{"start":"2020-04-15T12:07:12Z","end":"2020-04-15T12:09:29Z"},"provision":{"start":null,"end":null},"deprovision":{"start":null,"end":null}}}`
+	statusAnnotation  string = `{"operationalStatus":"OK","lastUpdated":"2020-04-15T15:00:50Z","hardwareProfile":"StatusProfile","hardware":{"systemVendor":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true},{"name":"eth1","model":"0x1af4  0x0001","mac":"00:b7:8b:bb:3d:f8","ip":"192.168.111.20","speedGbps":0,"vlanId":0,"pxe":false}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["aes","apic","arat","avx","clflush","cmov","constant_tsc","cx16","cx8","de","eagerfpu","ept","erms","f16c","flexpriority","fpu","fsgsbase","fxsr","hypervisor","lahf_lm","lm","mca","mce","mmx","msr","mtrr","nopl","nx","pae","pat","pclmulqdq","pge","pni","popcnt","pse","pse36","rdrand","rdtscp","rep_good","sep","smep","sse","sse2","sse4_1","sse4_2","ssse3","syscall","tpr_shadow","tsc","tsc_adjust","tsc_deadline_timer","vme","vmx","vnmi","vpid","x2apic","xsave","xsaveopt","xtopology"],"count":4},"hostname":"node-0"},"provisioning":{"state":"provisioned","ID":"8a0ede17-7b87-44ac-9293-5b7d50b94b08","image":{"url":"bar","checksum":""}},"goodCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"triedCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"errorMessage":"","poweredOn":true,"operationHistory":{"register":{"start":"2020-04-15T12:06:26Z","end":"2020-04-15T12:07:12Z"},"inspect":{"start":"2020-04-15T12:07:12Z","end":"2020-04-15T12:09:29Z"},"provision":{"start":null,"end":null},"deprovision":{"start":null,"end":null}}}`
 )
 
 func init() {
@@ -202,7 +202,7 @@ func waitForProvisioningState(t *testing.T, r *ReconcileBareMetalHost, host *met
 func TestStatusAnnotation_EmptyStatus(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		metal3v1alpha1.StatusAnnotation: statusAnnotaion,
+		metal3v1alpha1.StatusAnnotation: statusAnnotation,
 	}
 	host.Spec.Online = true
 	host.Spec.Image = &metal3v1alpha1.Image{URL: "foo", Checksum: "123"}
@@ -224,7 +224,7 @@ func TestStatusAnnotation_EmptyStatus(t *testing.T) {
 func TestStatusAnnotation_StatusPresent(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		metal3v1alpha1.StatusAnnotation: statusAnnotaion,
+		metal3v1alpha1.StatusAnnotation: statusAnnotation,
 	}
 	host.Spec.Online = true
 	time := metav1.Now()
@@ -242,7 +242,44 @@ func TestStatusAnnotation_StatusPresent(t *testing.T) {
 	)
 }
 
-// TestStatusAnnotation tests if statusAnnotaion is populated correctly
+// TestStatusAnnotation_Partial ensures that if the status annotation
+// does not include the LastUpdated value reconciliation does not go
+// into an infinite loop.
+func TestStatusAnnotation_Partial(t *testing.T) {
+	// Build a version of the annotation text that does not include
+	// a LastUpdated value.
+	unpackedStatus, err := unmarshalStatusAnnotation([]byte(statusAnnotation))
+	if err != nil {
+		t.Fatal(err)
+		return
+	}
+	unpackedStatus.LastUpdated = nil
+	packedStatus, err := marshalStatusAnnotation(unpackedStatus)
+	if err != nil {
+		t.Fatal(err)
+		return
+	}
+
+	host := newDefaultHost(t)
+	host.Annotations = map[string]string{
+		metal3v1alpha1.StatusAnnotation: string(packedStatus),
+	}
+	host.Spec.Online = true
+	host.Spec.Image = &metal3v1alpha1.Image{URL: "foo", Checksum: "123"}
+
+	r := newTestReconciler(host)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			if host.Status.HardwareProfile == "StatusProfile" && host.Status.Provisioning.Image.URL == "bar" {
+				return true
+			}
+			return false
+		},
+	)
+}
+
+// TestStatusAnnotation tests if statusAnnotation is populated correctly
 func TestStatusAnnotation(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Spec.Online = true