diff --git a/controllers/metal3.io/action_result.go b/controllers/metal3.io/action_result.go index 5f3a9717e3..a746f05c98 100644 --- a/controllers/metal3.io/action_result.go +++ b/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 } diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index b15a9ba924..502116e22a 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/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,8 +736,11 @@ 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 @@ -717,7 +748,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner, // 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) diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go index 3c02a2b966..e00d8e7dc4 100644 --- a/controllers/metal3.io/baremetalhost_controller_test.go +++ b/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 }, ) } diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go index 449782cebe..45ba5bb096 100644 --- a/controllers/metal3.io/host_state_machine.go +++ b/controllers/metal3.io/host_state_machine.go @@ -63,7 +63,7 @@ func recordStateBegin(host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.P } } -func recordStateEnd(info *reconcileInfo, host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.ProvisioningState, time metav1.Time) { +func recordStateEnd(info *reconcileInfo, host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.ProvisioningState, time metav1.Time) (changed bool) { if prevMetric := host.OperationMetricForState(state); prevMetric != nil { if !prevMetric.Start.IsZero() && prevMetric.End.IsZero() { prevMetric.End = time @@ -71,8 +71,10 @@ func recordStateEnd(info *reconcileInfo, host *metal3v1alpha1.BareMetalHost, sta observer := stateTime[state].With(hostMetricLabels(info.request)) observer.Observe(prevMetric.Duration().Seconds()) }) + changed = true } } + return } func (hsm *hostStateMachine) updateHostStateFrom(initialState metal3v1alpha1.ProvisioningState, @@ -170,8 +172,6 @@ func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actio return } - needsReregister := false - switch hsm.NextState { case metal3v1alpha1.StateNone, metal3v1alpha1.StateUnmanaged: // We haven't yet reached the Registration state, so don't attempt @@ -182,30 +182,24 @@ func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actio return case metal3v1alpha1.StateRegistering: default: - needsReregister = (hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError || - !hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret)) - if needsReregister { + if hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError || + !hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret) { info.log.Info("Retrying registration") recordStateBegin(hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now()) } } - result = hsm.Reconciler.actionRegistering(hsm.Provisioner, info) - 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 + result = hsm.Reconciler.registerHost(hsm.Provisioner, info) + _, complete := result.(actionComplete) + if (result == nil || complete) && + hsm.NextState != metal3v1alpha1.StateRegistering { + if recordStateEnd(info, hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now()) { + result = actionUpdate{} } } + if complete { + result = actionUpdate{} + } return } diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go index 96834305f9..51c1a97123 100644 --- a/controllers/metal3.io/host_state_machine_test.go +++ b/controllers/metal3.io/host_state_machine_test.go @@ -18,7 +18,7 @@ import ( func testStateMachine(host *metal3v1alpha1.BareMetalHost) *hostStateMachine { r := newTestReconciler() - p, _ := r.ProvisionerFactory(host, bmc.Credentials{}, + p, _ := r.ProvisionerFactory(*host.DeepCopy(), bmc.Credentials{}, func(reason, message string) {}) return newHostStateMachine(host, r, p, true) } @@ -366,8 +366,8 @@ func (m *mockProvisioner) setNextResult(dirty bool) { } } -func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) { - return m.nextResult, err +func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) { + return m.nextResult, "", err } func (m *mockProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) { @@ -375,8 +375,8 @@ func (m *mockProvisioner) InspectHardware(force bool) (result provisioner.Result return m.nextResult, details, err } -func (m *mockProvisioner) UpdateHardwareState() (result provisioner.Result, err error) { - return m.nextResult, err +func (m *mockProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) { + return } func (m *mockProvisioner) Adopt(force bool) (result provisioner.Result, err error) { diff --git a/main.go b/main.go index 1a8d477d58..e528383805 100644 --- a/main.go +++ b/main.go @@ -118,21 +118,24 @@ func main() { os.Exit(1) } - provisionerFactory := func(host *metal3iov1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish provisioner.EventPublisher) (provisioner.Provisioner, error) { + provisionerFactory := func(host metal3iov1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish provisioner.EventPublisher) (provisioner.Provisioner, error) { isUnmanaged := host.Spec.ExternallyProvisioned && !host.HasBMCDetails() + hostCopy := host.DeepCopy() + if runInTestMode { ctrl.Log.Info("using test provisioner") - return fixture.New(host, bmcCreds, publish) + fix := fixture.Fixture{} + return fix.New(*hostCopy, bmcCreds, publish) } else if runInDemoMode { ctrl.Log.Info("using demo provisioner") - return demo.New(host, bmcCreds, publish) + return demo.New(*hostCopy, bmcCreds, publish) } else if isUnmanaged { ctrl.Log.Info("using empty provisioner") - return empty.New(host, bmcCreds, publish) + return empty.New(*hostCopy, bmcCreds, publish) } ironic.LogStartup() - return ironic.New(host, bmcCreds, publish) + return ironic.New(*hostCopy, bmcCreds, publish) } if err = (&metal3iocontroller.BareMetalHostReconciler{ diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go index 9fc1bc8168..606bb99aae 100644 --- a/pkg/provisioner/demo/demo.go +++ b/pkg/provisioner/demo/demo.go @@ -46,7 +46,7 @@ const ( // and uses Ironic to manage the host. type demoProvisioner struct { // the host to be managed by this provisioner - host *metal3v1alpha1.BareMetalHost + host metal3v1alpha1.BareMetalHost // the bmc credentials bmcCreds bmc.Credentials // a logger configured for this host @@ -56,7 +56,7 @@ type demoProvisioner struct { } // New returns a new Ironic Provisioner -func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { +func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { p := &demoProvisioner{ host: host, bmcCreds: bmcCreds, @@ -68,7 +68,7 @@ func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher // ValidateManagementAccess tests the connection information for the // host to verify that the location and credentials work. -func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) { +func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) { p.log.Info("testing management access") hostName := p.host.ObjectMeta.Name @@ -88,14 +88,14 @@ func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged, force boo default: if p.host.Status.Provisioning.ID == "" { - p.host.Status.Provisioning.ID = p.host.ObjectMeta.Name + provID = p.host.ObjectMeta.Name p.log.Info("setting provisioning id", "provisioningID", p.host.Status.Provisioning.ID) result.Dirty = true } } - return result, nil + return } // InspectHardware updates the HardwareDetails field of the host with @@ -174,12 +174,10 @@ func (p *demoProvisioner) InspectHardware(force bool) (result provisioner.Result // UpdateHardwareState fetches the latest hardware state of the server // and updates the HardwareDetails field of the host with details. It // is expected to do this in the least expensive way possible, such as -// reading from a cache, and return dirty only if any state -// information has changed. -func (p *demoProvisioner) UpdateHardwareState() (result provisioner.Result, err error) { +// reading from a cache. +func (p *demoProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) { p.log.Info("updating hardware state") - result.Dirty = false - return result, nil + return } // Adopt allows an externally-provisioned server to be adopted. diff --git a/pkg/provisioner/empty/empty.go b/pkg/provisioner/empty/empty.go index a50af7fc82..0fbc48802c 100644 --- a/pkg/provisioner/empty/empty.go +++ b/pkg/provisioner/empty/empty.go @@ -15,14 +15,14 @@ type emptyProvisioner struct { } // New returns a new Empty Provisioner -func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { +func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { return &emptyProvisioner{}, nil } // ValidateManagementAccess tests the connection information for the // host to verify that the location and credentials work. -func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (provisioner.Result, error) { - return provisioner.Result{}, nil +func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (provisioner.Result, string, error) { + return provisioner.Result{}, "", nil } // InspectHardware updates the HardwareDetails field of the host with @@ -36,10 +36,9 @@ func (p *emptyProvisioner) InspectHardware(force bool) (provisioner.Result, *met // UpdateHardwareState fetches the latest hardware state of the server // and updates the HardwareDetails field of the host with details. It // is expected to do this in the least expensive way possible, such as -// reading from a cache, and return dirty only if any state -// information has changed. -func (p *emptyProvisioner) UpdateHardwareState() (provisioner.Result, error) { - return provisioner.Result{}, nil +// reading from a cache. +func (p *emptyProvisioner) UpdateHardwareState() (provisioner.HardwareState, error) { + return provisioner.HardwareState{}, nil } // Adopt allows an externally-provisioned server to be adopted. diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go index dcb365b30d..136264383e 100644 --- a/pkg/provisioner/fixture/fixture.go +++ b/pkg/provisioner/fixture/fixture.go @@ -45,53 +45,57 @@ func (cd *fixtureHostConfigData) MetaData() (string, error) { // and uses Ironic to manage the host. type fixtureProvisioner struct { // the host to be managed by this provisioner - host *metal3v1alpha1.BareMetalHost + host metal3v1alpha1.BareMetalHost // the bmc credentials bmcCreds bmc.Credentials // a logger configured for this host log logr.Logger // an event publisher for recording significant events publisher provisioner.EventPublisher + // state storage for the Host + state *Fixture +} + +type Fixture struct { + // counter to set the provisioner as ready + BecomeReadyCounter int + // state to manage deletion + Deleted bool // state to manage the two-step adopt process adopted bool - // counter to set the provisioner as ready - becomeReadyCounter int + // state to manage provisioning + image metal3v1alpha1.Image + // state to manage power + poweredOn bool } // New returns a new Ironic FixtureProvisioner -func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { - return NewMock(host, bmcCreds, publisher, 0) -} - -// NewMock is used in tests to build a fixture provisioner and inject additional test parameters -func NewMock(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, becomeReadyCounter int) (provisioner.Provisioner, error) { +func (f *Fixture) New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { p := &fixtureProvisioner{ - host: host, - bmcCreds: bmcCreds, - log: log.WithValues("host", host.Name), - publisher: publisher, - becomeReadyCounter: becomeReadyCounter, + host: *host.DeepCopy(), + bmcCreds: bmcCreds, + log: log.WithValues("host", host.Name), + publisher: publisher, + state: f, } return p, nil } // ValidateManagementAccess tests the connection information for the // host to verify that the location and credentials work. -func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) { +func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) { p.log.Info("testing management access") // Fill in the ID of the host in the provisioning system if p.host.Status.Provisioning.ID == "" { - p.host.Status.Provisioning.ID = "temporary-fake-id" - p.log.Info("setting provisioning id", - "provisioningID", p.host.Status.Provisioning.ID) + provID = "temporary-fake-id" result.Dirty = true result.RequeueAfter = time.Second * 5 p.publisher("Registered", "Registered new host") - return result, nil + return } - return result, nil + return } // InspectHardware updates the HardwareDetails field of the host with @@ -159,21 +163,20 @@ func (p *fixtureProvisioner) InspectHardware(force bool) (result provisioner.Res // UpdateHardwareState fetches the latest hardware state of the server // and updates the HardwareDetails field of the host with details. It // is expected to do this in the least expensive way possible, such as -// reading from a cache, and return dirty only if any state -// information has changed. -func (p *fixtureProvisioner) UpdateHardwareState() (result provisioner.Result, err error) { +// reading from a cache. +func (p *fixtureProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) { if !p.host.NeedsProvisioning() { + hwState.PoweredOn = &p.state.poweredOn p.log.Info("updating hardware state") - result.Dirty = false } - return result, nil + return } // Adopt allows an externally-provisioned server to be adopted. func (p *fixtureProvisioner) Adopt(force bool) (result provisioner.Result, err error) { p.log.Info("adopting host") - if p.host.Spec.ExternallyProvisioned && !p.adopted { - p.adopted = true + if p.host.Spec.ExternallyProvisioned && !p.state.adopted { + p.state.adopted = true result.Dirty = true result.RequeueAfter = provisionRequeueDelay } @@ -187,10 +190,10 @@ func (p *fixtureProvisioner) Provision(hostConf provisioner.HostConfigData) (res p.log.Info("provisioning image to host", "state", p.host.Status.Provisioning.State) - if p.host.Status.Provisioning.Image.URL == "" { + if p.state.image.URL == "" { p.publisher("ProvisioningComplete", "Image provisioning completed") p.log.Info("moving to done") - p.host.Status.Provisioning.Image = *p.host.Spec.Image + p.state.image = *p.host.Spec.Image result.Dirty = true result.RequeueAfter = provisionRequeueDelay } @@ -211,10 +214,10 @@ func (p *fixtureProvisioner) Deprovision(force bool) (result provisioner.Result, // necessary once we really have Fixture doing the deprovisioning // and we can monitor it's status. - if p.host.Status.HardwareDetails != nil { + if p.state.image.URL != "" { p.publisher("DeprovisionStarted", "Image deprovisioning started") p.log.Info("clearing hardware details") - p.host.Status.HardwareDetails = nil + p.state.image = metal3v1alpha1.Image{} result.Dirty = true return result, nil } @@ -229,9 +232,9 @@ func (p *fixtureProvisioner) Deprovision(force bool) (result provisioner.Result, func (p *fixtureProvisioner) Delete() (result provisioner.Result, err error) { p.log.Info("deleting host") - if p.host.Status.Provisioning.ID != "" { + if !p.state.Deleted { p.log.Info("clearing provisioning id") - p.host.Status.Provisioning.ID = "" + p.state.Deleted = true result.Dirty = true return result, nil } @@ -244,10 +247,10 @@ func (p *fixtureProvisioner) Delete() (result provisioner.Result, err error) { func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) { p.log.Info("ensuring host is powered on") - if !p.host.Status.PoweredOn { + if !p.state.poweredOn { p.publisher("PowerOn", "Host powered on") p.log.Info("changing status") - p.host.Status.PoweredOn = true + p.state.poweredOn = true result.Dirty = true return result, nil } @@ -260,10 +263,10 @@ func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) { func (p *fixtureProvisioner) PowerOff() (result provisioner.Result, err error) { p.log.Info("ensuring host is powered off") - if p.host.Status.PoweredOn { + if p.state.poweredOn { p.publisher("PowerOff", "Host powered off") p.log.Info("changing status") - p.host.Status.PoweredOn = false + p.state.poweredOn = false result.Dirty = true return result, nil } @@ -275,9 +278,9 @@ func (p *fixtureProvisioner) PowerOff() (result provisioner.Result, err error) { func (p *fixtureProvisioner) IsReady() (result bool, err error) { p.log.Info("checking provisioner status") - if p.becomeReadyCounter > 0 { - p.becomeReadyCounter-- + if p.state.BecomeReadyCounter > 0 { + p.state.BecomeReadyCounter-- } - return p.becomeReadyCounter == 0, nil + return p.state.BecomeReadyCounter == 0, nil } diff --git a/pkg/provisioner/ironic/delete_test.go b/pkg/provisioner/ironic/delete_test.go index 16b8e43889..00a1eb9a9f 100644 --- a/pkg/provisioner/ironic/delete_test.go +++ b/pkg/provisioner/ironic/delete_test.go @@ -50,7 +50,7 @@ func TestDelete(t *testing.T) { }, ).DeleteError(nodeUUID, http.StatusConflict), expectedDirty: true, - expectedRequestAfter: 0, + expectedRequestAfter: provisionRequeueDelay, }, { name: "delete-host-not-found", @@ -122,7 +122,7 @@ func TestDelete(t *testing.T) { ).NodeUpdateError(nodeUUID, http.StatusConflict), expectedDirty: true, - expectedRequestAfter: 0, + expectedRequestAfter: provisionRequeueDelay, }, { name: "not-in-maintenance-update", diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index efca3c4ba5..8dc7c6fe44 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -105,7 +105,7 @@ func init() { // and uses Ironic to manage the host. type ironicProvisioner struct { // the host to be managed by this provisioner - host *metal3v1alpha1.BareMetalHost + host metal3v1alpha1.BareMetalHost // a shorter path to the provisioning status data structure status *metal3v1alpha1.ProvisionStatus // access parameters for the BMC @@ -137,7 +137,7 @@ func LogStartup() { // A private function to construct an ironicProvisioner (rather than a // Provisioner interface) in a consistent way for tests. -func newProvisionerWithSettings(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, ironicURL string, ironicAuthSettings clients.AuthConfig, inspectorURL string, inspectorAuthSettings clients.AuthConfig) (*ironicProvisioner, error) { +func newProvisionerWithSettings(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, ironicURL string, ironicAuthSettings clients.AuthConfig, inspectorURL string, inspectorAuthSettings clients.AuthConfig) (*ironicProvisioner, error) { tlsConf := clients.TLSConfig{ TrustedCAFile: ironicTrustedCAFile, InsecureSkipVerify: ironicInsecure, @@ -156,7 +156,7 @@ func newProvisionerWithSettings(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc clientIronic, clientInspector) } -func newProvisionerWithIronicClients(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, clientIronic *gophercloud.ServiceClient, clientInspector *gophercloud.ServiceClient) (*ironicProvisioner, error) { +func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, clientIronic *gophercloud.ServiceClient, clientInspector *gophercloud.ServiceClient) (*ironicProvisioner, error) { bmcAccess, err := bmc.NewAccessDetails(host.Spec.BMC.Address, host.Spec.BMC.DisableCertificateVerification) if err != nil { @@ -182,7 +182,7 @@ func newProvisionerWithIronicClients(host *metal3v1alpha1.BareMetalHost, bmcCred // New returns a new Ironic Provisioner using the global configuration // for finding the Ironic services. -func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { +func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { var err error if clientIronicSingleton == nil || clientInspectorSingleton == nil { tlsConf := clients.TLSConfig{ @@ -333,14 +333,15 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro // // FIXME(dhellmann): We should rename this method to describe what it // actually does. -func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) { +func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) { var ironicNode *nodes.Node p.log.Info("validating management access") ironicNode, err = p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + result, err = transientError(errors.Wrap(err, "failed to find existing host")) + return } // Some BMC types require a MAC address, so ensure we have one @@ -348,8 +349,8 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b if p.bmcAccess.NeedsMAC() && p.host.Spec.BootMACAddress == "" { msg := fmt.Sprintf("BMC driver %s requires a BootMACAddress value", p.bmcAccess.Type()) p.log.Info(msg) - result.ErrorMessage = msg - return result, nil + result, err = operationFailed(msg) + return } driverInfo := p.bmcAccess.DriverInfo(p.bmcCreds) @@ -358,6 +359,8 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b driverInfo["deploy_kernel"] = deployKernelURL driverInfo["deploy_ramdisk"] = deployRamdiskURL + result, err = operationComplete() + // If we have not found a node yet, we need to create one if ironicNode == nil { p.log.Info("registering host in ironic") @@ -380,15 +383,14 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b }).Extract() // FIXME(dhellmann): Handle 409 and 503? errors here. if err != nil { - return result, errors.Wrap(err, "failed to register host in ironic") + result, err = transientError(errors.Wrap(err, "failed to register host in ironic")) + return } p.publisher("Registered", "Registered new host") // Store the ID so other methods can assume it is set and so // we can find the node again later. - p.status.ID = ironicNode.UUID - result.Dirty = true - p.log.Info("setting provisioning id", "ID", p.status.ID) + provID = ironicNode.UUID // If we know the MAC, create a port. Otherwise we will have // to do this after we run the introspection step. @@ -396,7 +398,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b enable := true p.log.Info("creating port for node in ironic", "MAC", p.host.Spec.BootMACAddress) - _, err := ports.Create( + _, err = ports.Create( p.client, ports.CreateOpts{ NodeUUID: ironicNode.UUID, @@ -404,7 +406,8 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b PXEEnabled: &enable, }).Extract() if err != nil { - return result, errors.Wrap(err, "failed to create port in ironic") + result, err = transientError(errors.Wrap(err, "failed to create port in ironic")) + return } } @@ -421,9 +424,10 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b } if imageData != nil { - updates, err := p.getImageUpdateOptsForNode(ironicNode, imageData) - if err != nil { - return result, errors.Wrap(err, "Could not get Image options for node") + updates, optsErr := p.getImageUpdateOptsForNode(ironicNode, imageData) + if optsErr != nil { + result, err = transientError(errors.Wrap(optsErr, "Could not get Image options for node")) + return } if len(updates) != 0 { _, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract() @@ -431,11 +435,11 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b case nil: case gophercloud.ErrDefault409: p.log.Info("could not update host settings in ironic, busy") - result.Dirty = true - result.RequeueAfter = provisionRequeueDelay - return result, nil + result, err = retryAfterDelay(provisionRequeueDelay) + return default: - return result, errors.Wrap(err, "failed to update host settings in ironic") + result, err = transientError(errors.Wrap(err, "failed to update host settings in ironic")) + return } } } @@ -444,13 +448,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b // node in ironic by looking it up. We need to check its // settings against what we have in the host, and change them // if there are differences. - if p.status.ID != ironicNode.UUID { - // Store the ID so other methods can assume it is set and - // so we can find the node using that value next time. - p.status.ID = ironicNode.UUID - result.Dirty = true - p.log.Info("setting provisioning id", "ID", p.status.ID) - } + provID = ironicNode.UUID if ironicNode.Name == "" { updates := nodes.UpdateOpts{ @@ -465,11 +463,11 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b case nil: case gophercloud.ErrDefault409: p.log.Info("could not update ironic node name, busy") - result.Dirty = true - result.RequeueAfter = provisionRequeueDelay - return result, nil + result, err = retryAfterDelay(provisionRequeueDelay) + return default: - return result, errors.Wrap(err, "failed to update ironc node name") + result, err = transientError(errors.Wrap(err, "failed to update ironc node name")) + return } p.log.Info("updated ironic node name") @@ -490,11 +488,11 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b case nil: case gophercloud.ErrDefault409: p.log.Info("could not update host driver settings, busy") - result.Dirty = true - result.RequeueAfter = provisionRequeueDelay - return result, nil + result, err = retryAfterDelay(provisionRequeueDelay) + return default: - return result, errors.Wrap(err, "failed to update host driver settings") + result, err = transientError(errors.Wrap(err, "failed to update host driver settings")) + return } p.log.Info("updated host driver settings") // We don't return here because we also have to set the @@ -521,48 +519,47 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b // If ironic is reporting an error, stop working on the node. if ironicNode.LastError != "" && !(credentialsChanged || force) { - result.ErrorMessage = ironicNode.LastError - return result, nil + result, err = operationFailed(ironicNode.LastError) + return } if ironicNode.TargetProvisionState == string(nodes.TargetManage) { // We have already tried to manage the node and did not // get an error, so do nothing and keep trying. - result.Dirty = true - result.RequeueAfter = provisionRequeueDelay - return result, nil + result, err = operationContinuing(provisionRequeueDelay) + return } - return p.changeNodeProvisionState( + result, err = p.changeNodeProvisionState( ironicNode, nodes.ProvisionStateOpts{Target: nodes.TargetManage}, ) + return case nodes.Verifying: // If we're still waiting for the state to change in Ironic, // return true to indicate that we're dirty and need to be // reconciled again. - result.RequeueAfter = provisionRequeueDelay - result.Dirty = true - return result, nil + result, err = operationContinuing(provisionRequeueDelay) + return case nodes.Manageable: p.log.Info("have manageable host") - return result, nil + return case nodes.Available: // The host is fully registered (and probably wasn't cleanly // deleted previously) p.log.Info("have available host") - return result, nil + return case nodes.Active: // The host is already running, maybe it's a master? p.log.Info("have active host", "image_source", ironicNode.InstanceInfo["image_source"]) - return result, nil + return default: - return result, nil + return } } @@ -579,14 +576,15 @@ func (p *ironicProvisioner) tryChangeNodeProvisionState(ironicNode *nodes.Node, success = true case gophercloud.ErrDefault409: p.log.Info("could not change state of host, busy") + result, err = retryAfterDelay(provisionRequeueDelay) + return default: - err = errors.Wrap(changeResult.Err, - fmt.Sprintf("failed to change provisioning state to %q", opts.Target)) + result, err = transientError(errors.Wrap(changeResult.Err, + fmt.Sprintf("failed to change provisioning state to %q", opts.Target))) return } - result.Dirty = true - result.RequeueAfter = provisionRequeueDelay + result, err = operationContinuing(provisionRequeueDelay) return } @@ -604,11 +602,12 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu ironicNode, err := p.findExistingHost() if err != nil { - err = errors.Wrap(err, "failed to find existing host") + result, err = transientError(errors.Wrap(err, "failed to find existing host")) return } if ironicNode == nil { - return result, nil, provisioner.NeedsRegistration + result, err = transientError(provisioner.NeedsRegistration) + return } status, err := introspection.GetIntrospectionStatus(p.inspector, ironicNode.UUID).Extract() @@ -617,9 +616,7 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu switch nodes.ProvisionState(ironicNode.ProvisionState) { case nodes.Inspecting, nodes.InspectWait: p.log.Info("inspection already started") - result.Dirty = true - result.RequeueAfter = introspectionRequeueDelay - err = nil + result, err = operationContinuing(introspectionRequeueDelay) return default: if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectFail && !force { @@ -645,10 +642,10 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu case nil: case gophercloud.ErrDefault409: p.log.Info("could not update host settings in ironic, busy") - result.Dirty = true + result, err = retryAfterDelay(provisionRequeueDelay) return default: - err = errors.Wrap(err, "failed to update host boot mode settings in ironic") + result, err = transientError(errors.Wrap(err, "failed to update host boot mode settings in ironic")) return } @@ -664,18 +661,17 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu return } } - err = errors.Wrap(err, "failed to extract hardware inspection status") + result, err = transientError(errors.Wrap(err, "failed to extract hardware inspection status")) return } if !status.Finished { p.log.Info("inspection in progress", "started_at", status.StartedAt) - result.Dirty = true // make sure we check back - result.RequeueAfter = introspectionRequeueDelay + result, err = operationContinuing(introspectionRequeueDelay) return } if status.Error != "" { p.log.Info("inspection failed", "error", status.Error) - result.ErrorMessage = status.Error + result, err = operationFailed(status.Error) return } @@ -684,52 +680,44 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu introData := introspection.GetIntrospectionData(p.inspector, ironicNode.UUID) data, err := introData.Extract() if err != nil { - err = errors.Wrap(err, "failed to retrieve hardware introspection data") + result, err = transientError(errors.Wrap(err, "failed to retrieve hardware introspection data")) return } p.log.Info("received introspection data", "data", introData.Body) details = hardwaredetails.GetHardwareDetails(data) p.publisher("InspectionComplete", "Hardware inspection completed") + result, err = operationComplete() return } // UpdateHardwareState fetches the latest hardware state of the server // and updates the HardwareDetails field of the host with details. It // is expected to do this in the least expensive way possible, such as -// reading from a cache, and return dirty only if any state -// information has changed. -func (p *ironicProvisioner) UpdateHardwareState() (result provisioner.Result, err error) { +// reading from a cache. +func (p *ironicProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) { p.log.Info("updating hardware state") ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + err = errors.Wrap(err, "failed to find existing host") + return } if ironicNode == nil { - return result, provisioner.NeedsRegistration + err = provisioner.NeedsRegistration + return } - var discoveredVal bool switch ironicNode.PowerState { - case powerOn: - discoveredVal = true - case powerOff: - discoveredVal = false + case powerOn, powerOff: + discoveredVal := ironicNode.PowerState == powerOn + hwState.PoweredOn = &discoveredVal case powerNone: p.log.Info("could not determine power state", "value", ironicNode.PowerState) - return result, nil default: p.log.Info("unknown power state", "value", ironicNode.PowerState) - return result, nil - } - - if discoveredVal != p.host.Status.PoweredOn { - p.log.Info("updating power status", "discovered", discoveredVal) - p.host.Status.PoweredOn = discoveredVal - result.Dirty = true } - return result, nil + return } func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image) (updates nodes.UpdateOpts, err error) { @@ -1009,17 +997,16 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, hostCon updates, err := p.getUpdateOptsForNode(ironicNode) if err != nil { - return result, errors.Wrap(err, "failed to update opts for node") + return transientError(errors.Wrap(err, "failed to update opts for node")) } _, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract() switch err.(type) { case nil: case gophercloud.ErrDefault409: p.log.Info("could not update host settings in ironic, busy") - result.Dirty = true - return result, nil + return retryAfterDelay(provisionRequeueDelay) default: - return result, errors.Wrap(err, "failed to update host settings in ironic") + return transientError(errors.Wrap(err, "failed to update host settings in ironic")) } p.log.Info("validating host settings") @@ -1029,15 +1016,12 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, hostCon case nil: case gophercloud.ErrDefault409: p.log.Info("could not validate host during registration, busy") - result.Dirty = true - return result, nil + return retryAfterDelay(provisionRequeueDelay) default: - return result, errors.Wrap(err, "failed to validate host during registration") + return transientError(errors.Wrap(err, "failed to validate host during registration")) } if errorMessage != "" { - result.ErrorMessage = errorMessage - result.Dirty = true // validateNode() would have set the errors - return result, nil + return operationFailed(errorMessage) } // If validation is successful we can start moving the host @@ -1058,18 +1042,16 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er var ironicNode *nodes.Node if ironicNode, err = p.findExistingHost(); err != nil { - err = errors.Wrap(err, "could not find host to adpot") - return + return transientError(errors.Wrap(err, "could not find host to adpot")) } if ironicNode == nil { - err = provisioner.NeedsRegistration - return + return transientError(provisioner.NeedsRegistration) } switch nodes.ProvisionState(ironicNode.ProvisionState) { case nodes.Enroll, nodes.Verifying: - err = fmt.Errorf("Invalid state for adopt: %s", - ironicNode.ProvisionState) + return transientError(fmt.Errorf("Invalid state for adopt: %s", + ironicNode.ProvisionState)) case nodes.Manageable: return p.changeNodeProvisionState( ironicNode, @@ -1078,8 +1060,7 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er }, ) case nodes.Adopting: - result.RequeueAfter = provisionRequeueDelay - result.Dirty = true + return operationContinuing(provisionRequeueDelay) case nodes.AdoptFail: if force { return p.changeNodeProvisionState( @@ -1089,13 +1070,13 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er }, ) } else { - result.ErrorMessage = fmt.Sprintf("Host adoption failed: %s", - ironicNode.LastError) + return operationFailed(fmt.Sprintf("Host adoption failed: %s", + ironicNode.LastError)) } case nodes.Active: default: } - return + return operationComplete() } // Provision writes the image from the host spec to the host. It may @@ -1105,10 +1086,10 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu var ironicNode *nodes.Node if ironicNode, err = p.findExistingHost(); err != nil { - return result, errors.Wrap(err, "could not find host to receive image") + return transientError(errors.Wrap(err, "could not find host to receive image")) } if ironicNode == nil { - return result, provisioner.NeedsRegistration + return transientError(provisioner.NeedsRegistration) } p.log.Info("provisioning image to host", "state", ironicNode.ProvisionState) @@ -1128,8 +1109,6 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu "same", ironicHasSameImage, "provisionState", ironicNode.ProvisionState) - result.RequeueAfter = provisionRequeueDelay - // Ironic has the settings it needs, see if it finds any issues // with them. switch nodes.ProvisionState(ironicNode.ProvisionState) { @@ -1144,13 +1123,11 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu // top of relational databases... if ironicNode.LastError == "" { p.log.Info("failed but error message not available") - result.Dirty = true - return result, nil + return retryAfterDelay(0) } p.log.Info("found error", "msg", ironicNode.LastError) - result.ErrorMessage = fmt.Sprintf("Image provisioning failed: %s", - ironicNode.LastError) - return result, nil + return operationFailed(fmt.Sprintf("Image provisioning failed: %s", + ironicNode.LastError)) } p.log.Info("recovering from previous failure") if provResult, err := p.setUpForProvisioning(ironicNode, hostConf); err != nil || provResult.Dirty || provResult.ErrorMessage != "" { @@ -1186,17 +1163,17 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu // Retrieve cloud-init user data userData, err := hostConf.UserData() if err != nil { - return result, errors.Wrap(err, "could not retrieve user data") + return transientError(errors.Wrap(err, "could not retrieve user data")) } // Retrieve cloud-init network_data.json. Default value is empty networkDataRaw, err := hostConf.NetworkData() if err != nil { - return result, errors.Wrap(err, "could not retrieve network data") + return transientError(errors.Wrap(err, "could not retrieve network data")) } var networkData map[string]interface{} if err = yaml.Unmarshal([]byte(networkDataRaw), &networkData); err != nil { - return result, errors.Wrap(err, "failed to unmarshal network_data.json from secret") + return transientError(errors.Wrap(err, "failed to unmarshal network_data.json from secret")) } // Retrieve cloud-init meta_data.json with falback to default @@ -1209,11 +1186,11 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu } metaDataRaw, err := hostConf.MetaData() if err != nil { - return result, errors.Wrap(err, "could not retrieve metadata") + return transientError(errors.Wrap(err, "could not retrieve metadata")) } if metaDataRaw != "" { if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil { - return result, errors.Wrap(err, "failed to unmarshal metadata from secret") + return transientError(errors.Wrap(err, "failed to unmarshal metadata from secret")) } } @@ -1225,7 +1202,7 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu NetworkData: networkData, } if err != nil { - return result, errors.Wrap(err, "failed to build config drive") + return transientError(errors.Wrap(err, "failed to build config drive")) } p.log.Info("triggering provisioning with config drive") } else { @@ -1245,15 +1222,14 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu p.publisher("ProvisioningComplete", fmt.Sprintf("Image provisioning completed for %s", p.host.Spec.Image.URL)) p.log.Info("finished provisioning") - return result, nil + return operationComplete() default: // wait states like cleaning and clean wait p.log.Info("waiting for host to become available", "state", ironicNode.ProvisionState, "deploy step", ironicNode.DeployStep) - result.Dirty = true - return result, nil + return operationContinuing(provisionRequeueDelay) } } @@ -1273,11 +1249,11 @@ func (p *ironicProvisioner) setMaintenanceFlag(ironicNode *nodes.Node, value boo case nil: case gophercloud.ErrDefault409: p.log.Info("could not set host maintenance flag, busy") + return retryAfterDelay(provisionRequeueDelay) default: - return result, errors.Wrap(err, "failed to set host maintenance flag") + return transientError(errors.Wrap(err, "failed to set host maintenance flag")) } - result.Dirty = true - return result, nil + return operationContinuing(0) } // Deprovision removes the host from the image. It may be called @@ -1288,10 +1264,10 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result, ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + return transientError(errors.Wrap(err, "failed to find existing host")) } if ironicNode == nil { - return result, provisioner.NeedsRegistration + return transientError(provisioner.NeedsRegistration) } p.log.Info("deprovisioning host", @@ -1337,27 +1313,21 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result, case nodes.Available: p.publisher("DeprovisioningComplete", "Image deprovisioning completed") - return result, nil + return operationComplete() case nodes.Deleting: p.log.Info("deleting") // Transitions to Cleaning upon completion - result.Dirty = true - result.RequeueAfter = deprovisionRequeueDelay - return result, nil + return operationContinuing(deprovisionRequeueDelay) case nodes.Cleaning: p.log.Info("cleaning") // Transitions to Available upon completion - result.Dirty = true - result.RequeueAfter = deprovisionRequeueDelay - return result, nil + return operationContinuing(deprovisionRequeueDelay) case nodes.CleanWait: p.log.Info("cleaning") - result.Dirty = true - result.RequeueAfter = deprovisionRequeueDelay - return result, nil + return operationContinuing(deprovisionRequeueDelay) case nodes.Active: p.log.Info("starting deprovisioning") @@ -1368,7 +1338,8 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result, ) default: - return result, fmt.Errorf("Unhandled ironic state %s", ironicNode.ProvisionState) + // FIXME(zaneb): this error is unlikely to actually be transient + return transientError(fmt.Errorf("Unhandled ironic state %s", ironicNode.ProvisionState)) } } @@ -1379,11 +1350,11 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) { ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + return transientError(errors.Wrap(err, "failed to find existing host")) } if ironicNode == nil { p.log.Info("no node found, already deleted") - return result, nil + return operationComplete() } p.log.Info("deleting host", @@ -1425,14 +1396,14 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) { p.log.Info("removed") case gophercloud.ErrDefault409: p.log.Info("could not remove host, busy") + return retryAfterDelay(provisionRequeueDelay) case gophercloud.ErrDefault404: p.log.Info("did not find host to delete, OK") default: - return result, errors.Wrap(err, "failed to remove host") + return transientError(errors.Wrap(err, "failed to remove host")) } - result.Dirty = true - return result, nil + return operationContinuing(0) } func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.TargetPowerState) (result provisioner.Result, err error) { @@ -1443,9 +1414,7 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar "state", ironicNode.ProvisionState, "target state", ironicNode.TargetProvisionState, ) - result.Dirty = true - result.RequeueAfter = powerRequeueDelay - return result, nil + return operationContinuing(powerRequeueDelay) } powerStateOpts := nodes.PowerStateOpts{ @@ -1462,12 +1431,11 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar switch changeResult.Err.(type) { case nil: - result.Dirty = true p.log.Info("power change OK") + return operationContinuing(0) case gophercloud.ErrDefault409: p.log.Info("host is locked, trying again after delay", "delay", powerRequeueDelay) - result.Dirty = true - result.RequeueAfter = powerRequeueDelay + result, _ = retryAfterDelay(powerRequeueDelay) return result, HostLockedError{Address: p.host.Spec.BMC.Address} case gophercloud.ErrDefault400: // Error 400 Bad Request means target power state is not supported by vendor driver @@ -1475,10 +1443,8 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar return result, SoftPowerOffUnsupportedError{Address: p.host.Spec.BMC.Address} default: p.log.Info("power change error", "message", changeResult.Err) - return result, errors.Wrap(changeResult.Err, "failed to change power state") + return transientError(errors.Wrap(changeResult.Err, "failed to change power state")) } - - return result, nil } // PowerOn ensures the server is powered on independently of any image @@ -1488,7 +1454,7 @@ func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) { ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + return transientError(errors.Wrap(err, "failed to find existing host")) } p.log.Info("checking current state", @@ -1498,13 +1464,14 @@ func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) { if ironicNode.PowerState != powerOn { if ironicNode.TargetPowerState == powerOn { p.log.Info("waiting for power status to change") - result.RequeueAfter = powerRequeueDelay - result.Dirty = true - return result, nil + return operationContinuing(powerRequeueDelay) } result, err = p.changePower(ironicNode, nodes.PowerOn) - if err != nil { - return result, errors.Wrap(err, "failed to power on host") + switch err.(type) { + case nil: + case HostLockedError: + default: + return transientError(errors.Wrap(err, "failed to power on host")) } p.publisher("PowerOn", "Host powered on") } @@ -1525,12 +1492,9 @@ func (p *ironicProvisioner) PowerOff() (result provisioner.Result, err error) { case SoftPowerOffUnsupportedError, SoftPowerOffFailed: return p.hardPowerOff() case HostLockedError: - result.RequeueAfter = powerRequeueDelay - result.Dirty = true - return result, nil + return retryAfterDelay(powerRequeueDelay) default: - result.RequeueAfter = powerRequeueDelay - return result, err + return transientError(err) } } return result, nil @@ -1542,24 +1506,23 @@ func (p *ironicProvisioner) hardPowerOff() (result provisioner.Result, err error ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + return transientError(errors.Wrap(err, "failed to find existing host")) } if ironicNode.PowerState != powerOff { if ironicNode.TargetPowerState == powerOff { p.log.Info("waiting for power status to change") - result.RequeueAfter = powerRequeueDelay - result.Dirty = true - return result, nil + return operationContinuing(powerRequeueDelay) } result, err = p.changePower(ironicNode, nodes.PowerOff) if err != nil { - return result, errors.Wrap(err, "failed to power off host") + return transientError(errors.Wrap(err, "failed to power off host")) } p.publisher("PowerOff", "Host powered off") + return result, err } - return result, nil + return operationComplete() } // softPowerOff sends 'soft power off' request to BM node. @@ -1572,7 +1535,7 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error ironicNode, err := p.findExistingHost() if err != nil { - return result, errors.Wrap(err, "failed to find existing host") + return transientError(errors.Wrap(err, "failed to find existing host")) } if ironicNode.PowerState != powerOff { @@ -1580,9 +1543,7 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error // If the target state is either powerOff or softPowerOff, then we should wait if targetState == powerOff || targetState == softPowerOff { p.log.Info("waiting for power status to change") - result.RequeueAfter = powerRequeueDelay - result.Dirty = true - return result, nil + return operationContinuing(powerRequeueDelay) } // If the target state is unset while the last error is set, // then the last execution of soft power off has failed. @@ -1591,8 +1552,7 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error } result, err = p.changePower(ironicNode, nodes.SoftPowerOff) if err != nil { - result.RequeueAfter = powerRequeueDelay - return result, err + return transientError(err) } p.publisher("PowerOff", "Host soft powered off") } diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go index fbf2cba020..864609a3a6 100644 --- a/pkg/provisioner/ironic/ironic_test.go +++ b/pkg/provisioner/ironic/ironic_test.go @@ -16,10 +16,10 @@ func init() { logf.SetLogger(logf.ZapLogger(true)) } -func makeHost() *metal3v1alpha1.BareMetalHost { +func makeHost() metal3v1alpha1.BareMetalHost { rotational := true - return &metal3v1alpha1.BareMetalHost{ + return metal3v1alpha1.BareMetalHost{ ObjectMeta: metav1.ObjectMeta{ Name: "myhost", Namespace: "myns", diff --git a/pkg/provisioner/ironic/power_test.go b/pkg/provisioner/ironic/power_test.go index 890fd19cf8..8aeda72434 100644 --- a/pkg/provisioner/ironic/power_test.go +++ b/pkg/provisioner/ironic/power_test.go @@ -73,7 +73,6 @@ func TestPowerOn(t *testing.T) { }).WithNodeStatesPower(nodeUUID, http.StatusConflict).WithNodeStatesPowerUpdate(nodeUUID, http.StatusConflict), expectedRequestAfter: 10, expectedDirty: true, - expectedError: true, }, } diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go index 8c463ffaab..947cecab83 100644 --- a/pkg/provisioner/ironic/provision_test.go +++ b/pkg/provisioner/ironic/provision_test.go @@ -22,6 +22,7 @@ func TestProvision(t *testing.T) { ironic *testserver.IronicMock expectedDirty bool expectedError bool + expectedErrorMessage bool expectedRequestAfter int }{ { @@ -31,7 +32,8 @@ func TestProvision(t *testing.T) { UUID: nodeUUID, }), expectedRequestAfter: 0, - expectedDirty: true, + expectedDirty: false, + expectedErrorMessage: true, }, { name: "cleanFail state", @@ -58,7 +60,8 @@ func TestProvision(t *testing.T) { UUID: nodeUUID, }), expectedRequestAfter: 0, - expectedDirty: true, + expectedDirty: false, + expectedErrorMessage: true, }, { name: "active state", @@ -66,7 +69,7 @@ func TestProvision(t *testing.T) { ProvisionState: string(nodes.Active), UUID: nodeUUID, }), - expectedRequestAfter: 10, + expectedRequestAfter: 0, expectedDirty: false, }, { @@ -108,6 +111,11 @@ func TestProvision(t *testing.T) { assert.Equal(t, tc.expectedDirty, result.Dirty) assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter) + if !tc.expectedErrorMessage { + assert.Equal(t, "", result.ErrorMessage) + } else { + assert.NotEqual(t, "", result.ErrorMessage) + } if !tc.expectedError { assert.NoError(t, err) } else { diff --git a/pkg/provisioner/ironic/result.go b/pkg/provisioner/ironic/result.go new file mode 100644 index 0000000000..cb9057e821 --- /dev/null +++ b/pkg/provisioner/ironic/result.go @@ -0,0 +1,36 @@ +package ironic + +import ( + "time" + + "github.com/metal3-io/baremetal-operator/pkg/provisioner" +) + +func retryAfterDelay(delay time.Duration) (provisioner.Result, error) { + // TODO(zaneb): this is currently indistinguishable from the result of + // operationContinuing() from the caller's perspective. Changes are + // required to the Result structure to enable this to be distinguished. + return provisioner.Result{ + Dirty: true, + RequeueAfter: delay, + }, nil +} + +func operationContinuing(delay time.Duration) (provisioner.Result, error) { + return provisioner.Result{ + Dirty: true, + RequeueAfter: delay, + }, nil +} + +func operationComplete() (provisioner.Result, error) { + return provisioner.Result{}, nil +} + +func operationFailed(message string) (provisioner.Result, error) { + return provisioner.Result{ErrorMessage: message}, nil +} + +func transientError(err error) (provisioner.Result, error) { + return provisioner.Result{}, err +} diff --git a/pkg/provisioner/ironic/updatehardwarestate_test.go b/pkg/provisioner/ironic/updatehardwarestate_test.go index 63944b8137..e4e89b0b80 100644 --- a/pkg/provisioner/ironic/updatehardwarestate_test.go +++ b/pkg/provisioner/ironic/updatehardwarestate_test.go @@ -3,7 +3,6 @@ package ironic import ( "net/http" "testing" - "time" "github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes" "github.com/stretchr/testify/assert" @@ -24,9 +23,7 @@ func TestUpdateHardwareState(t *testing.T) { hostCurrentlyPowered bool hostName string - expectedDirty bool - expectedRequestAfter int - expectedResultError string + expectUnreadablePower bool expectedPublish string expectedError string @@ -36,6 +33,7 @@ func TestUpdateHardwareState(t *testing.T) { ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{ UUID: nodeUUID, }), + expectUnreadablePower: true, }, { name: "updated-power-on-state", @@ -52,8 +50,6 @@ func TestUpdateHardwareState(t *testing.T) { PowerState: "power on", }), hostCurrentlyPowered: false, - - expectedDirty: true, }, { name: "updated-power-off-state", @@ -70,8 +66,6 @@ func TestUpdateHardwareState(t *testing.T) { PowerState: "power off", }), hostCurrentlyPowered: true, - - expectedDirty: true, }, { name: "no-power", @@ -79,7 +73,8 @@ func TestUpdateHardwareState(t *testing.T) { UUID: nodeUUID, PowerState: "None", }), - hostCurrentlyPowered: true, + hostCurrentlyPowered: true, + expectUnreadablePower: true, }, { name: "node-not-found", @@ -88,6 +83,8 @@ func TestUpdateHardwareState(t *testing.T) { ironic: testserver.NewIronic(t).Ready().NodeError(nodeUUID, http.StatusGatewayTimeout), expectedError: "failed to find existing host: failed to find node by ID 33ce8659-7400-4c68-9535-d10766f07a58: Expected HTTP response code \\[200\\].*", + + expectUnreadablePower: true, }, { name: "node-not-found-by-name", @@ -96,12 +93,16 @@ func TestUpdateHardwareState(t *testing.T) { ironic: testserver.NewIronic(t).Ready().NoNode(nodeUUID).NodeError("myhost", http.StatusGatewayTimeout), expectedError: "failed to find existing host: failed to find node by name worker-0: EOF", + + expectUnreadablePower: true, }, { name: "not-ironic-node", ironic: testserver.NewIronic(t).Ready().NoNode(nodeUUID).NoNode("myhost"), expectedError: "Host not registered", + + expectUnreadablePower: true, }, } @@ -136,11 +137,9 @@ func TestUpdateHardwareState(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.UpdateHardwareState() + hwStatus, err := prov.UpdateHardwareState() - 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) + assert.Equal(t, tc.expectUnreadablePower, hwStatus.PoweredOn == nil) assert.Equal(t, tc.expectedPublish, publishedMsg) if tc.expectedError == "" { diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go index 88714db735..d712064f9e 100644 --- a/pkg/provisioner/ironic/updateopts_test.go +++ b/pkg/provisioner/ironic/updateopts_test.go @@ -83,7 +83,7 @@ func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) { } func TestGetUpdateOptsForNodeVirtual(t *testing.T) { - host := &metal3v1alpha1.BareMetalHost{ + host := metal3v1alpha1.BareMetalHost{ ObjectMeta: metav1.ObjectMeta{ Name: "myhost", Namespace: "myns", @@ -188,7 +188,7 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) { } func TestGetUpdateOptsForNodeDell(t *testing.T) { - host := &metal3v1alpha1.BareMetalHost{ + host := metal3v1alpha1.BareMetalHost{ ObjectMeta: metav1.ObjectMeta{ Name: "myhost", Namespace: "myns", diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go index 13f4977788..378d62ff63 100644 --- a/pkg/provisioner/ironic/validatemanagementaccess_test.go +++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go @@ -33,7 +33,7 @@ func TestValidateManagementAccessNoMAC(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, _, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } @@ -63,7 +63,7 @@ func TestValidateManagementAccessMACOptional(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, _, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } @@ -95,13 +95,13 @@ func TestValidateManagementAccessCreateNode(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, provID, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } assert.Equal(t, "", result.ErrorMessage) assert.NotEqual(t, "", createdNode.UUID) - assert.Equal(t, createdNode.UUID, host.Status.Provisioning.ID) + assert.Equal(t, createdNode.UUID, provID) } func TestValidateManagementAccessExistingNode(t *testing.T) { @@ -130,12 +130,12 @@ func TestValidateManagementAccessExistingNode(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, provID, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } assert.Equal(t, "", result.ErrorMessage) - assert.Equal(t, "uuid", host.Status.Provisioning.ID) + assert.Equal(t, "uuid", provID) } func TestValidateManagementAccessExistingNodeContinue(t *testing.T) { @@ -193,7 +193,7 @@ func TestValidateManagementAccessExistingNodeContinue(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, _, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } @@ -238,7 +238,7 @@ func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, _, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } @@ -280,12 +280,12 @@ func TestValidateManagementAccessNewCredentials(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(true, false) + result, provID, err := prov.ValidateManagementAccess(true, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } assert.Equal(t, "", result.ErrorMessage) - assert.Equal(t, "uuid", host.Status.Provisioning.ID) + assert.Equal(t, "uuid", provID) updates := ironic.GetLastNodeUpdateRequestFor("uuid") newValues := updates[0].Value.(map[string]interface{}) @@ -327,12 +327,12 @@ func TestValidateManagementAccessLinkExistingIronicNodeByMAC(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, provID, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } assert.Equal(t, "", result.ErrorMessage) - assert.NotEqual(t, "", host.Status.Provisioning.ID) + assert.NotEqual(t, "", provID) } func TestValidateManagementAccessExistingPortWithWrongUUID(t *testing.T) { @@ -370,7 +370,7 @@ func TestValidateManagementAccessExistingPortWithWrongUUID(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - _, err = prov.ValidateManagementAccess(false, false) + _, _, err = prov.ValidateManagementAccess(false, false) assert.EqualError(t, err, "failed to find existing host: port exists but linked node doesn't random-wrong-id: Resource not found") } @@ -412,7 +412,7 @@ func TestValidateManagementAccessExistingPortButHasName(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - _, err = prov.ValidateManagementAccess(false, false) + _, _, err = prov.ValidateManagementAccess(false, false) assert.EqualError(t, err, "failed to find existing host: node found by MAC but has a name: wrong-name") } @@ -450,10 +450,10 @@ func TestValidateManagementAccessAddTwoHostsWithSameMAC(t *testing.T) { t.Fatalf("could not create provisioner: %s", err) } - result, err := prov.ValidateManagementAccess(false, false) + result, provID, err := prov.ValidateManagementAccess(false, false) if err != nil { t.Fatalf("error from ValidateManagementAccess: %s", err) } assert.Equal(t, "", result.ErrorMessage) - assert.NotEqual(t, "", host.Status.Provisioning.ID) + assert.NotEqual(t, "", provID) } diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go index e5e01f2c29..70c9b38027 100644 --- a/pkg/provisioner/provisioner.go +++ b/pkg/provisioner/provisioner.go @@ -17,7 +17,7 @@ Package provisioning defines the API for talking to the provisioning backend. type EventPublisher func(reason, message string) // Factory is the interface for creating new Provisioner objects. -type Factory func(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish EventPublisher) (Provisioner, error) +type Factory func(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish EventPublisher) (Provisioner, error) // HostConfigData retrieves host configuration data type HostConfigData interface { @@ -43,7 +43,7 @@ type Provisioner interface { // of credentials it has are different from the credentials it has // previously been using, without implying that either set of // credentials is correct. - ValidateManagementAccess(credentialsChanged, force bool) (result Result, err error) + ValidateManagementAccess(credentialsChanged, force bool) (result Result, provID string, err error) // InspectHardware updates the HardwareDetails field of the host with // details of devices discovered on the hardware. It may be called @@ -54,9 +54,8 @@ type Provisioner interface { // UpdateHardwareState fetches the latest hardware state of the // server and updates the HardwareDetails field of the host with // details. It is expected to do this in the least expensive way - // possible, such as reading from a cache, and return dirty only - // if any state information has changed. - UpdateHardwareState() (result Result, err error) + // possible, such as reading from a cache. + UpdateHardwareState() (hwState HardwareState, err error) // Adopt brings an externally-provisioned host under management by // the provisioner. @@ -102,4 +101,11 @@ type Result struct { ErrorMessage string } +// HardwareState holds the response from an UpdateHardwareState call +type HardwareState struct { + // PoweredOn is a pointer to a bool indicating whether the Host is currently + // powered on. The value is nil if the power state cannot be determined. + PoweredOn *bool +} + var NeedsRegistration = errors.New("Host not registered")