OPTI-3818: stop a transient DigitalOcean 422 from permanently stranding a Reserved IP

pkg/floatingip/ip_controller.go

@@ -446,27 +446,38 @@ func (i *ipController) reconcileIPStatus(ctx context.Context) {
 				}
 			}
 
-			expectedIP := i.providerIDToIP[providerID]
-			if expectedIP != "" && expectedIP != ip {
-				log.WithField("expected_ip", expectedIP).
-					Warn("node assignment mismatch; updating cache to reflect provider")
-				i.assignableIPs.Add(expectedIP, false)
-				// mark the node's old IP for immediate retry.
-				i.ipToStatus[expectedIP] = &ipStatus{
-					state:          flipopv1alpha1.IPStateError,
-					retry:          retry{retrySchedule: provider.RetryFast},
-					message:        "state unknown; cache / provider mismatch",
-					nodeProviderID: "", // reset
+			// Only record a new owner if there actually is one. Writing the empty
+			// key here means the next pass reads it back as expectedIP and reports a
+			// bogus assignment mismatch against an unrelated pool IP.
+			if providerID != "" {
+				expectedIP := i.providerIDToIP[providerID]
+				if expectedIP != "" && expectedIP != ip {
+					log.WithField("expected_ip", expectedIP).
+						Warn("node assignment mismatch; updating cache to reflect provider")
+					i.assignableIPs.Add(expectedIP, false)
+					// mark the node's old IP for immediate retry.
+					i.ipToStatus[expectedIP] = &ipStatus{
+						state:          flipopv1alpha1.IPStateError,
+						retry:          retry{retrySchedule: provider.RetryFast},
+						message:        "state unknown; cache / provider mismatch",
+						nodeProviderID: "", // reset
+					}
 				}
+				i.providerIDToIP[providerID] = ip
 			}
-			i.providerIDToIP[providerID] = ip
 
 			delete(i.providerIDToIP, expectedProviderID)
-			if evictedNodeName, ok := i.providerIDToNodeName[providerID]; ok {
+			// The node which held this IP no longer does, whether it was claimed by
+			// another node or released entirely. If it is still active it needs a
+			// replacement, so return it to the assignable queue. This was previously
+			// guarded on providerID - the IP's *new* owner - so an IP removed out of
+			// band, leaving it owned by nobody, left the node unqueued and no
+			// assignment was ever attempted again.
+			if evictedNodeName, ok := i.providerIDToNodeName[expectedProviderID]; ok {
 				log.WithFields(logrus.Fields{
 					"node": evictedNodeName,
-					"ip":   expectedIP,
-				}).Info("nodes ip was claimed by other node; marking for reassignment")
+					"ip":   ip,
+				}).Info("node no longer holds its ip; marking for reassignment")
 				i.assignableNodes.Add(expectedProviderID, true)
 			}
 		}
@@ -516,9 +527,14 @@ func (i *ipController) reconcileAssignment(ctx context.Context) {
 		ip := i.assignableIPs.Front()
 
 		// If this IP was previously involved in an error we shouldn't attempt to try again before
-		// its retry timestamp.
+		// its retry timestamp. The deadline must be compared with After(): deferring on an
+		// *elapsed* deadline and then handing that past timestamp to i.retry() makes
+		// retryTimerDuration() return 0, so run() reconciles again immediately and spins.
+		// The error state has to be part of the test as well - reconcileIPStatus reuses
+		// nextRetry for the routine healthyRetrySchedule refresh and sets it on every healthy
+		// IP just before this runs, so an unqualified check would defer them all.
 		status := i.ipToStatus[ip]
-		if !status.nextRetry.IsZero() && !status.nextRetry.After(now) {
+		if status.state == flipopv1alpha1.IPStateError && status.nextRetry.After(now) {
 			retryIPs = append(retryIPs, ip)
 			i.retry(status.nextRetry)
 			continue
@@ -527,9 +543,10 @@ func (i *ipController) reconcileAssignment(ctx context.Context) {
 		providerID := i.assignableNodes.Front()
 
 		// Similarly, if this node was involved in an error we should wait until after its retry
-		// timestamp has elapsed.
+		// timestamp has elapsed. providerIDToRetry only ever holds nodes whose assignment
+		// failed, and the entry is deleted on success, so no state qualifier is needed here.
 		nRetry, ok := i.providerIDToRetry[providerID]
-		if ok && !nRetry.nextRetry.IsZero() && !nRetry.nextRetry.After(now) {
+		if ok && nRetry.nextRetry.After(now) {
 			retryIPs = append(retryIPs, ip)
 			retryProviders = append(retryProviders, providerID)
 			i.retry(nRetry.nextRetry)
@@ -569,6 +586,16 @@ func (i *ipController) reconcileAssignment(ctx context.Context) {
 			status.message = fmt.Sprintf("assigning IP to node: %s", err)
 			status.assignmentErrors++
 			log.WithError(err).Error("assigning IP to node")
+
+			// Unwind optimistic bookkeeping and requeue both sides
+			delete(i.providerIDToIP, providerID)
+			status.nodeProviderID = oldProviderID
+			if oldProviderID != "" {
+				i.providerIDToIP[oldProviderID] = ip
+			}
+			retryIPs = append(retryIPs, ip)
+			retryProviders = append(retryProviders, providerID)
+
 			if nRetry == nil {
 				nRetry = &retry{
 					retrySchedule: status.retrySchedule,