Improve communications between controller and provisioner

controllers/metal3.io/action_result.go

@@ -25,17 +25,6 @@ type actionResult interface {
 	Dirty() bool
 }
 
-// actionContinueNoWrite is a result indicating that the current action is still
-// in progress, and that the resource should remain in the same provisioning
-// state without writing the status
-type actionContinueNoWrite struct {
-	actionContinue
-}
-
-func (r actionContinueNoWrite) Dirty() bool {
-	return false
-}
-
 // actionContinue is a result indicating that the current action is still
 // in progress, and that the resource should remain in the same provisioning
 // state.
@@ -51,6 +40,17 @@ func (r actionContinue) Result() (result reconcile.Result, err error) {
 }
 
 func (r actionContinue) Dirty() bool {
+	return false
+}
+
+// actionUpdate is a result indicating that the current action is still
+// in progress, and that the resource should remain in the same provisioning
+// state but write new Status data.
+type actionUpdate struct {
+	actionContinue
+}
+
+func (r actionUpdate) Dirty() bool {
 	return true
 }
 

controllers/metal3.io/baremetalhost_controller.go

@@ -208,7 +208,7 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 		request:        request,
 		bmcCredsSecret: bmcCredsSecret,
 	}
-	prov, err := r.ProvisionerFactory(host, *bmcCreds, info.publishEvent)
+	prov, err := r.ProvisionerFactory(*host, *bmcCreds, info.publishEvent)
 	if err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to create provisioner")
 	}
@@ -419,53 +419,71 @@ func (r *BareMetalHostReconciler) actionUnmanaged(prov provisioner.Provisioner,
 	if info.host.HasBMCDetails() {
 		return actionComplete{}
 	}
-	return actionContinueNoWrite{actionContinue{unmanagedRetryDelay}}
+	return actionContinue{unmanagedRetryDelay}
 }
 
 // Test the credentials by connecting to the management controller.
-func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
+func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("registering and validating access to management controller",
 		"credentials", info.host.Status.TriedCredentials)
+	dirty := false
 
 	credsChanged := !info.host.Status.TriedCredentials.Match(*info.bmcCredsSecret)
 	if credsChanged {
 		info.log.Info("new credentials")
 		info.host.UpdateTriedCredentials(*info.bmcCredsSecret)
 		info.postSaveCallbacks = append(info.postSaveCallbacks, updatedCredentials.Inc)
+		dirty = true
 	}
 
-	provResult, err := prov.ValidateManagementAccess(credsChanged, info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
+	provResult, provID, err := prov.ValidateManagementAccess(credsChanged, info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
 	if err != nil {
 		noManagementAccess.Inc()
 		return actionError{errors.Wrap(err, "failed to validate BMC access")}
 	}
 
-	info.log.Info("response from validate", "provResult", provResult)
-
 	if provResult.ErrorMessage != "" {
 		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
 	}
 
+	if provID != "" && info.host.Status.Provisioning.ID != provID {
+		info.log.Info("setting provisioning id", "ID", provID)
+		info.host.Status.Provisioning.ID = provID
+		dirty = true
+	}
+
 	if provResult.Dirty {
 		info.log.Info("host not ready", "wait", provResult.RequeueAfter)
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			dirty = true
+		}
+		if dirty {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	// Reaching this point means the credentials are valid and worked,
 	// 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")
-	clearError(info.host)
-
-	if registeredNewCreds {
+	if !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret) {
+		info.log.Info("updating credentials success status fields")
+		info.host.UpdateGoodCredentials(*info.bmcCredsSecret)
 		info.publishEvent("BMCAccessValidated", "Verified access to BMC")
+		dirty = true
+	} else {
+		info.log.Info("verified access to the BMC")
+	}
+	if clearError(info.host) {
+		info.log.Info("clearing previous error message")
+		dirty = true
 	}
 
-	return actionComplete{}
+	if dirty {
+		return actionComplete{}
+	}
+	return nil
 }
 
 // Ensure we have the information about the hardware on the host.
@@ -481,12 +499,15 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 		return recordActionFailure(info, metal3v1alpha1.InspectionError, provResult.ErrorMessage)
 	}
 
-	clearError(info.host)
-
 	if provResult.Dirty || details == nil {
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
+	clearError(info.host)
 	info.host.Status.HardwareDetails = details
 	return actionComplete{}
 }
@@ -548,7 +569,7 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione
 		if err := r.Update(context.TODO(), info.host); err != nil {
 			return actionError{errors.Wrap(err, "failed to remove reboot annotations from host")}
 		}
-		return actionContinueNoWrite{}
+		return actionContinue{}
 	}
 
 	provResult, err := prov.Provision(hostConf)
@@ -565,8 +586,11 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione
 		// Go back into the queue and wait for the Provision() method
 		// to return false, indicating that it has no more work to
 		// do.
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	// If the provisioner had no work, ensure the image settings match.
@@ -598,8 +622,11 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
 	}
 	if provResult.Dirty {
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	info.log.Info("deprovisioning")
@@ -614,15 +641,18 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 	}
 
 	if provResult.Dirty {
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	if clearRebootAnnotations(info.host) {
 		if err = r.Update(context.TODO(), info.host); err != nil {
 			return actionError{errors.Wrap(err, "failed to remove reboot annotations from host")}
 		}
-		return actionContinueNoWrite{}
+		return actionContinue{}
 	}
 
 	// After the provisioner is done, clear the provisioning settings
@@ -638,18 +668,16 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	var provResult provisioner.Result
 
 	// Check the current status and save it before trying to update it.
-	provResult, err := prov.UpdateHardwareState()
+	hwState, err := prov.UpdateHardwareState()
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to update the host power status")}
 	}
 
