Repository: metal3-io/baremetal-operator
Pull request #889 (2021-05-21 14:23:23)

Fix bug with re-inspection due to Node in "available" state
When an annotation appears to force re-inspection, the Host returns to
the Inspecting state from the Ready/Available state. Originally the
ironic node would always be in the "manageable" state for a ready Host,
but we have usually been leaving it in the "available" state after
deprovisioning since 18465130c8f42bda4aa8191b5093656dcc029d90, and after
the initial preparation since 9a55ce5bc63a2f24916f1f604c736570836fdaea.

Handle the "available" node state during inspection by moving the node
to the "manageable" state. Since the first call to InspectHardware may
not start a new inspection, we have to return a new value to indicate
when inspection has actually started and thus it is safe to clear the
annotation. This also fixes a bug where failing to set the boot mode due
to a conflict would cause the inspection annotation to be removed
without starting a new inspection.


Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index ceeff43c..1b633c06 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -667,7 +667,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 	info.log.Info("inspecting hardware")
 
 	refresh := hasInspectAnnotation(info.host)
-	provResult, details, err := prov.InspectHardware(
+	provResult, started, details, err := prov.InspectHardware(
 		provisioner.InspectData{
 			BootMode: info.host.Status.Provisioning.BootMode,
 		},
@@ -682,7 +682,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 	}
 
 	// Delete inspect annotation if exists
-	if hasInspectAnnotation(info.host) {
+	if started && hasInspectAnnotation(info.host) {
 		delete(info.host.Annotations, inspectAnnotationPrefix)
 		if err := r.Update(context.TODO(), info.host); err != nil {
 			return actionError{errors.Wrap(err, "failed to remove inspect annotation from host")}
@@ -1207,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.
@@ -1238,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/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/bmc/irmc.go b/pkg/bmc/irmc.go
index 8cf78939..0e6a2a0b 100644
--- a/pkg/bmc/irmc.go
+++ b/pkg/bmc/irmc.go
@@ -52,9 +52,6 @@ func (a *iRMCAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interfac
 		"irmc_username": bmcCreds.Username,
 		"irmc_password": bmcCreds.Password,
 		"irmc_address":  a.hostname,
-		"ipmi_username": bmcCreds.Username,
-		"ipmi_password": bmcCreds.Password,
-		"ipmi_address":  a.hostname,
 	}
 
 	if a.disableCertificateVerification {
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 783b4c38..9ac61e81 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -116,7 +116,8 @@ func (p *demoProvisioner) ValidateManagementAccess(data provisioner.ManagementAc
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *demoProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *demoProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, started bool, details *metal3v1alpha1.HardwareDetails, err error) {
+	started = true
 	hostName := p.objectMeta.Name
 
 	if hostName == InspectingHost {
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 70d9dbcc..1615fe3b 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -119,12 +119,13 @@ func (p *fixtureProvisioner) ValidateManagementAccess(data provisioner.Managemen
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *fixtureProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *fixtureProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, started bool, details *metal3v1alpha1.HardwareDetails, err error) {
 	// The inspection is ongoing. We'll need to check the fixture
 	// status for the server here until it is ready for us to get the
 	// inspection details. Simulate that for now by creating the
 	// hardware details struct as part of a second pass.
 	p.log.Info("continuing inspection by setting details")
+	started = true
 	details =
 		&metal3v1alpha1.HardwareDetails{
 			RAMMebibytes: 128 * 1024,
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 4bd0aa81..19968a22 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -735,7 +735,7 @@ func (p *ironicProvisioner) changeNodeProvisionState(ironicNode *nodes.Node, opt
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, started bool, details *metal3v1alpha1.HardwareDetails, err error) {
 	p.log.Info("inspecting hardware")
 
 	ironicNode, err := p.getNode()
@@ -748,6 +748,12 @@ func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force,
 	if err != nil || refresh {
 		if _, isNotFound := err.(gophercloud.ErrDefault404); isNotFound || refresh {
 			switch nodes.ProvisionState(ironicNode.ProvisionState) {
+			case nodes.Available:
+				result, err = p.changeNodeProvisionState(
+					ironicNode,
+					nodes.ProvisionStateOpts{Target: nodes.TargetManage},
+				)
+				return
 			case nodes.Inspecting, nodes.InspectWait:
 				p.log.Info("inspection already started")
 				result, err = operationContinuing(introspectionRequeueDelay)
@@ -764,24 +770,23 @@ func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force,
 				}
 				fallthrough
 			default:
-				var success bool
-				success, result, err = p.tryUpdateNode(
+				started, result, err = p.tryUpdateNode(
 					ironicNode,
 					updateOptsBuilder(p.debugLog).
 						SetPropertiesOpts(optionsData{
 							"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
 						}, ironicNode),
 				)
-				if !success {
+				if !started {
 					return
 				}
 
 				p.log.Info("starting new hardware inspection")
-				success, result, err = p.tryChangeNodeProvisionState(
+				started, result, err = p.tryChangeNodeProvisionState(
 					ironicNode,
 					nodes.ProvisionStateOpts{Target: nodes.TargetInspect},
 				)
-				if success {
+				if started {
 					p.publisher("InspectionStarted", "Hardware inspection started")
 				}
 				return
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 05358de0..73625cdb 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -100,7 +100,7 @@ type Provisioner interface {
 	// details of devices discovered on the hardware. It may be called
 	// multiple times, and should return true for its dirty flag until the
 	// inspection is completed.
-	InspectHardware(data InspectData, force, refresh bool) (result Result, details *metal3v1alpha1.HardwareDetails, err error)
+	InspectHardware(data InspectData, force, refresh bool) (result Result, started bool, details *metal3v1alpha1.HardwareDetails, err error)
 
 	// UpdateHardwareState fetches the latest hardware state of the
 	// server and updates the HardwareDetails field of the host with


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 26b62d18..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"
 )
@@ -2008,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 db100767..670f1425 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -920,9 +920,9 @@ func (m *mockProvisioner) ValidateManagementAccess(data provisioner.ManagementAc
 	return m.getNextResultByMethod("ValidateManagementAccess"), "", err
 }
 
-func (m *mockProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (m *mockProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, started bool, details *metal3v1alpha1.HardwareDetails, err error) {
 	details = &metal3v1alpha1.HardwareDetails{}
-	return m.getNextResultByMethod("InspectHardware"), details, err
+	return m.getNextResultByMethod("InspectHardware"), true, details, err
 }
 
 func (m *mockProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) {
diff --git a/pkg/bmc/access_test.go b/pkg/bmc/access_test.go
index 3c15e667..c80b7874 100644
--- a/pkg/bmc/access_test.go
+++ b/pkg/bmc/access_test.go
@@ -763,9 +763,6 @@ func TestDriverInfo(t *testing.T) {
 				"irmc_address":   "192.168.122.1",
 				"irmc_password":  "",
 				"irmc_username":  "",
-				"ipmi_address":   "192.168.122.1",
-				"ipmi_password":  "",
-				"ipmi_username":  "",
 				"irmc_verify_ca": false,
 			},
 		},
@@ -778,9 +775,6 @@ func TestDriverInfo(t *testing.T) {
 				"irmc_port":      "8080",
 				"irmc_password":  "",
 				"irmc_username":  "",
-				"ipmi_address":   "192.168.122.1",
-				"ipmi_password":  "",
-				"ipmi_username":  "",
 				"irmc_verify_ca": false,
 			},
 		},
@@ -792,9 +786,6 @@ func TestDriverInfo(t *testing.T) {
 				"irmc_address":   "fe80::fc33:62ff:fe83:8a76",
 				"irmc_password":  "",
 				"irmc_username":  "",
-				"ipmi_address":   "fe80::fc33:62ff:fe83:8a76",
-				"ipmi_password":  "",
-				"ipmi_username":  "",
 				"irmc_verify_ca": false,
 			},
 		},
@@ -807,9 +798,6 @@ func TestDriverInfo(t *testing.T) {
 				"irmc_port":      "8080",
 				"irmc_password":  "",
 				"irmc_username":  "",
-				"ipmi_address":   "fe80::fc33:62ff:fe83:8a76",
-				"ipmi_password":  "",
-				"ipmi_username":  "",
 				"irmc_verify_ca": false,
 			},
 		},
diff --git a/pkg/provisioner/ironic/inspecthardware_test.go b/pkg/provisioner/ironic/inspecthardware_test.go
index 06372d15..b81cfecd 100644
--- a/pkg/provisioner/ironic/inspecthardware_test.go
+++ b/pkg/provisioner/ironic/inspecthardware_test.go
@@ -28,6 +28,7 @@ func TestInspectHardware(t *testing.T) {
 
 		force bool
 
+		expectedStarted      bool
 		expectedDirty        bool
 		expectedRequestAfter int
 		expectedResultError  string
@@ -36,6 +37,18 @@ func TestInspectHardware(t *testing.T) {
 		expectedPublish string
 		expectedError   string
 	}{
+		{
+			name: "introspection-status-move-from-available",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: "available",
+			}),
+			inspector: testserver.NewInspector(t).Ready().WithIntrospectionFailed(nodeUUID, http.StatusNotFound),
+
+			expectedStarted:      false,
+			expectedDirty:        true,
+			expectedRequestAfter: 10,
+		},
 		{
 			name: "introspection-status-start-new-hardware-inspection",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
@@ -44,6 +57,7 @@ func TestInspectHardware(t *testing.T) {
 			}),
 			inspector: testserver.NewInspector(t).Ready().WithIntrospectionFailed(nodeUUID, http.StatusNotFound),
 
+			expectedStarted:      true,
 			expectedDirty:        true,
 			expectedRequestAfter: 10,
 			expectedPublish:      "InspectionStarted Hardware inspection started",
@@ -156,6 +170,7 @@ func TestInspectHardware(t *testing.T) {
 			inspector: testserver.NewInspector(t).Ready().WithIntrospectionFailed(nodeUUID, http.StatusNotFound),
 			force:     true,
 
+			expectedStarted:      true,
 			expectedDirty:        true,
 			expectedRequestAfter: 10,
 			expectedPublish:      "InspectionStarted Hardware inspection started",
@@ -208,10 +223,11 @@ func TestInspectHardware(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, details, err := prov.InspectHardware(
+			result, started, details, err := prov.InspectHardware(
 				provisioner.InspectData{BootMode: metal3v1alpha1.DefaultBootMode},
 				tc.force, false)
 
+			assert.Equal(t, tc.expectedStarted, started)
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
 			assert.Equal(t, tc.expectedResultError, result.ErrorMessage)