fix: hysteresis in ipampool during rapid pool scaling (#967)

* Fix CNS Scale Up/down fix to avoid cleanup pending ToBeDeleted ips

* remove pendingRelease bool flag

* Incorporate feedback

* incorporate lint failures

* fix more lint issues

* nit fixes again for lint errors

* fixes for lint

* fix comment spacing

* fix final lints

* fix broken ipampoolmonitor test

Co-authored-by: neaggarw <[email protected]>
Co-authored-by: Matthew Long <[email protected]>

Author: 3 people

cns/fakes/requestcontrollerfake.go

@@ -37,6 +37,7 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, scalar nnc.Scaler, su
 	rc.ip, _, _ = net.ParseCIDR(subnetAddressSpace)
 
 	rc.CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs)
+	rc.cachedCRD.Spec.RequestedIPCount = int64(numberOfIPConfigs)
 
 	return rc
 }
@@ -62,7 +63,6 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo
 	}
 
 	rc.fakecns.IPStateManager.AddIPConfigs(cnsIPConfigs)
-	rc.cachedCRD.Spec.RequestedIPCount = int64(len(cnsIPConfigs))
 
 	return cnsIPConfigs
 }
@@ -88,7 +88,7 @@ func remove(slice []nnc.IPAssignment, s int) []nnc.IPAssignment {
 	return append(slice[:s], slice[s+1:]...)
 }
 
