Addressed review comments. Fixed unit test.

Signed-off-by: Jacob Anders <[email protected]>

controllers/metal3.io/baremetalhost_controller.go

@@ -1360,32 +1360,24 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 func (r *BareMetalHostReconciler) checkServicing(prov provisioner.Provisioner, info *reconcileInfo, hup *metal3api.HostUpdatePolicy) (result actionResult, isServicing bool) {
 	servicingData := provisioner.ServicingData{}
 
-	var fwDirty bool
-	var hfsDirty bool
-
 	// (NOTE)janders: since Servicing is an opt-in feature that requires HostUpdatePolicy to be created and set to onReboot
 	// set below booleans to false by default and change to true based on policy settings
 
-	servicingData.LiveFirmwareUpdateAllowed = false
-	servicingData.LiveFirmwareSettingsAllowed = false
+	var fwDirty bool
+	var hfsDirty bool
+	var liveFirmwareSettingsAllowed bool
 
 	if hup != nil {
-		if hup.Spec.FirmwareSettings == metal3api.HostUpdatePolicyOnReboot {
-			servicingData.LiveFirmwareSettingsAllowed = true
-		}
-		if hup.Spec.FirmwareUpdates == metal3api.HostUpdatePolicyOnReboot {
-			servicingData.LiveFirmwareUpdateAllowed = true
-		}
+		liveFirmwareSettingsAllowed = (hup.Spec.FirmwareSettings == metal3api.HostUpdatePolicyOnReboot)
 	}
 
-	if servicingData.LiveFirmwareUpdateAllowed {
+	if liveFirmwareSettingsAllowed {
+		// handling pre-HFS FirmwareSettings here
 		if !reflect.DeepEqual(info.host.Status.Provisioning.Firmware, info.host.Spec.Firmware) {
 			servicingData.FirmwareConfig = info.host.Spec.Firmware
 			fwDirty = true
 		}
-	}
-
-	if servicingData.LiveFirmwareSettingsAllowed {
+		// handling HFS based FirmwareSettings here
 		var hfs *metal3api.HostFirmwareSettings
 		var err error
 		hfsDirty, hfs, err = r.getHostFirmwareSettings(info)

controllers/metal3.io/baremetalhost_controller_test.go

@@ -812,7 +812,19 @@ func TestRebootWithServicing(t *testing.T) {
 	}
 	host.Status.Provisioning.Image.URL = "foo"
 
-	r := newTestReconciler(host)
+	// HUP creation
+	hup := &metal3api.HostUpdatePolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      host.Name,
+			Namespace: namespace,
+		},
+		Spec: metal3api.HostUpdatePolicySpec{
+			FirmwareSettings: metal3api.HostUpdatePolicyOnReboot,
+			FirmwareUpdates:  metal3api.HostUpdatePolicyOnReboot,
+		},
+	}
+
+	r := newTestReconciler(host, hup)
 
 	tryReconcile(t, r, host,
 		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
@@ -853,6 +865,53 @@ func TestRebootWithServicing(t *testing.T) {
 	)
 }
 
+// TestRebootWithoutServicing ensures Servicing is not triggered if HostUpdatePolicy doesn't exist.
+func TestRebootWithoutServicing(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Annotations = make(map[string]string)
+	host.Annotations[metal3api.RebootAnnotationPrefix] = ""
+	host.Status.PoweredOn = true
+	host.Status.Provisioning.State = metal3api.StateProvisioned
+	host.Spec.Online = true
+	host.Spec.Image = &metal3api.Image{URL: "foo", Checksum: "123"}
+	host.Spec.Image.URL = "foo"
+	host.Status.Provisioning.Image.URL = "foo"
+	host.Spec.Firmware = &metal3api.FirmwareConfig{
+		VirtualizationEnabled: ptr.To(true),
+	}
+
+	r := newTestReconciler(host)
+
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			return !host.Status.PoweredOn
+		},
+	)
+
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			if _, exists := host.Annotations[metal3api.RebootAnnotationPrefix]; exists {
+				return false
+			}
+
+			return true
+		},
+	)
+
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			return host.Status.PoweredOn
+		},
+	)
+
+	// make sure we don't go into another reboot
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			return host.Status.PoweredOn
+		},
+	)
+}
+
 func getHostSecret(t *testing.T, r *BareMetalHostReconciler, host *metal3api.BareMetalHost) (secret *corev1.Secret) {
 	t.Helper()
 	secret = &corev1.Secret{}