OCPBUGS-74261: Fix NNCP MaxUnavailableLimitReached deadlock with unavailable-slot audit

api/shared/nodenetworkconfigurationenactment_types.go

@@ -83,6 +83,13 @@ const (
 	NodeNetworkConfigurationEnactmentConditionMaxUnavailableLimitReached ConditionReason = "MaxUnavailableLimitReached"
 	NodeNetworkConfigurationEnactmentConditionConfigurationProgressing   ConditionReason = "ConfigurationProgressing"
 	NodeNetworkConfigurationEnactmentConditionConfigurationAborted       ConditionReason = "ConfigurationAborted"
+	NodeNetworkConfigurationEnactmentConditionConfigurationInterrupted   ConditionReason = "ConfigurationInterrupted"
+	// NodeNetworkConfigurationEnactmentConditionConfigurationFinalizing marks the
+	// post-apply phase: the desired state was applied and only releasing the
+	// maxUnavailable slot and recording success remain. A reconcile that finds
+	// the enactment in this phase must not re-apply the (already committed)
+	// configuration.
+	NodeNetworkConfigurationEnactmentConditionConfigurationFinalizing ConditionReason = "ConfigurationFinalizing"
 )
 
 func EnactmentKey(node, policy string) types.NamespacedName {

cmd/handler/main.go

@@ -28,7 +28,6 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -64,11 +63,13 @@ import (
 	controllersmetrics "github.com/nmstate/kubernetes-nmstate/controllers/metrics"
 	"github.com/nmstate/kubernetes-nmstate/pkg/cluster"
 	"github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus"
+	enactmentconditions "github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus/conditions"
 	"github.com/nmstate/kubernetes-nmstate/pkg/environment"
 	"github.com/nmstate/kubernetes-nmstate/pkg/file"
 	nmstatelog "github.com/nmstate/kubernetes-nmstate/pkg/log"
 	"github.com/nmstate/kubernetes-nmstate/pkg/monitoring"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
+	"github.com/nmstate/kubernetes-nmstate/pkg/node"
 	"github.com/nmstate/kubernetes-nmstate/pkg/webhook"
 )
 
@@ -302,15 +303,15 @@ func setupWebhookEnvironment(mgr manager.Manager, tlsOpts func(*tls.Config)) err
 	return nil
 }
 
-// setupHandlerEnvironment cleans up unavailableNodeCounts after unexpected restart,
+// setupHandlerEnvironment reclaims maxUnavailable slots held by this node's
+// enactments interrupted by an unexpected restart (reclaimInterruptedSlots),
 // configures the handler controllers and performs health checks
 func setupHandlerEnvironment(mgr manager.Manager) error {
-	// Clean stale unavailable counts from node before starting controllers
-	// Prevents deadlock after unexpected cluster reboot where nodes were
-	// processing NNCP and left stale counts in etcd.
-	if err := cleanStaleUnavailableCounts(mgr); err != nil {
-		setupLog.Error(err, "Failed to cleanup stale unavailable counts, continuing anyway")
-		// Don't error this is best-effort (NNCP needs manual restart)
+	// Reclaim maxUnavailable slots held by enactments interrupted by an
+	// unexpected handler/cluster restart. Best-effort: the reconciler's
+	// audit-on-block heals the same state if this fails.
+	if err := reclaimInterruptedSlots(mgr); err != nil {
+		setupLog.Error(err, "Failed to reclaim interrupted slots, continuing anyway")
 	}
 
 	if err := setupHandlerControllers(mgr); err != nil {
@@ -333,18 +334,38 @@ func startManager(mgr manager.Manager, ctx context.Context) int {
 	return 0
 }
 
-// cleanStaleUnavailableCounts cleans up stale unavailable node counts that have been
-// left in NNCP status after unexpected handler restarts or cluster reboots.
+// startupListBackoff retries the initial enactment List for ~2 minutes: at
+// handler startup after an ungraceful reboot, the apiserver is frequently
+// not yet ready, and failing silently here would leave ghost slots in
+// place until the audit's staleness threshold elapses.
+var startupListBackoff = wait.Backoff{
+	Duration: 2 * time.Second,
+	Factor:   2.0,
+	Jitter:   0.1,
+	Steps:    7,
+	Cap:      60 * time.Second,
+}
+
+// reclaimInterruptedSlots recovers maxUnavailable slots held by this node's
+// enactments that were still Progressing when the handler last died. At
+// handler startup no applies are in progress for this node, so any own
+// enactment still Progressing is provably dead and is recovered by phase:
+//
+//   - Finalizing (ConfigurationFinalizing): the desired state was already
+//     applied and committed before the crash and the node's networking
+//     persists across a handler restart, so it is finalized without
+//     re-applying: record success, then release the slot with the idempotent
+//     set-to-truth audit.
+//   - Progressing (ConfigurationProgressing): the apply was interrupted
+//     mid-flight, so mark it interrupted (Pending, retry count reset) to be
+//     re-applied and audit the policy's unavailable-node counter.
 //
-// At handler startup, no applies are in progress for this node. Any enactment that
-// is NOT in Available=True state may have a stale UnavailableNodeCountMap entry from
-// a previous interrupted reconcile. We check !IsAvailable rather than IsProgressing
-// because crashes can leave enactments in various non-progressing states (Failing,
-// Pending, empty conditions) that still have stale counts.
-func cleanStaleUnavailableCounts(mgr manager.Manager) error {
+// This is a fast-path optimization: if it fails, the audit-on-block in the
+// NNCP reconciler heals the same state within one staleness threshold.
+func reclaimInterruptedSlots(mgr manager.Manager) error {
 	ctx := context.Background()
 	nodeName := environment.NodeName()
-	setupLog.Info("Cleaning up stale unavailable counts for node", "node", nodeName)
+	setupLog.Info("Reclaiming interrupted unavailable-node slots", "node", nodeName)
 
 	apiClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme()})
 	if err != nil {
@@ -353,92 +374,59 @@ func cleanStaleUnavailableCounts(mgr manager.Manager) error {
 
 	enactmentList := &nmstatev1beta1.NodeNetworkConfigurationEnactmentList{}
 	nodeLabel := client.MatchingLabels{nmstateapi.EnactmentNodeLabel: nodeName}
-	if err := apiClient.List(ctx, enactmentList, nodeLabel); err != nil {
+	if err := retry.OnError(startupListBackoff, func(error) bool { return true }, func() error {
+		return apiClient.List(ctx, enactmentList, nodeLabel)
+	}); err != nil {
 		return err
 	}
 
-	// For each enactment that is not Available (may have a stale count from an interrupted apply)
 	for i := range enactmentList.Items {
 		enactment := &enactmentList.Items[i]
-		if !enactmentstatus.IsAvailable(&enactment.Status.Conditions) {
-			policyName := enactment.Labels[nmstateapi.EnactmentPolicyLabel]
-			if policyName == "" {
-				continue
-			}
-			generationKey := strconv.FormatInt(enactment.Status.PolicyGeneration, 10)
-
-			setupLog.Info("detected stale non-available enactment, cleaning up",
-				"enactment", enactment.Name,
-				"policy", policyName,
-				"generation", generationKey)
-
-			// Decrement counter for this policy and generation
-			if err := decrementStaleUnavailableCount(ctx, apiClient, policyName, generationKey); err != nil {
-				setupLog.Error(err, "Failed to decrement stale count", "policy", policyName)
-				// no return to continue with other enactments
-			}
-
-			// Reset retry count for this enactment and generation
-			if err := resetStaleRetryCount(ctx, apiClient, enactment.Name, generationKey); err != nil {
-				setupLog.Error(err, "Failed to reset stale retry count", "enactment", enactment.Name)
-				// no return to continue with other enactments
-			}
-		}
-	}
-
-	setupLog.Info("Finished cleaning up stale unavailable counts", "node", nodeName)
-	return nil
-}
-
-// decrementStaleUnavailableCount decrements the UnavailableNodeCountMap of a specific
-// policy and generation for startup cleanup.
-func decrementStaleUnavailableCount(ctx context.Context, cli client.Client, policyName, generationKey string) error {
-	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		policy := &nmstatev1.NodeNetworkConfigurationPolicy{}
-		if err := cli.Get(ctx, types.NamespacedName{Name: policyName}, policy); err != nil {
-			if apierrors.IsNotFound(err) {
-				setupLog.Info("Policy not found during stale count cleanup, skipping", "policy", policyName)
-				return nil
-			}
-			return err
+		if !enactmentstatus.IsProgressing(&enactment.Status.Conditions) {
+			continue
 		}
-
-		if policy.Status.UnavailableNodeCountMap == nil {
-			return nil // Nothing to clean up
+		policyName := enactment.Labels[nmstateapi.EnactmentPolicyLabel]
+		if policyName == "" {
+			continue
 		}
-
-		if policy.Status.UnavailableNodeCountMap[generationKey] > 0 {
-			policy.Status.UnavailableNodeCountMap[generationKey]--
-			setupLog.Info("Decremented stale unavailable count",
-				"policy", policyName,
-				"generation", generationKey,
-				"newCount", policy.Status.UnavailableNodeCountMap[generationKey])
-			return cli.Status().Update(ctx, policy)
+		enactmentKey := types.NamespacedName{Name: enactment.Name}
+		policyKey := types.NamespacedName{Name: policyName}
+		generationKey := strconv.FormatInt(enactment.Status.PolicyGeneration, 10)
+
+		if enactmentstatus.IsFinalizing(&enactment.Status.Conditions) {
+			// Apply already committed before the crash: finalize without
+			// re-applying. Record success (moving it out of the live-holder
+			// set) then release the slot with the idempotent audit.
+			setupLog.Info("finalizing post-apply enactment interrupted by restart",
+				"enactment", enactment.Name, "policy", policyName)
+			ec := enactmentconditions.New(apiClient, enactmentKey)
+			if err := ec.NotifySuccess(ctx); err != nil {
+				setupLog.Error(err, "failed recording success for finalizing enactment", "enactment", enactment.Name)
+				continue // still Finalizing (a live holder); audit-on-block will heal
+			}
+			if _, err := node.AuditUnavailableSlots(ctx, apiClient, apiClient,
+				policyKey, node.StaleEnactmentThreshold()); err != nil {
+				setupLog.Error(err, "failed auditing unavailable slots", "policy", policyName)
+			}
+			continue
 		}
 
-		return nil
-	})
-}
+		setupLog.Info("marking interrupted enactment and auditing policy slots",
+			"enactment", enactment.Name, "policy", policyName, "generation", generationKey)
 
-// resetStaleRetryCount resets the RetryCount for a specific enactment and generation
-// during startup clean to prevent stale retry counts from previous interrupted reconciles.
-func resetStaleRetryCount(ctx context.Context, cli client.Client, enactmentName, generationKey string) error {
-	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		enactment := &nmstatev1beta1.NodeNetworkConfigurationEnactment{}
-		if err := cli.Get(ctx, types.NamespacedName{Name: enactmentName}, enactment); err != nil {
-			return err
+		if err := enactmentconditions.MarkInterrupted(ctx, apiClient, enactmentKey, generationKey); err != nil {
+			setupLog.Error(err, "failed marking enactment interrupted", "enactment", enactment.Name)
+			continue // audit would still count it live; skip to avoid double-freeing later
 		}
 
-		if enactment.Status.RetryCount == nil || enactment.Status.RetryCount[generationKey] == 0 {
-			return nil
+		if _, err := node.AuditUnavailableSlots(ctx, apiClient, apiClient,
+			policyKey, node.StaleEnactmentThreshold()); err != nil {
+			setupLog.Error(err, "failed auditing unavailable slots", "policy", policyName)
 		}
+	}
 
-		enactment.Status.RetryCount[generationKey] = 0
-		setupLog.Info("Reset stale retry count",
-			"enactment", enactmentName,
-			"generation", generationKey)
-		return cli.Status().Update(ctx, enactment)
-	})
+	setupLog.Info("Finished reclaiming interrupted slots", "node", nodeName)
+	return nil
 }
 
 // Handler runs as a daemonset and we want that each handler pod will cache/reconcile only resources that belong the node it runs on.