-func (rc *RequestControllerFake) Reconcile() error {
+func (rc *RequestControllerFake) Reconcile(removePendingReleaseIPs bool) error {
 
 	diff := int(rc.cachedCRD.Spec.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState())
 
@@ -114,12 +114,11 @@ func (rc *RequestControllerFake) Reconcile() error {
 			rc.cachedCRD.Status.NetworkContainers[0].IPAssignments = remove(rc.cachedCRD.Status.NetworkContainers[0].IPAssignments, index)
 
 		}
+	}
 
-		// remove ipconfig from CNS
+	// remove ipconfig from CNS
+	if removePendingReleaseIPs {
 		rc.fakecns.IPStateManager.RemovePendingReleaseIPConfigs(rc.cachedCRD.Spec.IPsNotInUse)
-
-		// empty the not in use ip's from the spec
-		rc.cachedCRD.Spec.IPsNotInUse = []string{}
 	}
 
 	// update

cns/ipampoolmonitor/ipampoolmonitor.go

@@ -22,7 +22,6 @@ type CNSIPAMPoolMonitor struct {
 	cachedNNC                nnc.NodeNetworkConfig
 	httpService              cns.HTTPService
 	mu                       sync.RWMutex
-	pendingRelease           bool
 	rc                       singletenantcontroller.RequestController
 	scalarUnits              nnc.Scaler
 	updatingIpsNotInUseCount int
@@ -31,9 +30,8 @@ type CNSIPAMPoolMonitor struct {
 func NewCNSIPAMPoolMonitor(httpService cns.HTTPService, rc singletenantcontroller.RequestController) *CNSIPAMPoolMonitor {
 	logger.Printf("NewCNSIPAMPoolMonitor: Create IPAM Pool Monitor")
 	return &CNSIPAMPoolMonitor{
-		pendingRelease: false,
-		httpService:    httpService,
-		rc:             rc,
+		httpService: httpService,
+		rc:          rc,
 	}
 }
 
@@ -80,13 +78,13 @@ func (pm *CNSIPAMPoolMonitor) Reconcile(ctx context.Context) error {
 		return pm.increasePoolSize(ctx)
 
 	// pod count is decreasing
-	case freeIPConfigCount > pm.MaximumFreeIps:
+	case freeIPConfigCount >= pm.MaximumFreeIps:
 		logger.Printf("[ipam-pool-monitor] Decreasing pool size...%s ", msg)
 		return pm.decreasePoolSize(ctx, pendingReleaseIPCount)
 
 	// CRD has reconciled CNS state, and target spec is now the same size as the state
 	// free to remove the IP's from the CRD
-	case pm.pendingRelease && int(pm.cachedNNC.Spec.RequestedIPCount) == cnsPodIPConfigCount:
+	case len(pm.cachedNNC.Spec.IPsNotInUse) != pendingReleaseIPCount:
 		logger.Printf("[ipam-pool-monitor] Removing Pending Release IP's from CRD...%s ", msg)
 		return pm.cleanPendingRelease(ctx)
 
@@ -105,10 +103,7 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize(ctx context.Context) error {
 
 	var err error
 	var tempNNCSpec nnc.NodeNetworkConfigSpec
-	tempNNCSpec, err = pm.createNNCSpecForCRD(false)
-	if err != nil {
-		return err
-	}
+	tempNNCSpec = pm.createNNCSpecForCRD()
 
 	// Query the max IP count
 	maxIPCount := pm.getMaxIPCount()
@@ -187,10 +182,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize(ctx context.Context, existingPend
 	}
 
 	var tempNNCSpec nnc.NodeNetworkConfigSpec
-	tempNNCSpec, err = pm.createNNCSpecForCRD(false)
-	if err != nil {
-		return err
-	}
+	tempNNCSpec = pm.createNNCSpecForCRD()
 
 	if newIpsMarkedAsPending {
 		// cache the updatingPendingRelease so that we dont re-set new IPs to PendingRelease in case UpdateCRD call fails
@@ -212,7 +204,6 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize(ctx context.Context, existingPend
 
 	// save the updated state to cachedSpec
 	pm.cachedNNC.Spec = tempNNCSpec
-	pm.pendingRelease = true
 
 	// clear the updatingPendingIpsNotInUse, as we have Updated the CRD
 	logger.Printf("[ipam-pool-monitor] cleaning the updatingPendingIpsNotInUse, existing length %d", pm.updatingIpsNotInUseCount)
@@ -229,10 +220,7 @@ func (pm *CNSIPAMPoolMonitor) cleanPendingRelease(ctx context.Context) error {
 
 	var err error
 	var tempNNCSpec nnc.NodeNetworkConfigSpec
-	tempNNCSpec, err = pm.createNNCSpecForCRD(true)
-	if err != nil {
-		return err
-	}
+	tempNNCSpec = pm.createNNCSpecForCRD()
 
 	err = pm.rc.UpdateCRDSpec(ctx, tempNNCSpec)
 	if err != nil {
@@ -244,31 +232,25 @@ func (pm *CNSIPAMPoolMonitor) cleanPendingRelease(ctx context.Context) error {
 
 	// save the updated state to cachedSpec
 	pm.cachedNNC.Spec = tempNNCSpec
-	pm.pendingRelease = false
 	return nil
 }
 
 // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec
-func (pm *CNSIPAMPoolMonitor) createNNCSpecForCRD(resetNotInUseList bool) (nnc.NodeNetworkConfigSpec, error) {
+func (pm *CNSIPAMPoolMonitor) createNNCSpecForCRD() nnc.NodeNetworkConfigSpec {
 	var (
 		spec nnc.NodeNetworkConfigSpec
 	)
 
 	// DUpdate the count from cached spec
 	spec.RequestedIPCount = pm.cachedNNC.Spec.RequestedIPCount
 
-	// Discard the ToBeDeleted list if requested. This happens if DNC has cleaned up the pending ips and CNS has also updated its state.
-	if resetNotInUseList == true {
-		spec.IPsNotInUse = make([]string, 0)
-	} else {
-		// Get All Pending IPs from CNS and populate it again.
-		pendingIps := pm.httpService.GetPendingReleaseIPConfigs()
-		for _, pendingIp := range pendingIps {
-			spec.IPsNotInUse = append(spec.IPsNotInUse, pendingIp.ID)
-		}
+	// Get All Pending IPs from CNS and populate it again.
+	pendingIPs := pm.httpService.GetPendingReleaseIPConfigs()
+	for _, pendingIP := range pendingIPs {
+		spec.IPsNotInUse = append(spec.IPsNotInUse, pendingIP.ID)
 	}
 
-	return spec, nil
+	return spec
 }
 
 // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits