Repository: metal3-io/baremetal-operator
Pull request #785 (2021-02-02 11:52:49)

Wait for inspection completion
The previous logic for `Provisioner.InspectHardware()` considered the action completed by just looking at the Inspector result flags `Finished` and `Error`. Anyhow, even though the inspection was effectively completed, there could be some cases where the related node `Provisioning` state was still in `inspectWait` since the update in Ironic is not simultaneous.
This could be a problem since the BMO state machine goes on and some BMO states could report an error if the node is not in the expected provisioning state (as the new `Preparing` state introduced in #763).

This PR so waits also for the node to _not be_ in `inspectWait` before considering the inspection successfully done.

/cc @dtantsur @zaneb @Hellcatlk 


Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 5fc067de..d38b56fe 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -696,16 +696,16 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu
 		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, err = operationContinuing(introspectionRequeueDelay)
-		return
-	}
 	if status.Error != "" {
 		p.log.Info("inspection failed", "error", status.Error)
 		result, err = operationFailed(status.Error)
 		return
 	}
+	if !status.Finished || nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectWait {
+		p.log.Info("inspection in progress", "started_at", status.StartedAt)
+		result, err = operationContinuing(introspectionRequeueDelay)
+		return
+	}
 
 	// Introspection is done
 	p.log.Info("getting hardware details from inspection")


Test patch:
diff --git a/pkg/provisioner/ironic/inspecthardware_test.go b/pkg/provisioner/ironic/inspecthardware_test.go
index 7118cb7c..5cacdf4e 100644
--- a/pkg/provisioner/ironic/inspecthardware_test.go
+++ b/pkg/provisioner/ironic/inspecthardware_test.go
@@ -99,8 +99,11 @@ func TestInspectHardware(t *testing.T) {
 			expectedResultError: "Canceled by operator",
 		},
 		{
-			name:   "inspection-in-progress",
-			ironic: testserver.NewIronic(t).WithDefaultResponses(),
+			name: "inspection-in-progress (not yet finished)",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.Manageable),
+			}),
 			inspector: testserver.NewInspector(t).Ready().WithIntrospection(nodeUUID, introspection.Introspection{
 				Finished: false,
 			}),
@@ -108,8 +111,30 @@ func TestInspectHardware(t *testing.T) {
 			expectedRequestAfter: 15,
 		},
 		{
-			name:   "inspection-completed",
-			ironic: testserver.NewIronic(t).WithDefaultResponses(),
+			name: "inspection-in-progress (but node still in InspectWait)",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.InspectWait),
+			}),
+			inspector: testserver.NewInspector(t).Ready().
+				WithIntrospection(nodeUUID, introspection.Introspection{
+					Finished: true,
+				}).
+				WithIntrospectionData(nodeUUID, introspection.Data{
+					Inventory: introspection.InventoryType{
+						Hostname: "node-0",
+					},
+				}),
+
+			expectedDirty:        true,
+			expectedRequestAfter: 15,
+		},
+		{
+			name: "inspection-complete",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				UUID:           nodeUUID,
+				ProvisionState: string(nodes.Manageable),
+			}),
 			inspector: testserver.NewInspector(t).Ready().
 				WithIntrospection(nodeUUID, introspection.Introspection{
 					Finished: true,