Repository: metal3-io/baremetal-operator
Pull request #882 (2021-05-17 16:31:58)

Do automated cleaning in Prepare state
As [proposed on the mailing list](https://groups.google.com/g/metal3-dev/c/N-EpqQn94bw):

After completing manual cleaning, move the Node to the available state
before returning the Host to the Ready state. Previously we left it in
manageable; this transition involves doing automated cleaning. This
should mean that a Host in the Ready/Available state should usually
(with a few exceptions) represent a Node in the available state.

On Hosts that are created with provisioning information specified, this
will have little real-world effect. However, on Hosts where provisioning
information is specified later, this should make provisioning quicker
because automated cleaning will already have been performed.


Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 6086095c..40eeaf9d 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -750,19 +750,17 @@ func (r *BareMetalHostReconciler) actionMatchProfile(prov provisioner.Provisione
 func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("preparing")
 
-	// Save provisioning settings.
-	provisioningSettings := info.host.Status.Provisioning.DeepCopy()
-	dirty, err := saveHostProvisioningSettings(info.host)
+	dirty, newStatus, err := getHostProvisioningSettings(info.host)
 	if err != nil {
-		return actionError{errors.Wrap(err, "Could not save the host provisioning settings")}
+		return actionError{err}
 	}
 
 	prepareData := provisioner.PrepareData{
-		RAIDConfig:      info.host.Status.Provisioning.RAID.DeepCopy(),
-		RootDeviceHints: info.host.Status.Provisioning.RootDeviceHints.DeepCopy(),
+		RAIDConfig:      newStatus.Provisioning.RAID.DeepCopy(),
+		RootDeviceHints: newStatus.Provisioning.RootDeviceHints.DeepCopy(),
 	}
-	// Do prepare(manual clean).
-	provResult, started, err := prov.Prepare(prepareData, dirty)
+	provResult, started, err := prov.Prepare(prepareData,
+		dirty || info.host.Status.ErrorType == metal3v1alpha1.PreparationError)
 	if err != nil {
 		return actionError{errors.Wrap(err, "error preparing host")}
 	}
@@ -773,19 +771,24 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 		return recordActionFailure(info, metal3v1alpha1.PreparationError, provResult.ErrorMessage)
 	}
 
+	if dirty && started {
+		info.log.Info("saving host provisioning settings")
+		_, err := saveHostProvisioningSettings(info.host)
+		if err != nil {
+			return actionError{errors.Wrap(err, "could not save the host provisioning settings")}
+		}
+	}
+	if started && clearError(info.host) {
+		dirty = true
+	}
 	if provResult.Dirty {
 		result := actionContinue{provResult.RequeueAfter}
-		if clearError(info.host) || (dirty && started) {
-			// If clearError return true, but started is false, restore provisioningSettings.
-			if dirty && !started {
-				info.host.Status.Provisioning = *provisioningSettings
-			}
+		if dirty {
 			return actionUpdate{result}
 		}
 		return result
 	}
 
-	clearError(info.host)
 	return actionComplete{}
 }
 
@@ -1036,22 +1039,22 @@ func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provi
 // having been provisioned. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageReady(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	if info.host.NeedsProvisioning() {
-		// Ensure the provisioning settings we're going to use are stored.
-		dirty, err := saveHostProvisioningSettings(info.host)
-		if err != nil {
-			return actionError{errors.Wrap(err, "Could not save the host provisioning settings")}
-		}
-		if dirty {
-			info.log.Info("Host provisioning settings have been updated, go back to Preparing state")
-			clearHostProvisioningSettings(info.host)
-			return actionUpdate{}
-		}
 		clearError(info.host)
 		return actionComplete{}
 	}
 	return r.manageHostPower(prov, info)
 }
 
+func getHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty bool, status *metal3v1alpha1.BareMetalHostStatus, err error) {
+	hostCopy := host.DeepCopy()
+	dirty, err = saveHostProvisioningSettings(hostCopy)
+	if err != nil {
+		err = errors.Wrap(err, "could not determine the host provisioning settings")
+	}
+	status = &hostCopy.Status
+	return
+}
+
 // saveHostProvisioningSettings copies the values related to
 // provisioning that do not trigger re-provisioning into the status
 // fields of the host.
@@ -1204,20 +1207,6 @@ func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(request ctrl.Request,
 	return bmcCredsSecret, nil
 }
 
-func credentialsFromSecret(bmcCredsSecret *corev1.Secret) *bmc.Credentials {
-	// We trim surrounding whitespace because those characters are
-	// unlikely to be part of the username or password and it is
-	// common for users to encode the values with a command like
-	//
-	//     echo "my-password" | base64
-	//
-	// which introduces a trailing newline.
-	return &bmc.Credentials{
-		Username: strings.TrimSpace(string(bmcCredsSecret.Data["username"])),
-		Password: strings.TrimSpace(string(bmcCredsSecret.Data["password"])),
-	}
-}
-
 // Make sure the credentials for the management controller look
 // right and manufacture bmc.Credentials.  This does not actually try
 // to use the credentials.
@@ -1235,7 +1224,10 @@ func (r *BareMetalHostReconciler) buildAndValidateBMCCredentials(request ctrl.Re
 		return nil, nil, &EmptyBMCAddressError{message: "Missing BMC connection detail 'Address'"}
 	}
 
-	bmcCreds = credentialsFromSecret(bmcCredsSecret)
+	bmcCreds = &bmc.Credentials{
+		Username: string(bmcCredsSecret.Data["username"]),
+		Password: string(bmcCredsSecret.Data["password"]),
+	}
 
 	// Verify that the secret contains the expected info.
 	err = bmcCreds.Validate()
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index a35ce906..a6753f67 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -408,11 +408,16 @@ func (hsm *hostStateMachine) handleReady(info *reconcileInfo) actionResult {
 		return actionComplete{}
 	}
 
+	if dirty, _, err := getHostProvisioningSettings(info.host); err != nil {
+		return actionError{err}
+	} else if dirty {
+		hsm.NextState = metal3v1alpha1.StatePreparing
+		return actionComplete{}
+	}
+
 	// ErrorCount is cleared when appropriate inside actionManageReady
 	actResult := hsm.Reconciler.actionManageReady(hsm.Provisioner, info)
-	if _, update := actResult.(actionUpdate); update {
-		hsm.NextState = metal3v1alpha1.StatePreparing
-	} else if _, complete := actResult.(actionComplete); complete {
+	if _, complete := actResult.(actionComplete); complete {
 		hsm.NextState = metal3v1alpha1.StateProvisioning
 	}
 	return actResult
diff --git a/docs/api.md b/docs/api.md
index 72de94cd..ba1bef00 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -475,10 +475,6 @@ data:
   password: cGFzc3dvcmQ=
 ```
 
-NOTE: After decoding the secret content, whitespace is stripped from
-the beginning and end before the username and password values are
-used.
-
 ## Triggering Provisioning
 
 Several conditions must be met in order to initiate provisioning.
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 827b5578..783b4c38 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -197,14 +197,16 @@ func (p *demoProvisioner) Prepare(data provisioner.PrepareData, unprepared bool)
 
 	case PreparingHost:
 		p.log.Info("preparing host")
+		started = unprepared
 		result.Dirty = true
 		result.RequeueAfter = time.Second * 5
 
 	default:
 		p.log.Info("finished preparing")
+		started = true
 	}
 
-	return result, false, nil
+	return
 }
 
 // Adopt notifies the provisioner that the state machine believes the host
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 2b2b6a62..70d9dbcc 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -186,6 +186,7 @@ func (p *fixtureProvisioner) UpdateHardwareState() (hwState provisioner.Hardware
 // Prepare remove existing configuration and set new configuration
 func (p *fixtureProvisioner) Prepare(data provisioner.PrepareData, unprepared bool) (result provisioner.Result, started bool, err error) {
 	p.log.Info("preparing host")
+	started = unprepared
 	return
 }
 
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index f71b58d5..4bd0aa81 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1151,28 +1151,40 @@ func (p *ironicProvisioner) Prepare(data provisioner.PrepareData, unprepared boo
 
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
 	case nodes.Available:
-		var cleanSteps []nodes.CleanStep
-		cleanSteps, err = p.buildManualCleaningSteps(bmcAccess, data)
-		if err != nil {
-			result, err = operationFailed(err.Error())
-			return
-		}
-		if unprepared && len(cleanSteps) != 0 {
-			result, err = p.changeNodeProvisionState(
-				ironicNode,
-				nodes.ProvisionStateOpts{Target: nodes.TargetManage},
-			)
-			return
+		if unprepared {
+			var cleanSteps []nodes.CleanStep
+			cleanSteps, err = p.buildManualCleaningSteps(bmcAccess, data)
+			if err != nil {
+				result, err = operationFailed(err.Error())
+				return
+			}
+			if len(cleanSteps) != 0 {
+				result, err = p.changeNodeProvisionState(
+					ironicNode,
+					nodes.ProvisionStateOpts{Target: nodes.TargetManage},
+				)
+				return
+			}
+			// nothing to do
+			started = true
 		}
+		// Automated clean finished
 		result, err = operationComplete()
 
 	case nodes.Manageable:
 		if unprepared {
 			started, result, err = p.startManualCleaning(bmcAccess, ironicNode, data)
-			return
+			if started || result.Dirty || result.ErrorMessage != "" || err != nil {
+				return
+			}
+			// nothing to do
+			started = true
 		}
 		// Manual clean finished
-		result, err = operationComplete()
+		result, err = p.changeNodeProvisionState(
+			ironicNode,
+			nodes.ProvisionStateOpts{Target: nodes.TargetProvide},
+		)
 
 	case nodes.CleanFail:
 		// When clean failed, we need to clean host provisioning settings.


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 91006bb8..21282042 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -23,7 +23,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
-	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/metal3-io/baremetal-operator/pkg/utils"
 )
@@ -1311,7 +1310,14 @@ func TestUpdateRootDeviceHints(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.Scenario, func(t *testing.T) {
-			dirty, err := saveHostProvisioningSettings(&tc.Host)
+			dirty, newStatus, err := getHostProvisioningSettings(&tc.Host)
+			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)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -2001,63 +2007,3 @@ func TestInvalidBMHCanBeDeleted(t *testing.T) {
 		return host.Status.Provisioning.State == metal3v1alpha1.StateDeleting && len(host.Finalizers) == 0
 	})
 }
-
-func TestCredentialsFromSecret(t *testing.T) {
-	cases := []struct {
-		name     string
-		input    corev1.Secret
-		expected bmc.Credentials
-	}{
-		{
-			name:     "empty",
-			input:    corev1.Secret{},
-			expected: bmc.Credentials{},
-		},
-		{
-			name: "clean",
-			input: corev1.Secret{
-				Data: map[string][]byte{
-					"username": []byte("username"),
-					"password": []byte("password"),
-				},
-			},
-			expected: bmc.Credentials{
-				Username: "username",
-				Password: "password",
-			},
-		},
-		{
-			name: "newline",
-			input: corev1.Secret{
-				Data: map[string][]byte{
-					"username": []byte("username\n"),
-					"password": []byte("password\n"),
-				},
-			},
-			expected: bmc.Credentials{
-				Username: "username",
-				Password: "password",
-			},
-		},
-		{
-			name: "non-newline",
-			input: corev1.Secret{
-				Data: map[string][]byte{
-					"username": []byte(" username\t"),
-					"password": []byte(" password\t"),
-				},
-			},
-			expected: bmc.Credentials{
-				Username: "username",
-				Password: "password",
-			},
-		},
-	}
-
-	for _, c := range cases {
-		t.Run(c.name, func(t *testing.T) {
-			actual := credentialsFromSecret(&c.input)
-			assert.Equal(t, c.expected, *actual)
-		})
-	}
-}
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index e597de48..db100767 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -307,7 +307,7 @@ func TestDetach(t *testing.T) {
 			ExpectedDetach:            false,
 			ExpectedDirty:             true,
 			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusOK,
-			ExpectedState:             metal3v1alpha1.StatePreparing,
+			ExpectedState:             metal3v1alpha1.StateProvisioning,
 		},
 		{
 			Scenario:                  "PreparingHost",
@@ -325,7 +325,7 @@ func TestDetach(t *testing.T) {
 			ExpectedDetach:            false,
 			ExpectedDirty:             true,
 			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusOK,
-			ExpectedState:             metal3v1alpha1.StatePreparing,
+			ExpectedState:             metal3v1alpha1.StateProvisioning,
 		},
 		{
 			Scenario:                  "ProvisioningHost",
@@ -613,12 +613,12 @@ func TestErrorCountIncreasedOnActionFailure(t *testing.T) {
 		},
 		{
 			Scenario:           "ready-power-on",
-			Host:               host(metal3v1alpha1.StateReady).SetStatusImageURL("imageSpecUrl").SetStatusPoweredOn(false).build(),
+			Host:               host(metal3v1alpha1.StateReady).SetImageURL("").SetStatusPoweredOn(false).build(),
 			ProvisionerErrorOn: "PowerOn",
 		},
 		{
 			Scenario:           "ready-power-off",
-			Host:               host(metal3v1alpha1.StateReady).SetStatusImageURL("imageSpecUrl").SetOnline(false).build(),
+			Host:               host(metal3v1alpha1.StateReady).SetImageURL("").SetOnline(false).build(),
 			ProvisionerErrorOn: "PowerOff",
 		},
 		{
@@ -642,6 +642,7 @@ func TestErrorCountIncreasedOnActionFailure(t *testing.T) {
 			result := hsm.ReconcileState(info)
 
 			assert.Equal(t, 1, tt.Host.Status.ErrorCount)
+			assert.Equal(t, "some error", tt.Host.Status.ErrorMessage)
 			assert.True(t, result.Dirty())
 		})
 	}
@@ -778,8 +779,9 @@ func host(state metal3v1alpha1.ProvisioningState) *hostBuilder {
 			Status: metal3v1alpha1.BareMetalHostStatus{
 				HardwareProfile: hardware.DefaultProfileName,
 				Provisioning: metal3v1alpha1.ProvisionStatus{
-					State:    state,
-					BootMode: v1alpha1.DefaultBootMode,
+					State:           state,
+					BootMode:        v1alpha1.DefaultBootMode,
+					RootDeviceHints: &metal3v1alpha1.RootDeviceHints{},
 					Image: v1alpha1.Image{
 						URL: "", //needs provisioning
 					},
diff --git a/pkg/provisioner/ironic/prepare_test.go b/pkg/provisioner/ironic/prepare_test.go
index 3633b558..fe3c9468 100644
--- a/pkg/provisioner/ironic/prepare_test.go
+++ b/pkg/provisioner/ironic/prepare_test.go
@@ -34,9 +34,9 @@ func TestPrepare(t *testing.T) {
 				UUID:           nodeUUID,
 			}),
 			unprepared:           true,
-			expectedStarted:      false,
-			expectedRequestAfter: 0,
-			expectedDirty:        false,
+			expectedStarted:      true,
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
 		},
 		{
 			name: "manageable state(have clean steps)",
@@ -50,6 +50,29 @@ func TestPrepare(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
+		{
+			name: "available state(haven't clean steps)",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.Available),
+				UUID:           nodeUUID,
+			}),
+			unprepared:           true,
+			expectedStarted:      true,
+			expectedRequestAfter: 0,
+			expectedDirty:        false,
+		},
+		{
+			name: "available state(have clean steps)",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.Available),
+				UUID:           nodeUUID,
+			}),
+			unprepared:           true,
+			existRaidConfig:      true,
+			expectedStarted:      false,
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
+		},
 		{
 			name: "cleanFail state(cleaned provision settings)",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
@@ -102,6 +125,17 @@ func TestPrepare(t *testing.T) {
 			}),
 			existRaidConfig:      true,
 			expectedStarted:      false,
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
+		},
+		{
+			name: "available state(automated clean finished)",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.Available),
+				UUID:           nodeUUID,
+			}),
+			existRaidConfig:      true,
+			expectedStarted:      false,
 			expectedRequestAfter: 0,
 			expectedDirty:        false,
 		},