Handle re-registration in all states

apis/metal3.io/v1alpha1/baremetalhost_types.go

@@ -849,27 +849,31 @@ func (host *BareMetalHost) OperationMetricForState(operation ProvisioningState)
 
 // GetImageChecksum returns the hash value and its algo.
 func (host *BareMetalHost) GetImageChecksum() (string, string, bool) {
-	if host.Spec.Image == nil {
-		return "", "", false
-	}
+	return host.Spec.Image.GetChecksum()
+}
 
-	checksum := host.Spec.Image.Checksum
-	checksumType := host.Spec.Image.ChecksumType
+func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
+	if image == nil {
+		return
+	}
 
-	if checksum == "" {
+	if image.Checksum == "" {
 		// Return empty if checksum is not provided
-		return "", "", false
+		return
 	}
-	if checksumType == "" {
-		// If only checksum is specified. Assume type is md5
-		return checksum, string(MD5), true
-	}
-	switch checksumType {
+
+	switch image.ChecksumType {
+	case "":
+		checksumType = string(MD5)
 	case MD5, SHA256, SHA512:
-		return checksum, string(checksumType), true
+		checksumType = string(image.ChecksumType)
 	default:
-		return "", "", false
+		return
 	}
+
+	checksum = image.Checksum
+	ok = true
+	return
 }
 
 // +kubebuilder:object:root=true

controllers/metal3.io/action_result.go

@@ -1,13 +1,15 @@
 package controllers
 
 import (
+	"errors"
 	"math"
 	"math/rand"
 	"time"
 
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 )
 
 const maxBackOffCount = 10
@@ -96,6 +98,10 @@ func (r actionError) Dirty() bool {
 	return false
 }
 
+func (r actionError) NeedsRegistration() bool {
+	return errors.Is(r.err, provisioner.NeedsRegistration)
+}
+
 // actionFailed is a result indicating that the current action has failed,
 // and that the resource should be marked as in error.
 type actionFailed struct {

controllers/metal3.io/baremetalhost_controller.go

@@ -428,15 +428,13 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner
 	// so clear any previous error and record the success in the
 	// status block.
 	info.log.Info("updating credentials success status fields")
+	registeredNewCreds := !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret)
 	info.host.UpdateGoodCredentials(*info.bmcCredsSecret)
 	info.log.Info("clearing previous error message")
 	info.host.ClearError()
 
-	info.publishEvent("BMCAccessValidated", "Verified access to BMC")
-
-	if info.host.Spec.ExternallyProvisioned {
-		info.publishEvent("ExternallyProvisioned",
-			"Registered host that was externally provisioned")
+	if registeredNewCreds {
+		info.publishEvent("BMCAccessValidated", "Verified access to BMC")
 	}
 
 	return actionComplete{}
@@ -679,13 +677,11 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	return steadyStateResult
 }
 
-// A host reaching this action handler should be provisioned or
-// externally provisioned -- a state that it will stay in until the
-// user takes further action. Both of those states mean that it has
-// been registered with the provisioner once, so we use the Adopt()
-// API to ensure that is still true. Then we monitor its power status.
+// A host reaching this action handler should be provisioned or externally
+// provisioned -- a state that it will stay in until the user takes further
+// action. We use the Adopt() API to make sure that the provisioner is aware of
+// the provisioning details. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
-
 	provResult, err := prov.Adopt()
 	if err != nil {
 		return actionError{err}
@@ -702,28 +698,10 @@ func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provi
 }
 
 // A host reaching this action handler should be ready -- a state that
-// it will stay in until the user takes further action. It has been
-// registered with the provisioner once, so we use
-// ValidateManagementAccess() to ensure that is still true. We don't
+// it will stay in until the user takes further action. We don't
 // use Adopt() because we don't want Ironic to treat the host as
 // having been provisioned. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageReady(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
-
-	// We always pass false for credentialsChanged because if they had
-	// changed we would have ended up in actionRegister() instead of
-	// here.
-	provResult, err := prov.ValidateManagementAccess(false)
-	if err != nil {
-		return actionError{err}
-	}
-	if provResult.ErrorMessage != "" {
-		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
-	}
-	if provResult.Dirty {
-		info.host.ClearError()
-		return actionContinue{provResult.RequeueAfter}
-	}
-
 	if info.host.NeedsProvisioning() {
 		// Ensure the provisioning settings we're going to use are stored.
 		dirty, err := saveHostProvisioningSettings(info.host)

controllers/metal3.io/host_state_machine.go

@@ -62,7 +62,7 @@ func recordStateBegin(host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.P
 
 func recordStateEnd(info *reconcileInfo, host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.ProvisioningState, time metav1.Time) {
 	if prevMetric := host.OperationMetricForState(state); prevMetric != nil {
-		if !prevMetric.Start.IsZero() {
+		if !prevMetric.Start.IsZero() && prevMetric.End.IsZero() {
 			prevMetric.End = time
 			info.postSaveCallbacks = append(info.postSaveCallbacks, func() {
 				observer := stateTime[state].With(hostMetricLabels(info.request))
@@ -160,12 +160,6 @@ func (hsm *hostStateMachine) checkInitiateDelete() bool {
 }
 
 func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actionResult) {
-	switch hsm.NextState {
-	case metal3v1alpha1.StateNone, metal3v1alpha1.StateUnmanaged:
-		// We haven't yet reached the Registration state, so don't attempt
-		// to register the Host.
-		return
-	}
 	if !hsm.Host.DeletionTimestamp.IsZero() {
 		// BUG(zaneb) We currently don't attempt to re-register the Host
 		// if we find it missing once a delete has been requested (in
@@ -177,23 +171,38 @@ func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actio
 		return
 	}
 
-	if hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret) {
-		// Credentials are unchanged since we verified them.
-		return
-	}
+	needsReregister := false
 
-	recordStateBegin(hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
-	if hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError {
-		if hsm.Host.Status.TriedCredentials.Match(*info.bmcCredsSecret) {
-			// Already tried with these credentials; no point retrying
-			info.log.Info("Unmodified credentials; not retrying")
-			return actionFailed{ErrorType: metal3v1alpha1.RegistrationError}
+	switch hsm.NextState {
+	case metal3v1alpha1.StateNone, metal3v1alpha1.StateUnmanaged:
+		// We haven't yet reached the Registration state, so don't attempt
+		// to register the Host.
+		return
+	case metal3v1alpha1.StateRegistering:
+	default:
+		needsReregister = (hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError ||
+			!hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret))
+		if needsReregister {
+			info.log.Info("Retrying registration")
+			recordStateBegin(hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
 		}
-		info.log.Info("Modified credentials detected; will retry registration")
 	}
+
 	result = hsm.Reconciler.actionRegistering(hsm.Provisioner, info)
-	if _, complete := result.(actionComplete); complete && hsm.Host.Status.Provisioning.State != metal3v1alpha1.StateRegistering {
-		recordStateEnd(info, hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
+	if _, complete := result.(actionComplete); complete {
+		if hsm.NextState != metal3v1alpha1.StateRegistering {
+			recordStateEnd(info, hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
+		}
+		if needsReregister {
+			// Host was re-registered, so requeue and run the state machine on
+			// the next reconcile
+			result = actionContinue{}
+		} else {
+			// Allow the state machine to run, either because we were just
+			// reconfirming an existing registration, or because we are in the
+			// Registering state
+			result = nil
+		}
 	}
 	return
 }
@@ -224,7 +233,6 @@ func (hsm *hostStateMachine) handleRegistering(info *reconcileInfo) actionResult
 	// registered using the current BMC credentials, so we can move to the
 	// next state. We will not return to the Registering state, even
 	// if the credentials change and the Host must be re-registered.
-	hsm.Host.ClearError()
 	if hsm.Host.Spec.ExternallyProvisioned {
 		hsm.NextState = metal3v1alpha1.StateExternallyProvisioned
 	} else {
@@ -323,15 +331,15 @@ func (hsm *hostStateMachine) handleProvisioned(info *reconcileInfo) actionResult
 func (hsm *hostStateMachine) handleDeprovisioning(info *reconcileInfo) actionResult {
 	actResult := hsm.Reconciler.actionDeprovisioning(hsm.Provisioner, info)
 
-	switch actResult.(type) {
-	case actionComplete:
-		if !hsm.Host.DeletionTimestamp.IsZero() {
-			hsm.NextState = metal3v1alpha1.StateDeleting
-		} else {
+	if hsm.Host.DeletionTimestamp.IsZero() {
+		if _, complete := actResult.(actionComplete); complete {
 			hsm.NextState = metal3v1alpha1.StateReady
 		}
-	case actionFailed:
-		if !hsm.Host.DeletionTimestamp.IsZero() {
+	} else {
+		switch actResult.(type) {
+		case actionComplete:
+			hsm.NextState = metal3v1alpha1.StateDeleting
+		case actionFailed:
 			// If the provisioner gives up deprovisioning and
 			// deletion has been requested, continue to delete.
 			// Note that this is entirely theoretical, as the

pkg/provisioner/ironic/adopt_test.go

@@ -61,8 +61,8 @@ func TestAdopt(t *testing.T) {
 				UUID:           nodeUUID,
 			}),
 
-			expectedDirty:        true,
-			expectedRequestAfter: 10,
+			expectedDirty: false,
+			expectedError: true,
 		},
 		{
 			name: "node-in-AdoptFail",

pkg/provisioner/ironic/ironic.go

@@ -560,7 +560,7 @@ func (p *ironicProvisioner) InspectHardware() (result provisioner.Result, detail
 		return
 	}
 	if ironicNode == nil {
-		return result, nil, fmt.Errorf("no ironic node for host")
+		return result, nil, provisioner.NeedsRegistration
 	}
 
 	status, err := introspection.GetIntrospectionStatus(p.inspector, ironicNode.UUID).Extract()
@@ -650,7 +650,7 @@ func (p *ironicProvisioner) UpdateHardwareState() (result provisioner.Result, er
 		return result, errors.Wrap(err, "failed to find existing host")
 	}
 	if ironicNode == nil {
-		return result, fmt.Errorf("no ironic node for host")
+		return result, provisioner.NeedsRegistration
 	}
 
 	var discoveredVal bool
@@ -1000,18 +1000,12 @@ func (p *ironicProvisioner) Adopt() (result provisioner.Result, err error) {
 		return
 	}
 	if ironicNode == nil {
-		// The node does not exist, but we were called so the
-		// controller thinks that the node existed at one time. That
-		// likely means data loss from restarting the database, so
-		// pass through the validation process to register the node
-		// again. Pass true to indicate that we need to re-test the
-		// credentials, just in case.
-		p.log.Info("re-registering host")
-		return p.ValidateManagementAccess(true)
+		err = provisioner.NeedsRegistration
+		return
 	}
 
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
-	case nodes.Enroll:
+	case nodes.Enroll, nodes.Verifying:
 		err = fmt.Errorf("Invalid state for adopt: %s",
 			ironicNode.ProvisionState)
 	case nodes.Manageable:
@@ -1021,7 +1015,7 @@ func (p *ironicProvisioner) Adopt() (result provisioner.Result, err error) {
 				Target: nodes.TargetAdopt,
 			},
 		)
-	case nodes.Adopting, nodes.Verifying:
+	case nodes.Adopting:
 		result.RequeueAfter = provisionRequeueDelay
 		result.Dirty = true
 	case nodes.AdoptFail:
@@ -1043,7 +1037,7 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu
 		return result, errors.Wrap(err, "could not find host to receive image")
 	}
 	if ironicNode == nil {
-		return result, fmt.Errorf("no ironic node for host")
+		return result, provisioner.NeedsRegistration
 	}
 
 	p.log.Info("provisioning image to host", "state", ironicNode.ProvisionState)

pkg/provisioner/ironic/updatehardwarestate_test.go

@@ -101,7 +101,7 @@ func TestUpdateHardwareState(t *testing.T) {
 			name:   "not-ironic-node",
 			ironic: testserver.NewIronic(t).Ready().NoNode(nodeUUID).NoNode("myhost"),
 
-			expectedError: "no ironic node for host",
+			expectedError: "Host not registered",
 		},
 	}
 

pkg/provisioner/provisioner.go

@@ -1,6 +1,7 @@
 package provisioner
 
 import (
+	"errors"
 	"time"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
@@ -100,3 +101,5 @@ type Result struct {
 	// Any error message produced by the provisioner.
 	ErrorMessage string
 }
+
+var NeedsRegistration = errors.New("Host not registered")