-	if provResult.ErrorMessage != "" {
-		return recordActionFailure(info, metal3v1alpha1.PowerManagementError, provResult.ErrorMessage)
-	}
-
-	if provResult.Dirty {
+	if hwState.PoweredOn != nil && *hwState.PoweredOn != info.host.Status.PoweredOn {
+		info.log.Info("updating power status", "discovered", *hwState.PoweredOn)
+		info.host.Status.PoweredOn = *hwState.PoweredOn
 		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		return actionUpdate{}
 	}
 
 	desiredPowerOnState := info.host.Spec.Online
@@ -662,7 +690,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 				return actionError{errors.Wrap(err, "failed to remove reboot annotation from host")}
 			}
 
-			return actionContinueNoWrite{}
+			return actionContinue{}
 		}
 	}
 
@@ -677,7 +705,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	// a delay.
 	steadyStateResult := actionContinue{time.Second * 60}
 	if info.host.Status.PoweredOn == desiredPowerOnState {
-		return actionContinueNoWrite{steadyStateResult}
+		return steadyStateResult
 	}
 
 	info.log.Info("power state change needed",
@@ -708,16 +736,19 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 			}
 			powerChangeAttempts.With(metricLabels).Inc()
 		})
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	// The provisioner did not have to do anything to change the power
 	// state and there were no errors, so reflect the new state in the
 	// host status field.
 	info.host.Status.PoweredOn = info.host.Spec.Online
 	info.host.Status.ErrorCount = 0
-	return steadyStateResult
+	return actionUpdate{steadyStateResult}
 }
 
 // A host reaching this action handler should be provisioned or externally
@@ -733,8 +764,11 @@ func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provi
 		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
 	}
 	if provResult.Dirty {
-		clearError(info.host)
-		return actionContinue{provResult.RequeueAfter}
+		result := actionContinue{provResult.RequeueAfter}
+		if clearError(info.host) {
+			return actionUpdate{result}
+		}
+		return result
 	}
 
 	return r.manageHostPower(prov, info)

controllers/metal3.io/baremetalhost_controller_test.go

@@ -23,8 +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"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/metal3-io/baremetal-operator/pkg/utils"
 )
@@ -88,7 +86,7 @@ func newDefaultHost(t *testing.T) *metal3v1alpha1.BareMetalHost {
 	return newDefaultNamedHost(t.Name(), t)
 }
 
-func newTestReconcilerWithProvisionerFactory(factory provisioner.Factory, initObjs ...runtime.Object) *BareMetalHostReconciler {
+func newTestReconcilerWithFixture(fix *fixture.Fixture, initObjs ...runtime.Object) *BareMetalHostReconciler {
 
 	c := fakeclient.NewFakeClient(initObjs...)
 
@@ -99,13 +97,14 @@ func newTestReconcilerWithProvisionerFactory(factory provisioner.Factory, initOb
 	return &BareMetalHostReconciler{
 		Client:             c,
 		Scheme:             scheme.Scheme,
-		ProvisionerFactory: factory,
+		ProvisionerFactory: fix.New,
 		Log:                ctrl.Log.WithName("controllers").WithName("BareMetalHost"),
 	}
 }
 
 func newTestReconciler(initObjs ...runtime.Object) *BareMetalHostReconciler {
-	return newTestReconcilerWithProvisionerFactory(fixture.New, initObjs...)
+	fix := fixture.Fixture{}
+	return newTestReconcilerWithFixture(&fix, initObjs...)
 }
 
 type DoneFunc func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool
@@ -1033,12 +1032,12 @@ func TestDeleteHost(t *testing.T) {
 			host.DeletionTimestamp = &now
 			host.Status.Provisioning.ID = "made-up-id"
 			badSecret := newBMCCredsSecret("bmc-creds-no-user", "", "Pass")
-			r := newTestReconciler(host, badSecret)
+			fix := fixture.Fixture{}
+			r := newTestReconcilerWithFixture(&fix, host, badSecret)
 
 			tryReconcile(t, r, host,
 				func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
-					t.Logf("provisioning id: %q", host.Status.Provisioning.ID)
-					return host.Status.Provisioning.ID == ""
+					return fix.Deleted
 				},
 			)
 		})
@@ -1156,22 +1155,12 @@ func TestUpdateRootDeviceHints(t *testing.T) {
 func TestProvisionerIsReady(t *testing.T) {
 	host := newDefaultHost(t)
 
-	var prov provisioner.Provisioner
-	r := newTestReconcilerWithProvisionerFactory(func(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner provisioner.Provisioner, err error) {
-		if prov == nil {
-			prov, err = fixture.NewMock(host, bmcCreds, publisher, 5)
-		}
-		return prov, err
-	}, host)
+	fix := fixture.Fixture{BecomeReadyCounter: 5}
+	r := newTestReconcilerWithFixture(&fix, host)
 
 	tryReconcile(t, r, host,
 		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
-			if prov == nil {
-				return false
-			}
-
-			ready, _ := prov.IsReady()
-			return ready
+			return host.Status.Provisioning.State != metal3v1alpha1.StateNone
 		},
 	)
 }