Handle failed CloudPrivateIPConfig assignments

When a CPIC is created and the cloud controller externally sets its
status to Failed, ovn-kubernetes would not clean up or retry the assignment.

Reproducer:
1. CPIC assignment fails
2. The EgressIP status never reflects it as it ignores non-sucessfull assignments
3. Later reconciliation attempts trying to create the same CPIC again
4. Creation failures due to "already exists" errors

To fix the issue remove failed CPICs druing CPIC add reconciliation.

Signed-off-by: Patryk Diak <[email protected]>

Author: kyrtapz

go-controller/pkg/clustermanager/egressip_controller.go

@@ -18,9 +18,11 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/client-go/util/flowcontrol"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
@@ -211,6 +213,26 @@ func (eIPC *egressIPClusterController) executeCloudPrivateIPConfigOps(egressIPNa
 				if cloudPrivateIPConfig.GetDeletionTimestamp() != nil && !cloudPrivateIPConfig.GetDeletionTimestamp().IsZero() {
 					return fmt.Errorf("cloud update request failed, CloudPrivateIPConfig: %s is being deleted", cloudPrivateIPConfigName)
 				}
+
+				// Handle a scenario in which the object exists in a failed state by removing it
+				assignedCondition := meta.FindStatusCondition(cloudPrivateIPConfig.Status.Conditions, string(ocpcloudnetworkapi.Assigned))
+				if assignedCondition != nil && assignedCondition.Status == metav1.ConditionFalse {
+					klog.Warningf("CloudPrivateIPConfig: %s is in Failed state (reason: %s), deleting to allow retry", cloudPrivateIPConfigName, assignedCondition.Message)
+					eIPRef := corev1.ObjectReference{
+						Kind: "EgressIP",
+						Name: egressIPName,
+					}
+					eIPC.recorder.Eventf(&eIPRef, corev1.EventTypeWarning, "CloudAssignmentRetry",
+						"egress IP: %s previously failed on node %s (reason: %s), will retry assignment",
+						egressIP, cloudPrivateIPConfig.Spec.Node, assignedCondition.Message)
+					if err := eIPC.kube.DeleteCloudPrivateIPConfig(cloudPrivateIPConfigName); err != nil {
+						return fmt.Errorf("failed to delete failed CloudPrivateIPConfig: %s, err: %v", cloudPrivateIPConfigName, err)
+					}
+
+					// Return an error to trigger retry
+					return fmt.Errorf("deleted failed CloudPrivateIPConfig: %s, will retry creation in next reconciliation", cloudPrivateIPConfigName)
+				}
+
 				if op.toAdd == cloudPrivateIPConfig.Spec.Node {
 					klog.Infof("CloudPrivateIPConfig: %s already assigned to node: %s", cloudPrivateIPConfigName, cloudPrivateIPConfig.Spec.Node)
 					continue
@@ -371,6 +393,8 @@ type egressIPClusterController struct {
 	egressIPHandler *factory.Handler
 	// cloudPrivateIPConfig events factory handler
 	cloudPrivateIPConfigHandler *factory.Handler
+	// cloudPrivateIPBackoff failure backoff tracker
+	cloudPrivateIPBackoff *flowcontrol.Backoff
 }
 
 func newEgressIPController(ovnClient *util.OVNClusterManagerClientset, wf *factory.WatchFactory, recorder record.EventRecorder) *egressIPClusterController {
@@ -396,6 +420,7 @@ func newEgressIPController(ovnClient *util.OVNClusterManagerClientset, wf *facto
 		reachabilityCheckInterval:         egressIPReachabilityCheckInterval,
 		egressIPNodeHealthCheckPort:       config.OVNKubernetesFeature.EgressIPNodeHealthCheckPort,
 		stopChan:                          make(chan struct{}),
+		cloudPrivateIPBackoff:             flowcontrol.NewBackOff(30*time.Second, 5*time.Minute),
 	}
 	eIPC.initRetryFramework()
 	return eIPC
@@ -1498,18 +1523,22 @@ func (eIPC *egressIPClusterController) reconcileCloudPrivateIPConfig(old, new *o
 	}
 	if new != nil {
 		newCloudPrivateIPConfig = new
+		assignedCondition := meta.FindStatusCondition(newCloudPrivateIPConfig.Status.Conditions, string(ocpcloudnetworkapi.Assigned))
+
+		if assignedCondition != nil && assignedCondition.Status == metav1.ConditionFalse {
+			return eIPC.handleFailedCloudPrivateIPConfig(oldCloudPrivateIPConfig, assignedCondition)
+		}
+
 		// We should only proceed to setting things up for objects where the new
 		// object has the same .spec.node and .status.node, and assignment
 		// condition being true. This is how the cloud-network-config-controller
 		// indicates a successful cloud assignment.
-		shouldAdd = newCloudPrivateIPConfig.Status.Node == newCloudPrivateIPConfig.Spec.Node &&
-			ocpcloudnetworkapi.CloudPrivateIPConfigConditionType(newCloudPrivateIPConfig.Status.Conditions[0].Type) == ocpcloudnetworkapi.Assigned &&
-			corev1.ConditionStatus(newCloudPrivateIPConfig.Status.Conditions[0].Status) == corev1.ConditionTrue
+		shouldAdd = newCloudPrivateIPConfig.Status.Node == newCloudPrivateIPConfig.Spec.Node && assignedCondition != nil && assignedCondition.Status == metav1.ConditionTrue
 		// See above explanation for the delete
 		shouldDelete = shouldDelete &&
 			(newCloudPrivateIPConfig.Status.Node == "" || newCloudPrivateIPConfig.Status.Node != oldCloudPrivateIPConfig.Status.Node) &&
-			ocpcloudnetworkapi.CloudPrivateIPConfigConditionType(newCloudPrivateIPConfig.Status.Conditions[0].Type) == ocpcloudnetworkapi.Assigned &&
-			corev1.ConditionStatus(newCloudPrivateIPConfig.Status.Conditions[0].Status) == corev1.ConditionTrue
+			assignedCondition != nil && assignedCondition.Status == metav1.ConditionTrue
+
 		// On UPDATE we need to delete the old .status.node
 		if shouldDelete {
 			nodeToDelete = oldCloudPrivateIPConfig.Status.Node
@@ -1570,7 +1599,7 @@ func (eIPC *egressIPClusterController) reconcileCloudPrivateIPConfig(old, new *o
 		}
 		for _, resyncEgressIP := range resyncEgressIPs {
 			if err := eIPC.reconcileEgressIP(nil, resyncEgressIP); err != nil {
-				return fmt.Errorf("synthetic update for EgressIP: %s failed, err: %v", egressIP.Name, err)
+				return fmt.Errorf("synthetic update for EgressIP: %s failed, err: %v", resyncEgressIP.Name, err)
 			}
 		}
 	}
@@ -1587,11 +1616,15 @@ func (eIPC *egressIPClusterController) reconcileCloudPrivateIPConfig(old, new *o
 				oldCloudPrivateIPConfig.Name)
 			return nil
 		}
+
+		// Reset backoff on successful assignment for this specific IP
+		egressIPString := cloudPrivateIPConfigNameToIPString(newCloudPrivateIPConfig.Name)
+		eIPC.cloudPrivateIPBackoff.Reset(egressIPString)
+
 		egressIP, err := eIPC.kube.GetEgressIP(egressIPName)
 		if err != nil {
 			return err
 		}
-		egressIPString := cloudPrivateIPConfigNameToIPString(newCloudPrivateIPConfig.Name)
 		statusItem := egressipv1.EgressIPStatusItem{
 			Node:     newCloudPrivateIPConfig.Status.Node,
 			EgressIP: egressIPString,
@@ -1668,24 +1701,63 @@ func cloudPrivateIPConfigNameToIPString(name string) string {
 	return net.ParseIP(name).String()
 }
 
+// handleFailedCloudPrivateIPConfig handles CPIC assignment failures by
+// scheduling EgressIP retry with exponential backoff
+func (eIPC *egressIPClusterController) handleFailedCloudPrivateIPConfig(
+	cloudPrivateIPConfig *ocpcloudnetworkapi.CloudPrivateIPConfig,
+	failureCondition *metav1.Condition) error {
+
+	egressIPName, exists := cloudPrivateIPConfig.Annotations[util.OVNEgressIPOwnerRefLabel]
+	if !exists {
+		klog.Warningf("CloudPrivateIPConfig %q is missing egress IP owner reference annotation upon failure",
+			cloudPrivateIPConfig.Name)
+		return nil
+	}
+
+	klog.Warningf("CloudPrivateIPConfig %s for EgressIP %s failed (reason: %s), scheduling retry with backoff",
+		cloudPrivateIPConfig.Name, egressIPName, failureCondition.Message)
+
+	// Get the EgressIP object
+	egressIP, err := eIPC.kube.GetEgressIP(egressIPName)
+	if apierrors.IsNotFound(err) {
+		// EgressIP was deleted, nothing to retry
+		klog.V(5).Infof("EgressIP %s no longer exists, skipping retry", egressIPName)
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+
+	egressIPAddr := cloudPrivateIPConfigNameToIPString(cloudPrivateIPConfig.Name)
+	eIPC.cloudPrivateIPBackoff.Next(egressIPAddr, eIPC.cloudPrivateIPBackoff.Clock.Now())
+	backoff := eIPC.cloudPrivateIPBackoff.Get(egressIPAddr)
+	klog.Infof("Scheduling EgressIP %s retry for IP %s with backoff %v after CPIC failure",
+		egressIPName, egressIPAddr, backoff)
+	if err := eIPC.retryEgressIPs.AddRetryObjWithAddBackoff(egressIP, backoff); err != nil {
+		return fmt.Errorf("failed to add EgressIP %s to retry framework: %v", egressIPName, err)
+	}
+
+	return nil
+}
+
 // removePendingOps removes the existing pending CloudPrivateIPConfig operations
 // from the cache and returns the EgressIP object which can be re-synced given
 // the new assignment possibilities.
-func (eIPC *egressIPClusterController) removePendingOpsAndGetResyncs(egressIPName, egressIP string) ([]*egressipv1.EgressIP, error) {
+func (eIPC *egressIPClusterController) removePendingOpsAndGetResyncs(egressIPName, egressIPAddr string) ([]*egressipv1.EgressIP, error) {
 	eIPC.pendingCloudPrivateIPConfigsMutex.Lock()
 	defer eIPC.pendingCloudPrivateIPConfigsMutex.Unlock()
 	ops, pending := eIPC.pendingCloudPrivateIPConfigsOps[egressIPName]
 	if !pending {
 		return nil, fmt.Errorf("no pending operation found for EgressIP: %s", egressIPName)
 	}
-	op, exists := ops[egressIP]
+	op, exists := ops[egressIPAddr]
 	if !exists {
-		return nil, fmt.Errorf("pending operations found for EgressIP: %s, but not for the finalized IP: %s", egressIPName, egressIP)
+		return nil, fmt.Errorf("pending operations found for EgressIP: %s, but not for the finalized IP: %s", egressIPName, egressIPAddr)
 	}
 	// Make sure we are dealing with a delete operation, since for update
 	// operations will still need to process the add afterwards.
 	if op.toAdd == "" && op.toDelete != "" {
-		delete(ops, egressIP)
+		delete(ops, egressIPAddr)
 	}
 	if len(ops) == 0 {
 		delete(eIPC.pendingCloudPrivateIPConfigsOps, egressIPName)
@@ -1707,7 +1779,12 @@ func (eIPC *egressIPClusterController) removePendingOpsAndGetResyncs(egressIPNam
 		// CloudPrivateIPConfig for which we are currently processing the
 		// deletion for.
 		if egressIP.Name == egressIPName {
-			continue
+			for _, specIP := range egressIP.Spec.EgressIPs {
+				// if the EgressIP object still has the IP that got removed in the spec it likely wants it assigned back so retry
+				if specIP == egressIPAddr {
+					resyncs = append(resyncs, &egressIP)
+				}
+			}
 		}
 		unassigned := len(egressIP.Spec.EgressIPs) - len(egressIP.Status.Items)
 		ops, pending := eIPC.pendingCloudPrivateIPConfigsOps[egressIP.Name]

go-controller/pkg/retry/obj_retry.go

@@ -201,6 +201,19 @@ func (r *RetryFramework) AddRetryObjWithAddNoBackoff(obj interface{}) error {
 	return nil
 }
 
+// AddRetryObjWithAddBackoff adds an object to be retried for add with a custom backoff.
+// It will lock the key, create or update retryObject, and unlock the key
+func (r *RetryFramework) AddRetryObjWithAddBackoff(obj interface{}, backoff time.Duration) error {
+	key, err := GetResourceKey(obj)
+	if err != nil {
+		return fmt.Errorf("could not get the key of %s %v: %v", r.ResourceHandler.ObjType, obj, err)
+	}
+	r.DoWithLock(key, func(key string) {
+		r.initRetryObjWithAddBackoff(obj, key, backoff)
+	})
+	return nil
+}
+
 func (r *RetryFramework) getRetryObj(lockedKey string) (value *retryObjEntry, found bool) {
 	return r.retryEntries.Load(lockedKey)
 }