Improve Day 2 retry logic for hardware configuration

Enable retry of hardware configuration operations after timeouts or
failures by allowing spec changes to trigger new configuration attempts.

Key changes:
- Add webhook validation to prevent spec updates for Day 0 provisioning
  timeouts/failures (requires delete and recreate)
- Track ObservedConfigTransactionId to detect configuration spec changes
- Skip timeout checking when ConfigTransactionId changes (indicates retry)
- Add waitingForConfigStart logic to handle new configuration attempts
- Update shouldUpdateHardwareStatus to properly handle terminal states
- Add Day 2 retry test scenarios

Assisted-by: Cursor/claude-4-sonnet
Signed-off-by: Tao Liu <[email protected]>

Author: tliu2021

api/provisioning/v1alpha1/provisioningrequest_webhook.go

@@ -9,6 +9,7 @@ package v1alpha1
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/google/uuid"
@@ -139,6 +140,22 @@ func (v *provisioningRequestValidator) validateCreateOrUpdate(ctx context.Contex
 		return nil
 	}
 
+	// Check if hardware provisioning has timed out or failed
+	// If so, reject any spec updates - user must delete and recreate the PR
+	hwProvisionedCond := meta.FindStatusCondition(
+		newPr.Status.Conditions, string(PRconditionTypes.HardwareProvisioned))
+	if hwProvisionedCond != nil &&
+		hwProvisionedCond.Status == "False" &&
+		(hwProvisionedCond.Reason == string(CRconditionReasons.TimedOut) ||
+			hwProvisionedCond.Reason == string(CRconditionReasons.Failed)) {
+		// Compare specs to see if there's an actual spec change
+		if !reflect.DeepEqual(oldPr.Spec, newPr.Spec) {
+			return fmt.Errorf("hardware provisioning has timed out or failed. " +
+				"Spec changes are not allowed. " +
+				"Please delete and recreate the ProvisioningRequest to retry")
+		}
+	}
+
 	crProvisionedCond := meta.FindStatusCondition(
 		newPr.Status.Conditions, string(PRconditionTypes.ClusterProvisioned))
 	if crProvisionedCond == nil ||

hwmgr-plugins/controller/utils/node_allocation_request.go

@@ -201,6 +201,11 @@ func UpdateNodeAllocationRequestStatusCondition(
 			conditionStatus,
 			message)
 
+		// Update the observed config transaction id if the condition is Configured
+		if conditionType == hwmgmtv1alpha1.Configured {
+			newNodeAllocationRequest.Status.ObservedConfigTransactionId = nodeAllocationRequest.Spec.ConfigTransactionId
+		}
+
 		// Copy both start time fields from the local object to persist them
 		newNodeAllocationRequest.Status.ProvisioningStartTime = nodeAllocationRequest.Status.ProvisioningStartTime
 		newNodeAllocationRequest.Status.ConfiguringStartTime = nodeAllocationRequest.Status.ConfiguringStartTime

hwmgr-plugins/metal3/controller/metal3_nodeallocationrequest_controller.go

@@ -684,6 +684,13 @@ func (r *NodeAllocationRequestReconciler) handleNodeAllocationRequestSpecChanged
 		nodeAllocationRequest.Status.Conditions,
 		string(hwmgmtv1alpha1.Configured))
 
+	if configuredCondition != nil {
+		if configuredCondition.Reason == string(hwmgmtv1alpha1.TimedOut) ||
+			configuredCondition.Reason == string(hwmgmtv1alpha1.Failed) {
+			r.Logger.InfoContext(ctx, "NodeAllocationRequest configuration is in terminal state, but config change detected - allowing retry",
+				slog.String("reason", configuredCondition.Reason))
+		}
+	}
 	// Set a default status that will be updated during the configuration process
 	if configuredCondition == nil || configuredCondition.Status == metav1.ConditionTrue {
 		if result, err := setAwaitConfigCondition(ctx, r.Client, nodeAllocationRequest); err != nil {
@@ -859,6 +866,19 @@ func (r *NodeAllocationRequestReconciler) checkHardwareTimeout(
 
 	// If provisioning is complete (or not in-progress) and configuring is in-progress, use ConfiguringStartTime.
 	if inProgress(cfg) {
+		// For Day 2 retry: if there's a spec change (ConfigTransactionId mismatch),
+		// skip timeout checking as this is a new configuration attempt
+		// Note: ObservedConfigTransactionId is 0 when not set, so we check for 0 or mismatch
+		if nar.Spec.ConfigTransactionId != 0 &&
+			(nar.Status.ObservedConfigTransactionId == 0 ||
+				nar.Status.ObservedConfigTransactionId != nar.Spec.ConfigTransactionId) {
+			r.Logger.InfoContext(context.Background(), "Configuration spec change detected, skipping timeout check for retry",
+				slog.String("nar", nar.Name),
+				slog.Int64("specConfigTransactionId", nar.Spec.ConfigTransactionId),
+				slog.Int64("observedConfigTransactionId", nar.Status.ObservedConfigTransactionId))
+			return false, hwmgmtv1alpha1.ConditionType(""), nil
+		}
+
 		if nar.Status.ConfiguringStartTime == nil || nar.Status.ConfiguringStartTime.Time.IsZero() {
 			// Inconsistent state: configuring says in-progress but no start time.
 			r.Logger.WarnContext(context.Background(), "Configuration in progress but no start time set",

hwmgr-plugins/metal3/controller/metal3_nodeallocationrequest_controller_test.go

@@ -286,4 +286,92 @@ var _ = Describe("Metal3 NodeAllocationRequest Controller Timeout Handling", fun
 			})
 		})
 	})
+
+	Describe("Day 2 retry scenarios", func() {
+		var nar *pluginsv1alpha1.NodeAllocationRequest
+
+		BeforeEach(func() {
+			nar = &pluginsv1alpha1.NodeAllocationRequest{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-nar-day2",
+					Namespace: "default",
+				},
+				Spec: pluginsv1alpha1.NodeAllocationRequestSpec{
+					HardwareProvisioningTimeout: "5m",
+					ConfigTransactionId:         2, // Indicates spec change
+				},
+				Status: pluginsv1alpha1.NodeAllocationRequestStatus{
+					Conditions: []metav1.Condition{},
+				},
+			}
+		})
+
+		Context("when configuration failed and spec changed", func() {
+			BeforeEach(func() {
+				// Set provisioning as completed
+				hwmgrutils.SetStatusCondition(&nar.Status.Conditions,
+					string(hwmgmtv1alpha1.Provisioned),
+					string(hwmgmtv1alpha1.Completed),
+					metav1.ConditionTrue,
+					"Hardware provisioning completed")
+
+				// Set configuration as failed
+				hwmgrutils.SetStatusCondition(&nar.Status.Conditions,
+					string(hwmgmtv1alpha1.Configured),
+					string(hwmgmtv1alpha1.Failed),
+					metav1.ConditionFalse,
+					"Hardware configuration failed")
+
+				// Set configuration start time to old (exceeded timeout) - this should be ignored when spec changes
+				startTime := metav1.Time{Time: time.Now().Add(-10 * time.Minute)}
+				nar.Status.ConfiguringStartTime = &startTime
+
+				// Set ObservedConfigTransactionId to 1, but Spec.ConfigTransactionId is 2 (mismatch = spec change)
+				nar.Status.ObservedConfigTransactionId = 1
+			})
+
+			It("should allow retry when spec changes", func() {
+				// The Metal3 controller should detect the spec change and skip timeout checking
+				// This allows retry even when the previous configuration failed/timed out
+				timeoutExceeded, conditionType, err := reconciler.checkHardwareTimeout(nar)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(timeoutExceeded).To(BeFalse())
+				Expect(conditionType).To(Equal(hwmgmtv1alpha1.ConditionType("")))
+			})
+		})
+
+		Context("when configuration timed out and spec changed", func() {
+			BeforeEach(func() {
+				// Set provisioning as completed
+				hwmgrutils.SetStatusCondition(&nar.Status.Conditions,
+					string(hwmgmtv1alpha1.Provisioned),
+					string(hwmgmtv1alpha1.Completed),
+					metav1.ConditionTrue,
+					"Hardware provisioning completed")
+
+				// Set configuration as timed out
+				hwmgrutils.SetStatusCondition(&nar.Status.Conditions,
+					string(hwmgmtv1alpha1.Configured),
+					string(hwmgmtv1alpha1.TimedOut),
+					metav1.ConditionFalse,
+					"Hardware configuration timed out")
+
+				// Set configuration start time to old (exceeded timeout) - this should be ignored when spec changes
+				startTime := metav1.Time{Time: time.Now().Add(-10 * time.Minute)}
+				nar.Status.ConfiguringStartTime = &startTime
+
+				// Set ObservedConfigTransactionId to 1, but Spec.ConfigTransactionId is 2 (mismatch = spec change)
+				nar.Status.ObservedConfigTransactionId = 1
+			})
+
+			It("should allow retry when spec changes", func() {
+				// Similar to failed case, should allow retry with spec change
+				// Timeout check should be skipped when spec changes
+				timeoutExceeded, conditionType, err := reconciler.checkHardwareTimeout(nar)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(timeoutExceeded).To(BeFalse())
+				Expect(conditionType).To(Equal(hwmgmtv1alpha1.ConditionType("")))
+			})
+		})
+	})
 })

internal/controllers/provisioningrequest_hwprovision.go

@@ -548,8 +548,21 @@ func (t *provisioningRequestReconcilerTask) updateHardwareStatus(
 		}
 	}
 
+	// Check if we're waiting for a new configuration to start (only when condition doesn't exist)
+	waitingForConfigStart := condition == hwmgmtv1alpha1.Configured &&
+		hwCondition == nil &&
+		(nodeAllocationRequest.Status.ObservedConfigTransactionId == nil ||
+			*nodeAllocationRequest.Status.ObservedConfigTransactionId == 0 ||
+			*nodeAllocationRequest.Status.ObservedConfigTransactionId != t.object.Generation)
+
 	if hwCondition == nil {
-		// Condition does not exist
+		// Condition does not exist in plugin response
+		if waitingForConfigStart {
+			// We're waiting for a new configuration to start - return ConditionDoesNotExistsErr
+			// to indicate that configuration hasn't started yet for this transaction
+			return false, false, &ctlrutils.ConditionDoesNotExistsErr{ConditionName: string(condition)}
+		}
+		// Condition doesn't exist and we're not waiting for config start
 		status = metav1.ConditionFalse
 		reason = string(provisioningv1alpha1.CRconditionReasons.InProgress)
 		message = fmt.Sprintf("Hardware %s is in progress", ctlrutils.GetStatusMessage(condition))
@@ -561,7 +574,8 @@ func (t *provisioningRequestReconcilerTask) updateHardwareStatus(
 		}
 		ctlrutils.SetProvisioningStateInProgress(t.object, message)
 	} else {
-		// A hardware condition was found and should be processed; use its details.
+		// A hardware condition was found in plugin response - always process it
+		// (even if we're waiting for config start, the plugin has provided valid state)
 		status, reason, message, timedOutOrFailed = t.processExistingHardwareCondition(hwCondition, condition)
 	}
 
@@ -624,31 +638,50 @@ func (t *provisioningRequestReconcilerTask) isCallbackTriggeredReconciliation()
 	return true
 }
 
-// shouldUpdateHardwareStatus determines if we should update hardware status based on callback
-func (t *provisioningRequestReconcilerTask) shouldUpdateHardwareStatus(condition hwmgmtv1alpha1.ConditionType) bool {
-	// Always update on callback-triggered reconciliation
+// shouldUpdateHardwareStatus decides whether to update hardware status during reconciliation.
+// Rules:
+// - Always update on callback-triggered reconciliations.
+// - During polling (non-callback), for Provisioned/Configured:
+//   - If no prior condition exists, update.
+//   - If prior condition is terminal (TimedOut/Failed), do not update.
+//   - Otherwise, update only if prior Status != True.
+func (t *provisioningRequestReconcilerTask) shouldUpdateHardwareStatus(
+	condition hwmgmtv1alpha1.ConditionType,
+) bool {
+	// Callbacks always allowed to update.
 	if t.isCallbackTriggeredReconciliation() {
 		return true
 	}
 
-	// For non-callback reconciliation, check different conditions
-	switch condition {
-	case hwmgmtv1alpha1.Provisioned:
-		hwProvisionedCond := meta.FindStatusCondition(t.object.Status.Conditions, string(provisioningv1alpha1.PRconditionTypes.HardwareProvisioned))
-		return hwProvisionedCond == nil // Only update if no status set yet
-	case hwmgmtv1alpha1.Configured:
-		hwConfiguredCond := meta.FindStatusCondition(t.object.Status.Conditions, string(provisioningv1alpha1.PRconditionTypes.HardwareConfigured))
+	// Map HW condition → PR condition type stored in Status.Conditions.
+	prTypeByHW := map[hwmgmtv1alpha1.ConditionType]string{
+		hwmgmtv1alpha1.Provisioned: string(provisioningv1alpha1.PRconditionTypes.HardwareProvisioned),
+		hwmgmtv1alpha1.Configured:  string(provisioningv1alpha1.PRconditionTypes.HardwareConfigured),
+	}
 
-		// If no status exists yet, allow update
-		if hwConfiguredCond == nil {
-			return true
-		}
+	prCondType, ok := prTypeByHW[condition]
+	if !ok {
+		// Unknown/non-actionable condition types: do not update during polling.
+		return false
+	}
+
+	hwCond := meta.FindStatusCondition(t.object.Status.Conditions, prCondType)
+	if hwCond == nil {
+		// No status yet → allow first write.
+		return true
+	}
 
-		// Allow updates for in-progress states (not completed)
-		return hwConfiguredCond.Status != metav1.ConditionTrue
+	if isTerminalReason(hwCond.Reason) {
+		return false
 	}
 
-	return false
+	// Update only if not already True (i.e., False or Unknown).
+	return hwCond.Status != metav1.ConditionTrue
+}
+
+func isTerminalReason(reason string) bool {
+	return reason == string(provisioningv1alpha1.CRconditionReasons.TimedOut) ||
+		reason == string(provisioningv1alpha1.CRconditionReasons.Failed)
 }
 
 // checkExistingNodeAllocationRequest checks for an existing NodeAllocationRequest and verifies changes if necessary
@@ -730,6 +763,7 @@ func (t *provisioningRequestReconcilerTask) buildNodeAllocationRequest(clusterIn
 	nodeAllocationRequest.ClusterId = clusterId.(string)
 	nodeAllocationRequest.NodeGroup = nodeGroups
 	nodeAllocationRequest.BootInterfaceLabel = hwTemplate.Spec.BootInterfaceLabel
+	nodeAllocationRequest.ConfigTransactionId = t.object.Generation
 
 	// Set HardwareProvisioningTimeout - use template value or default
 	timeoutStr := hwTemplate.Spec.HardwareProvisioningTimeout