Repository: metal3-io/baremetal-operator
Pull request #1083 (2022-02-11 22:22:46)

Use deep copy/equal for host root device hints
The root device hints have a rotational field which is a pointer to a bool. Use DeepCopy and DeepEqual to ensure its handled properly.


Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 9d5eed0e..577da8e0 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -951,7 +951,7 @@ func (r *BareMetalHostReconciler) actionMatchProfile(prov provisioner.Provisione
 func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("preparing")
 
-	bmhDirty, newStatus, err := getHostProvisioningSettings(info.host)
+	bmhDirty, newStatus, err := getHostProvisioningSettings(info.host, info)
 	if err != nil {
 		return actionError{err}
 	}
@@ -1000,7 +1000,7 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 
 	if bmhDirty && started {
 		info.log.Info("saving host provisioning settings")
-		_, err := saveHostProvisioningSettings(info.host)
+		_, err := saveHostProvisioningSettings(info.host, info)
 		if err != nil {
 			return actionError{errors.Wrap(err, "could not save the host provisioning settings")}
 		}
@@ -1296,9 +1296,9 @@ func (r *BareMetalHostReconciler) actionManageAvailable(prov provisioner.Provisi
 	return r.manageHostPower(prov, info)
 }
 
-func getHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty bool, status *metal3v1alpha1.BareMetalHostStatus, err error) {
+func getHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost, info *reconcileInfo) (dirty bool, status *metal3v1alpha1.BareMetalHostStatus, err error) {
 	hostCopy := host.DeepCopy()
-	dirty, err = saveHostProvisioningSettings(hostCopy)
+	dirty, err = saveHostProvisioningSettings(hostCopy, info)
 	if err != nil {
 		err = errors.Wrap(err, "could not determine the host provisioning settings")
 	}
@@ -1309,22 +1309,23 @@ func getHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty bool
 // saveHostProvisioningSettings copies the values related to
 // provisioning that do not trigger re-provisioning into the status
 // fields of the host.
-func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty bool, err error) {
+func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost, info *reconcileInfo) (dirty bool, err error) {
 
 	// Ensure the root device hints we're going to use are stored.
 	//
 	// If the user has provided explicit root device hints, they take
 	// precedence. Otherwise use the values from the hardware profile.
-	hintSource := host.Spec.RootDeviceHints
+	hintSource := host.Spec.RootDeviceHints.DeepCopy()
 	if hintSource == nil {
 		hwProf, err := hardware.GetProfile(host.HardwareProfile())
 		if err != nil {
 			return false, errors.Wrap(err, "Could not update root device hints")
 		}
-		hintSource = &hwProf.RootDeviceHints
+		hintSource = hwProf.RootDeviceHints.DeepCopy()
 	}
-	if (hintSource != nil && host.Status.Provisioning.RootDeviceHints == nil) || *hintSource != *(host.Status.Provisioning.RootDeviceHints) {
-		host.Status.Provisioning.RootDeviceHints = hintSource
+	if !reflect.DeepEqual(hintSource, host.Status.Provisioning.RootDeviceHints) {
+		host.Status.Provisioning.RootDeviceHints = hintSource.DeepCopy()
+		info.log.Info("RootDeviceHints have changed")
 		dirty = true
 	}
 
@@ -1346,12 +1347,14 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo
 	}
 	if !reflect.DeepEqual(host.Status.Provisioning.RAID, specRAID) {
 		host.Status.Provisioning.RAID = specRAID
+		info.log.Info("RAID settings have changed")
 		dirty = true
 	}
 
 	// Copy BIOS settings
 	if !reflect.DeepEqual(host.Status.Provisioning.Firmware, host.Spec.Firmware) {
 		host.Status.Provisioning.Firmware = host.Spec.Firmware
+		info.log.Info("Firmware settings have changed")
 		dirty = true
 	}
 
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index 3ead90de..7e79b297 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -412,7 +412,7 @@ func (hsm *hostStateMachine) handleAvailable(info *reconcileInfo) actionResult {
 		return actionComplete{}
 	}
 
-	if dirty, _, err := getHostProvisioningSettings(info.host); err != nil {
+	if dirty, _, err := getHostProvisioningSettings(info.host, info); err != nil {
 		return actionError{err}
 	} else if dirty {
 		hsm.NextState = metal3v1alpha1.StatePreparing


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 33e22fc7..b2676762 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -1460,6 +1460,8 @@ func TestDeleteHost(t *testing.T) {
 // precedence rules to the root device hints settings for a host.
 func TestUpdateRootDeviceHints(t *testing.T) {
 	rotational := true
+	rotationalTwo := true
+	rotationalFalse := false
 
 	testCases := []struct {
 		Scenario string
@@ -1489,6 +1491,9 @@ func TestUpdateRootDeviceHints(t *testing.T) {
 						WWNVendorExtension: "userd_vendor_extension",
 						Rotational:         &rotational,
 					},
+					RAID: &metal3v1alpha1.RAIDConfig{
+						SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
+					},
 				},
 				Status: metal3v1alpha1.BareMetalHostStatus{
 					HardwareProfile: "libvirt",
@@ -1550,18 +1555,87 @@ func TestUpdateRootDeviceHints(t *testing.T) {
 				DeviceName: "/dev/sda",
 			},
 		},
+
+		{
+			Scenario: "rotational values same",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					HardwareProfile: "libvirt",
+					RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+						MinSizeGigabytes: 40,
+						Rotational:       &rotational,
+					},
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+					Provisioning: metal3v1alpha1.ProvisionStatus{
+						RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+							MinSizeGigabytes: 40,
+							Rotational:       &rotationalTwo,
+						},
+						RAID: &metal3v1alpha1.RAIDConfig{
+							SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
+						}},
+				},
+			},
+			Dirty: false,
+			Expected: &metal3v1alpha1.RootDeviceHints{
+				MinSizeGigabytes: 40,
+				Rotational:       &rotational,
+			},
+		},
+
+		{
+			Scenario: "rotational values different",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					HardwareProfile: "libvirt",
+					RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+						MinSizeGigabytes: 40,
+						Rotational:       &rotational,
+					},
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+					Provisioning: metal3v1alpha1.ProvisionStatus{
+						RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+							MinSizeGigabytes: 40,
+							Rotational:       &rotationalFalse,
+						},
+						RAID: &metal3v1alpha1.RAIDConfig{
+							SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
+						}},
+				},
+			},
+			Dirty: true,
+			Expected: &metal3v1alpha1.RootDeviceHints{
+				MinSizeGigabytes: 40,
+				Rotational:       &rotational,
+			},
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.Scenario, func(t *testing.T) {
-			dirty, newStatus, err := getHostProvisioningSettings(&tc.Host)
+			info := makeReconcileInfo(&tc.Host)
+			dirty, newStatus, err := getHostProvisioningSettings(&tc.Host, info)
 			if err != nil {
 				t.Fatal(err)
 			}
 			assert.Equal(t, tc.Dirty, dirty, "dirty flag did not match")
 			assert.Equal(t, tc.Expected, newStatus.Provisioning.RootDeviceHints)
 
-			dirty, err = saveHostProvisioningSettings(&tc.Host)
+			dirty, err = saveHostProvisioningSettings(&tc.Host, info)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -1977,10 +2051,11 @@ func TestUpdateRAID(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
 			host.Spec.RAID = c.raid
-			dirty, _ := saveHostProvisioningSettings(&host)
+			info := makeReconcileInfo(&host)
+			dirty, _ := saveHostProvisioningSettings(&host, info)
 			assert.Equal(t, c.dirty, dirty)
 			assert.Equal(t, c.expected, host.Status.Provisioning.RAID)
-			dirty, _, _ = getHostProvisioningSettings(&host)
+			dirty, _, _ = getHostProvisioningSettings(&host, info)
 			assert.Equal(t, false, dirty)
 		})
 	}
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 0b93f10a..c65c8267 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1129,7 +1129,8 @@ func (hb *hostBuilder) build() *metal3v1alpha1.BareMetalHost {
 }
 
 func (hb *hostBuilder) SaveHostProvisioningSettings() *hostBuilder {
-	saveHostProvisioningSettings(&hb.BareMetalHost)
+	info := makeDefaultReconcileInfo(&hb.BareMetalHost)
+	saveHostProvisioningSettings(&hb.BareMetalHost, info)
 	return hb
 }