Repository: metal3-io/baremetal-operator
Pull request #841 (2021-03-30 22:25:29)

Better handling of power management errors
Attempts to fix https://github.com/metal3-io/baremetal-operator/issues/828

This fix should have the following components based on the above bug:
1. If Ironic accepts the power state change but is unable to carry it out (expressed via LastError from ironic), that should result in PowerOn() and PowerOff() returning an ErrorMessage. 
2. Refactor PowerOff() in such a way that if soft power off fails, it proceeds to hard power off without reporting an error. It is possible that this logic is moved out of here and into Ironic itself.
3. Add a force flag to the PowerOff() so that it skips soft power off and directly proceeds to hard power off after the previous call results in an error.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#" title="Author self-approved">sadasu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **russellb**
You can assign the PR to them by writing `/assign @russellb` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["russellb"]} -->
metal3-io-bot: @sadasu: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | 6ded954a46a97feb282be1015935a27773c07b71 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/841/unit/1377024239709720576) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
sadasu: /test-integration
sadasu: /assign @zaneb @andfasano @dtantsur 
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#" title="Author self-approved">sadasu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dtantsur**
You can assign the PR to them by writing `/assign @dtantsur` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dtantsur"]} -->
metal3-io-bot: @sadasu: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | 59c160d2f248d2f1c10fdd7c8775a09b2e83243c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/841/unit/1379486262079000584) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
sadasu: /test-integration
sadasu: /test-integration
sadasu: /test-integration
sadasu: /test-integration
sadasu: /test-integration 
sadasu: /test-integration
sadasu: /test-integration
sadasu: /test-integration
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#" title="Author self-approved">sadasu</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-639030462" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
sadasu: @rdoxenham @andfasano any chance I can get a 2nd approval from either of you ?
rdoxenham: > @rdoxenham @andfasano any chance I can get a 2nd approval from either of you ?

sorry, I don't have approve/lgtm rights on metal3-io, only OpenShift :( 
sadasu: /cc @stbenjam 
sadasu: /cc @elfosardo @dtantsur 
elfosardo: /lgtm
metal3-io-bot: @elfosardo: adding LGTM is restricted to approvers and reviewers in OWNERS files.

<details>

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/841#issuecomment-864857358):

>/lgtm


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-688184844" title="Approved">elfosardo</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-639030462" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
sadasu: /cc @honza 
sadasu: /cc @maelk @furkatgofurov7 

This has been fixed for a while. I am looking for a /lgtm.
zaneb: @andfasano can we merge this now?
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-698004905" title="LGTM">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-688184844" title="Approved">elfosardo</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/841#pullrequestreview-639030462" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
furkatgofurov7: /test ?
metal3-io-bot: @furkatgofurov7: The following commands are available to trigger jobs:
* `/test gofmt`
* `/test gosec`
* `/test gomod`
* `/test markdownlint`
* `/test shellcheck`
* `/test unit`
* `/test generate`
* `/test golint`
* `/test manifestlint`

Use `/test all` to run all jobs.


<details>

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/841#issuecomment-876954874):

