Repository: metal3-io/baremetal-operator
Pull request #1055 (2021-12-17 08:56:30)

Update error message when soft poweroff fails
This PR updates the error message in case of soft poweroff failure. 
Currently, the workflow when turning off a machine, BMO tries soft poweroff first. If failed, it performs a hard poweroff. The problem is that after soft poweroff fails, BMO would publish an error message got from ironic conductor, something like: `Failed to change power state to 'power off' by 'soft power off'. Error: Redfish exception occurred. Error: Setting power state to soft power off failed for node <node id>...`. This message can make user confused and think that this is a serious error and it can interrupt the poweroff workflow. 
This PR aims to clarify a bit more about the workflow by informing user about the followed hard poweroff after the soft one failure. 

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **maelk** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @maelk` 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":["maelk"]} -->
metal3-io-bot: @namnx228: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | 2e8b13c866a93b2e14a0236e89fbb0fb41f31c6d | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1055/unit/1471766249204617216) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1055). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:namnx228).

<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 -->
metal3-io-bot: @namnx228: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | 1e42fd2cecb1d4ab217efbbeee9ca81610914015 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1055/unit/1471766857122844672) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1055). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:namnx228).

<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 -->
namnx228: /test-v1b1-centos-integration
/test-v1b1-integration
namnx228: /assign @fmuyassarov 
/cc @kashifest 
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **fmuyassarov** after the PR has been reviewed.

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":["fmuyassarov"]} -->
namnx228: /test-v1b1-centos-integration
/test-v1b1-integration
namnx228: /cc @dtantsur 
metal3-io-bot: @namnx228: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
golint | f320946dac9e0c6f5b8db697b6fd5c484504255e | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1055/golint/1472905429703987200) | true | `/test golint`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1055). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:namnx228).

<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 -->
kashifest: /hold cancel
The switch is done. BMO's default branch is now `main`
kashifest: /hold cancel
The switch is done. BMO's default branch is now `main`
furkatgofurov7: /test golint
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **fmuyassarov** after the PR has been reviewed.

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/main/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":["fmuyassarov"]} -->
namnx228: /test-v1b1-centos-integration
/test-v1b1-integration
namnx228: /test-v1b1-centos-integration
/test-v1b1-integration
namnx228: /test-v1b1-centos-integration

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index f06e1c68..870f3713 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -59,6 +59,7 @@ const (
 	rebootAnnotationPrefix        = "reboot.metal3.io"
 	inspectAnnotationPrefix       = "inspect.metal3.io"
 	hardwareDetailsAnnotation     = inspectAnnotationPrefix + "/hardwaredetails"
+	clarifySoftPoweroffFailure    = "Continuing with hard poweroff after soft poweroff fails. More details: "
 )
 
 // BareMetalHostReconciler reconciles a BareMetalHost object
@@ -1218,6 +1219,10 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	}
 
 	if provResult.ErrorMessage != "" {
+		if !desiredPowerOnState && desiredRebootMode == metal3v1alpha1.RebootModeSoft &&
+			info.host.Status.ErrorType != metal3v1alpha1.PowerManagementError {
+			provResult.ErrorMessage = clarifySoftPoweroffFailure + provResult.ErrorMessage
+		}
 		return recordActionFailure(info, metal3v1alpha1.PowerManagementError, provResult.ErrorMessage)
 	}
 


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 050ab897..fde1bd08 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -874,61 +874,84 @@ func TestProvisioningCancelled(t *testing.T) {
 }
 
 func TestErrorCountIncreasedOnActionFailure(t *testing.T) {
-
+	defaultError := "some error"
+	poweroffError := "some details"
 	tests := []struct {
 		Scenario           string
 		Host               *metal3v1alpha1.BareMetalHost
 		ProvisionerErrorOn string
+		originalError      string
+		ExpectedError      string
 	}{
 		{
 			Scenario:           "registration",
 			Host:               host(metal3v1alpha1.StateRegistering).build(),
 			ProvisionerErrorOn: "ValidateManagementAccess",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "inspecting",
 			Host:               host(metal3v1alpha1.StateInspecting).build(),
 			ProvisionerErrorOn: "InspectHardware",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "provisioning",
 			Host:               host(metal3v1alpha1.StateProvisioning).SetImageURL("imageSpecUrl").build(),
 			ProvisionerErrorOn: "Provision",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "deprovisioning",
 			Host:               host(metal3v1alpha1.StateDeprovisioning).build(),
 			ProvisionerErrorOn: "Deprovision",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "available-power-on",
 			Host:               host(metal3v1alpha1.StateAvailable).SetImageURL("").SetStatusPoweredOn(false).build(),
 			ProvisionerErrorOn: "PowerOn",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "available-power-off",
 			Host:               host(metal3v1alpha1.StateAvailable).SetImageURL("").SetOnline(false).build(),
 			ProvisionerErrorOn: "PowerOff",
+			originalError:      poweroffError,
+			ExpectedError:      clarifySoftPoweroffFailure + poweroffError,
 		},
 		{
 			Scenario:           "deprecated-ready-power-on",
 			Host:               host(metal3v1alpha1.StateReady).SetImageURL("").SetStatusPoweredOn(false).build(),
 			ProvisionerErrorOn: "PowerOn",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "deprecated-ready-power-off",
 			Host:               host(metal3v1alpha1.StateReady).SetImageURL("").SetOnline(false).build(),
 			ProvisionerErrorOn: "PowerOff",
+			originalError:      poweroffError,
+			ExpectedError:      clarifySoftPoweroffFailure + poweroffError,
 		},
 		{
 			Scenario:           "externally-provisioned-adopt-failed",
 			Host:               host(metal3v1alpha1.StateExternallyProvisioned).SetExternallyProvisioned().build(),
 			ProvisionerErrorOn: "Adopt",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 		{
 			Scenario:           "provisioned-adopt-failed",
 			Host:               host(metal3v1alpha1.StateProvisioned).build(),
 			ProvisionerErrorOn: "Adopt",
+			originalError:      defaultError,
+			ExpectedError:      defaultError,
 		},
 	}
 	for _, tt := range tests {
@@ -938,11 +961,11 @@ func TestErrorCountIncreasedOnActionFailure(t *testing.T) {
 			hsm := newHostStateMachine(tt.Host, reconciler, prov, true)
 			info := makeDefaultReconcileInfo(tt.Host)
 
-			prov.setNextError(tt.ProvisionerErrorOn, "some error")
+			prov.setNextError(tt.ProvisionerErrorOn, tt.originalError)
 			result := hsm.ReconcileState(info)
 
 			assert.Equal(t, 1, tt.Host.Status.ErrorCount)
-			assert.Equal(t, "some error", tt.Host.Status.ErrorMessage)
+			assert.Equal(t, tt.ExpectedError, tt.Host.Status.ErrorMessage)
 			assert.True(t, result.Dirty())
 		})
 	}