Repository: metal3-io/baremetal-operator
Pull request #840 (2021-03-30 16:55:29)

Don't immediately retry on Inspect fail
Ensure that if the ironic node enters the InspectFail state, we not only report the error but also wait until after the exponential backoff before retrying, instead of retrying immediately.

The previous attempt to fix this in 2d02462fb45dbb5dd9dd0189ce91e0c417bbab2e didn't take due to a missing return.

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/840#" title="Author self-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":[]} -->
s3rj1k: /approve
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/840#issuecomment-810463567" title="Approved">s3rj1k</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/840#" title="Author self-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":[]} -->

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 0570fb18..ab78c828 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -673,16 +673,18 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu
 				p.log.Info("inspection already started")
 				result, err = operationContinuing(introspectionRequeueDelay)
 				return
-			default:
-				if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectFail && !force {
+			case nodes.InspectFail:
+				if !force {
 					p.log.Info("starting inspection failed", "error", status.Error)
-					if ironicNode.LastError == "" {
-						result.ErrorMessage = "Inspection failed"
-					} else {
-						result.ErrorMessage = ironicNode.LastError
+					failure := ironicNode.LastError
+					if failure == "" {
+						failure = "Inspection failed"
 					}
-					err = nil
+					result, err = operationFailed(failure)
+					return
 				}
+				fallthrough
+			default:
 				p.log.Info("updating boot mode before hardware inspection")
 				op, value := buildCapabilitiesValue(ironicNode, p.host.Status.Provisioning.BootMode)
 				updates := nodes.UpdateOpts{


Test patch:
diff --git a/pkg/provisioner/ironic/inspecthardware_test.go b/pkg/provisioner/ironic/inspecthardware_test.go
index 5cacdf4e..ae7ab827 100644
--- a/pkg/provisioner/ironic/inspecthardware_test.go
+++ b/pkg/provisioner/ironic/inspecthardware_test.go
@@ -23,6 +23,8 @@ func TestInspectHardware(t *testing.T) {
 		ironic    *testserver.IronicMock
 		inspector *testserver.InspectorMock
 
+		force bool
+
 		expectedDirty        bool
 		expectedRequestAfter int
 		expectedResultError  string
@@ -129,6 +131,32 @@ func TestInspectHardware(t *testing.T) {
 			expectedDirty:        true,
 			expectedRequestAfter: 15,
 		},
+		{
+			name: "inspection-failed",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.InspectFail),
+			}),
+			inspector: testserver.NewInspector(t).Ready().WithIntrospectionFailed(nodeUUID, http.StatusNotFound),
+
+			expectedResultError: "Inspection failed",
+		},
+		{
+			name: "inspection-failed force",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.InspectFail),
+			}).NodeUpdate(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.InspectFail),
+			}).WithNodeStatesProvisionUpdate(nodeUUID),
+			inspector: testserver.NewInspector(t).Ready().WithIntrospectionFailed(nodeUUID, http.StatusNotFound),
+			force:     true,
+
+			expectedDirty:        true,
+			expectedRequestAfter: 10,
+			expectedPublish:      "InspectionStarted Hardware inspection started",
+		},
 		{
 			name: "inspection-complete",
 			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
@@ -177,7 +205,7 @@ func TestInspectHardware(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, details, err := prov.InspectHardware(false)
+			result, details, err := prov.InspectHardware(tc.force)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)