>/test ?


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
furkatgofurov7: /test all
furkatgofurov7: /hold
metal3-io-bot: @sadasu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 55542d26cea2f405b6f7a3009e1f8a52b3441eb8 | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/841/golint/1413388271420968960) | `/test golint`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
furkatgofurov7: /test golint
metal3-io-bot: @sadasu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 55542d26cea2f405b6f7a3009e1f8a52b3441eb8 | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/841/golint/1413389581599903744) | `/test golint`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
furkatgofurov7: /hold cancel
fmuyassarov: /test golint
metal3-io-bot: @sadasu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 55542d26cea2f405b6f7a3009e1f8a52b3441eb8 | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/841/golint/1413406627465269248) | `/test golint`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 5d25bd9e..30c06ab5 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -1005,9 +1005,12 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 		"reboot process", desiredPowerOnState != info.host.Spec.Online)
 
 	if desiredPowerOnState {
-		provResult, err = prov.PowerOn()
+		provResult, err = prov.PowerOn(info.host.Status.ErrorType == metal3v1alpha1.PowerManagementError)
 	} else {
-		provResult, err = prov.PowerOff(desiredRebootMode)
+		if info.host.Status.ErrorCount > 0 {
+			desiredRebootMode = metal3v1alpha1.RebootModeHard
+		}
+		provResult, err = prov.PowerOff(desiredRebootMode, info.host.Status.ErrorType == metal3v1alpha1.PowerManagementError)
 	}
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to manage power state of host")}
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 00741b33..be79a6f4 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -305,7 +305,7 @@ func (p *demoProvisioner) Detach() (result provisioner.Result, err error) {
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *demoProvisioner) PowerOn() (result provisioner.Result, err error) {
+func (p *demoProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
 
 	hostName := p.objectMeta.Name
 	switch hostName {
@@ -326,7 +326,7 @@ func (p *demoProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *demoProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
+func (p *demoProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 
 	hostName := p.objectMeta.Name
 	switch hostName {
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index ab0b2e21..b3eee1cf 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -291,7 +291,7 @@ func (p *fixtureProvisioner) Detach() (result provisioner.Result, err error) {
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered on")
 
 	if !p.state.poweredOn {
@@ -307,7 +307,7 @@ func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *fixtureProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off")
 
 	if p.state.poweredOn {
diff --git a/pkg/provisioner/ironic/errors.go b/pkg/provisioner/ironic/errors.go
index c3384bcd..8a69c0cb 100644
--- a/pkg/provisioner/ironic/errors.go
+++ b/pkg/provisioner/ironic/errors.go
@@ -9,15 +9,6 @@ func (e SoftPowerOffUnsupportedError) Error() string {
 	return "soft power off is unsupported on BMC"
 }
 
-// SoftPowerOffFailed is returned when the soft power off command
-// finishes with failure.
-type SoftPowerOffFailed struct {
-}
-
-func (e SoftPowerOffFailed) Error() string {
-	return "Soft power off has failed on BMC"
-}
-
 // HostLockedError is returned when the BMC host is
 // locked.
 type HostLockedError struct {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 7d4a3580..eb42929c 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1519,7 +1519,7 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) {
+func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered on")
 
 	ironicNode, err := p.getNode()
@@ -1535,46 +1535,39 @@ func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) {
 			p.log.Info("waiting for power status to change")
 			return operationContinuing(powerRequeueDelay)
 		}
-		result, err = p.changePower(ironicNode, nodes.PowerOn)
+		if ironicNode.LastError != "" && !force {
+			p.log.Info("PowerOn operation failed", "msg", ironicNode.LastError)
+			return operationFailed(fmt.Sprintf("PowerOn operation failed: %s",
+				ironicNode.LastError))
+		}
+		if result, err = p.changePower(ironicNode, nodes.PowerOn); err == nil {
+			p.publisher("PowerOn", "Host powered on")
+			return result, nil
+		}
 		switch err.(type) {
-		case nil:
 		case HostLockedError:
+			return retryAfterDelay(powerRequeueDelay)
 		default:
-			return transientError(errors.Wrap(err, "failed to power on host"))
+			return transientError(errors.Wrap(err, "failed to PowerOn node"))
 		}
-		p.publisher("PowerOn", "Host powered on")
 	}
-
 	return result, nil
 }
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 	p.log.Info(fmt.Sprintf("ensuring host is powered off (mode: %s)", rebootMode))
 
-	if rebootMode == metal3v1alpha1.RebootModeHard {
-		result, err = p.hardPowerOff()
-	} else {
-		result, err = p.softPowerOff()
+	if rebootMode == metal3v1alpha1.RebootModeSoft {
+		return p.softPowerOff()
 	}
-	if err != nil {
-		switch err.(type) {
-		// In case of soft power off is unsupported or has failed,
-		// we activate hard power off.
-		case SoftPowerOffUnsupportedError, SoftPowerOffFailed:
-			return p.hardPowerOff()
-		case HostLockedError:
-			return retryAfterDelay(powerRequeueDelay)
-		default:
-			return transientError(err)
-		}
-	}
-	return result, nil
+	// Reboot mode is hard or force flag is set
+	return p.hardPowerOff(force)
 }
 
 // hardPowerOff sends 'power off' request to BM node and waits for the result
-func (p *ironicProvisioner) hardPowerOff() (result provisioner.Result, err error) {
+func (p *ironicProvisioner) hardPowerOff(force bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off by \"hard power off\" command")
 
 	ironicNode, err := p.getNode()
@@ -1583,13 +1576,22 @@ func (p *ironicProvisioner) hardPowerOff() (result provisioner.Result, err error
 	}
 
 	if ironicNode.PowerState != powerOff {
+		if ironicNode.LastError != "" && !force {
+			p.log.Info("hard power off error", "msg", ironicNode.LastError)
+			return operationFailed(ironicNode.LastError)
+		}
 		if ironicNode.TargetPowerState == powerOff {
 			p.log.Info("waiting for power status to change")
 			return operationContinuing(powerRequeueDelay)
 		}
 		result, err = p.changePower(ironicNode, nodes.PowerOff)
 		if err != nil {
-			return transientError(errors.Wrap(err, "failed to power off host"))
+			switch err.(type) {
+			case HostLockedError:
+				return retryAfterDelay(powerRequeueDelay)
+			default:
+				return transientError(errors.Wrap(err, "failed to power off host"))
+			}
 		}
 		p.publisher("PowerOff", "Host powered off")
 		return result, err
@@ -1621,11 +1623,17 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error
 		// If the target state is unset while the last error is set,
 		// then the last execution of soft power off has failed.
 		if targetState == "" && ironicNode.LastError != "" {
-			return result, SoftPowerOffFailed{}
+			p.log.Info("soft power off error", "msg", ironicNode.LastError)
+			return operationFailed(ironicNode.LastError)
 		}
 		result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
 		if err != nil {
-			return transientError(err)
+			switch err.(type) {
+			case HostLockedError:
+				return retryAfterDelay(powerRequeueDelay)
+			default:
+				return transientError(errors.Wrap(err, "failed to power off host"))
+			}
 		}
 		p.publisher("PowerOff", "Host soft powered off")
 	}
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 39d241b8..a579e44c 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -144,12 +144,12 @@ type Provisioner interface {
 
 	// PowerOn ensures the server is powered on independently of any image
 	// provisioning operation.
-	PowerOn() (result Result, err error)
+	PowerOn(force bool) (result Result, err error)
 
 	// PowerOff ensures the server is powered off independently of any image
 	// provisioning operation. The boolean argument may be used to specify
 	// if a hard reboot (force power off) is required - true if so.
-	PowerOff(rebootMode metal3v1alpha1.RebootMode) (result Result, err error)
+	PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result Result, err error)
 
 	// IsReady checks if the provisioning backend is available to accept
 	// all the incoming requests.


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index acab71fb..4982fbd7 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1172,11 +1172,11 @@ func (m *mockProvisioner) Detach() (result provisioner.Result, err error) {
 	return res, err
 }
 
-func (m *mockProvisioner) PowerOn() (result provisioner.Result, err error) {
+func (m *mockProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOn"), err
 }
 
-func (m *mockProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
+func (m *mockProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOff"), err
 }
 
diff --git a/pkg/provisioner/ironic/power_test.go b/pkg/provisioner/ironic/power_test.go
index 2db33570..0a899d5f 100644
--- a/pkg/provisioner/ironic/power_test.go
+++ b/pkg/provisioner/ironic/power_test.go
@@ -20,11 +20,13 @@ func TestPowerOn(t *testing.T) {
 	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
 	cases := []struct {
 		name   string
+		force  bool
 		ironic *testserver.IronicMock
 
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
+		expectedErrorResult  bool
 	}{
 		{
 			name: "node-already-power-on",
@@ -75,6 +77,30 @@ func TestPowerOn(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
+		{
+			name: "power-on with LastError",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				PowerState:       powerOff,
+				TargetPowerState: powerOff,
+				UUID:             nodeUUID,
+				LastError:        "power on failed",
+			}),
+			expectedRequestAfter: 0,
+			expectedDirty:        false,
+			expectedErrorResult:  true,
+		},
+		{
+			name:  "power-on with LastError",
+			force: true,
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				PowerState:       powerOff,
+				TargetPowerState: powerOff,
+				UUID:             nodeUUID,
+				LastError:        "power on failed",
+			}),
+			expectedError:       true,
+			expectedErrorResult: false,
+		},
 	}
 
 	for _, tc := range cases {
@@ -101,7 +127,7 @@ func TestPowerOn(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, err := prov.PowerOn()
+			result, err := prov.PowerOn(tc.force)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
@@ -110,6 +136,9 @@ func TestPowerOn(t *testing.T) {
 			} else {
 				assert.Error(t, err)
 			}
+			if tc.expectedErrorResult {
+				assert.Contains(t, result.ErrorMessage, "PowerOn operation failed")
+			}
 		})
 	}
 }
@@ -120,10 +149,12 @@ func TestPowerOff(t *testing.T) {
 	cases := []struct {
 		name   string
 		ironic *testserver.IronicMock
+		force  bool
 
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
+		expectedErrorResult  bool
 		rebootMode           metal3v1alpha1.RebootMode
 	}{
 		{
@@ -187,6 +218,44 @@ func TestPowerOff(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
+		{
+			name: "power-off soft with force",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
+				TargetProvisionState: "",
+				UUID:                 nodeUUID,
+			}),
+			rebootMode:    metal3v1alpha1.RebootModeSoft,
+			force:         true,
+			expectedDirty: true,
+		},
+		{
+			name: "power-off hard with LastError",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
+				TargetProvisionState: "",
+				UUID:                 nodeUUID,
+				LastError:            "hard power off failed",
+			}),
+			rebootMode:          metal3v1alpha1.RebootModeHard,
+			expectedDirty:       false,
+			expectedErrorResult: true,
+		},
+		{
+			name: "power-off hard with force",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
+				TargetProvisionState: "",
+				UUID:                 nodeUUID,
+				LastError:            "hard power off failed",
+			}),
+			rebootMode:    metal3v1alpha1.RebootModeHard,
+			force:         true,
+			expectedDirty: true,
+		},
 	}
 
 	for _, tc := range cases {
@@ -214,7 +283,7 @@ func TestPowerOff(t *testing.T) {
 			}
 
 			// We pass the RebootMode type here to define the reboot action
-			result, err := prov.PowerOff(tc.rebootMode)
+			result, err := prov.PowerOff(tc.rebootMode, tc.force)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
@@ -223,6 +292,10 @@ func TestPowerOff(t *testing.T) {
 			} else {
 				assert.Error(t, err)
 			}
+			if tc.expectedErrorResult {
+				assert.Contains(t, result.ErrorMessage, "hard power off failed")
+			}
+
 		})
 	}
 }