Repository: metal3-io/baremetal-operator
Pull request #1154 (2022-07-29 13:25:36)

Handle maintenance more gracefully
First, do not set the maintenance flag for nodes in manageable state,
it's not required.
    
Second, if the maintenance flag is on when we don't expect it and Ironic
does not report a fault, move the nodes off maintenance.

Finally, remove stale instanceUUID before deletion if it's present.



Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index aefc8419..9c575752 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -666,6 +666,18 @@ func (p *ironicProvisioner) tryChangeNodeProvisionState(ironicNode *nodes.Node,
 		"new target", opts.Target,
 	)
 
+	// Changing provision state in maintenance mode will not work.
+	if ironicNode.Fault != "" {
+		p.log.Info("node has a fault, will retry", "fault", ironicNode.Fault, "reason", ironicNode.MaintenanceReason)
+		result, err = retryAfterDelay(provisionRequeueDelay)
+		return
+	}
+	if ironicNode.Maintenance {
+		p.log.Info("trying to change a provision state for a node in maintenance, removing maintenance first", "reason", ironicNode.MaintenanceReason)
+		result, err = p.setMaintenanceFlag(ironicNode, false)
+		return
+	}
+
 	changeResult := nodes.ChangeProvisionState(p.client, ironicNode.UUID, opts)
 	switch changeResult.Err.(type) {
 	case nil:
@@ -1133,6 +1145,11 @@ func (p *ironicProvisioner) Adopt(data provisioner.AdoptData, force bool) (resul
 		return operationFailed(fmt.Sprintf("Host adoption failed: %s",
 			ironicNode.LastError))
 	case nodes.Active:
+		// Empty Fault means that maintenance was set manually, not by Ironic
+		if ironicNode.Maintenance && ironicNode.Fault == "" && data.State != metal3v1alpha1.StateDeleting {
+			p.log.Info("active node was found to be in maintenance, updating", "state", data.State)
+			return p.setMaintenanceFlag(ironicNode, false)
+		}
 	default:
 	}
 	return operationComplete()
@@ -1665,15 +1682,19 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) {
 		"deploy step", ironicNode.DeployStep,
 	)
 
-	if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.Available {
-		// Move back to manageable so we can delete it cleanly.
-		return p.changeNodeProvisionState(
-			ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
-		)
-	}
-
-	if !ironicNode.Maintenance {
+	currentProvState := nodes.ProvisionState(ironicNode.ProvisionState)
+	if currentProvState == nodes.Available || currentProvState == nodes.Manageable {
+		// Make sure we don't have a stale instance UUID
+		if ironicNode.InstanceUUID != "" {
+			p.log.Info("removing stale instance UUID before deletion", "instanceUUID", ironicNode.InstanceUUID)
+			updater := updateOptsBuilder(p.debugLog)
+			updater.SetTopLevelOpt("instance_uuid", nil, ironicNode.InstanceUUID)
+			success, result, err := p.tryUpdateNode(ironicNode, updater)
+			if !success {
+				return result, err
+			}
+		}
+	} else if !ironicNode.Maintenance {
 		// If we see an active node and the controller doesn't think
 		// we need to deprovision it, that means the node was
 		// ExternallyProvisioned and we should remove it from Ironic


Test patch:
diff --git a/pkg/provisioner/ironic/adopt_test.go b/pkg/provisioner/ironic/adopt_test.go
index fbd8334f..53716c9b 100644
--- a/pkg/provisioner/ironic/adopt_test.go
+++ b/pkg/provisioner/ironic/adopt_test.go
@@ -87,6 +87,38 @@ func TestAdopt(t *testing.T) {
 			expectedRequestAfter: 10,
 			force:                true,
 		},
+		{
+			name: "node-in-Active",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				ProvisionState: string(nodes.Active),
+				UUID:           nodeUUID,
+			}),
+
+			expectedDirty: false,
+		},
+		{
+			name: "node-in-Maintenance",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				ProvisionState: string(nodes.Active),
+				UUID:           nodeUUID,
+				Maintenance:    true,
+			}),
+
+			expectedDirty:        false,
+			expectedRequestAfter: 0,
+			expectedError:        true,
+		},
+		{
+			name: "node-in-Fault",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				ProvisionState: string(nodes.Active),
+				UUID:           nodeUUID,
+				Maintenance:    true,
+				Fault:          "power fault",
+			}),
+
+			expectedDirty: false,
+		},
 	}
 
 	for _, tc := range cases {
diff --git a/pkg/provisioner/ironic/delete_test.go b/pkg/provisioner/ironic/delete_test.go
index 1475bec9..3c7eba10 100644
--- a/pkg/provisioner/ironic/delete_test.go
+++ b/pkg/provisioner/ironic/delete_test.go
@@ -101,14 +101,33 @@ func deleteTest(t *testing.T, detach bool) {
 		},
 		{
 			name: "available-node",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(
+			ironic: testserver.NewIronic(t).Node(
 				nodes.Node{
 					UUID:           nodeUUID,
 					ProvisionState: "available",
+					Maintenance:    false,
 				},
-			),
+			).Delete(nodeUUID),
 			expectedDirty:        true,
-			expectedRequestAfter: provisionRequeueDelay,
+			expectedRequestAfter: 0,
+		},
+		{
+			name: "stale-instance-uuid-update",
+			ironic: testserver.NewIronic(t).Node(
+				nodes.Node{
+					UUID:           nodeUUID,
+					ProvisionState: "available",
+					InstanceUUID:   nodeUUID,
+				},
+			).NodeUpdate(nodes.Node{
+				UUID: nodeUUID,
+			}).Delete(nodeUUID),
+			expectedDirty:        true,
+			expectedRequestAfter: 0,
+			expectedUpdate: &nodes.UpdateOperation{
+				Op:   nodes.RemoveOp,
+				Path: "/instance_uuid",
+			},
 		},
 		{
 			name: "not-in-maintenance-update-fail",
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index 9330510c..13c63ab9 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -88,6 +88,27 @@ func TestProvision(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
+		{
+			name: "fault state",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.Manageable),
+				UUID:           nodeUUID,
+				Fault:          "power fault",
+				Maintenance:    true,
+			}),
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
+		},
+		{
+			name: "maintenance mode",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.Manageable),
+				UUID:           nodeUUID,
+				Maintenance:    true,
+			}),
+			expectedRequestAfter: 0,
+			expectedDirty:        true,
+		},
 	}
 
 	for _, tc := range cases {