diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 0dcaccaef8..7616bda300 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/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 diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 1d2bd60bb8..dd751bc544 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/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,12 +232,11 @@ 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 ) @@ -257,18 +244,13 @@ func (pm *CNSIPAMPoolMonitor) createNNCSpecForCRD(resetNotInUseList bool) (nnc.N // 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 diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index d20ba00d4d..24bc063480 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -10,7 +10,12 @@ import ( nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) -func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int, maxPodIPCount int64) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { +func initFakes(t *testing.T, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent int, + maxPodIPCount int64) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { logger.InitLogger("testlogs", 0, 0, "./") scalarUnits := nnc.Scaler{ @@ -28,7 +33,10 @@ func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, release fakecns.PoolMonitor = poolmonitor - fakerc.Reconcile() + err := fakerc.Reconcile(true) + if err != nil { + t.Fatalf("Failed to reconcile fake requestcontroller with err: %v", err) + } return fakecns, fakerc, poolmonitor } @@ -42,7 +50,12 @@ func TestPoolSizeIncrease(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, fakerc, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) // increase number of allocated IP's in CNS err := fakecns.SetNumberOfAllocatedIPs(8) @@ -58,11 +71,13 @@ func TestPoolSizeIncrease(t *testing.T) { // ensure pool monitor has reached quorum with cns if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state "+ + "after reconcile: %v, "+ + "actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // request controller reconciles, carves new IP's from the test subnet and adds to CNS state - err = fakerc.Reconcile() + err = fakerc.Reconcile(true) if err != nil { t.Fatalf("Failed to reconcile fake requestcontroller with err: %v", err) } @@ -76,15 +91,20 @@ func TestPoolSizeIncrease(t *testing.T) { // ensure pool monitor has reached quorum with cns if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't "+ + "match CNS pool state after reconcile: %v, "+ + "actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // make sure IPConfig state size reflects the new pool size if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { - t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) + t.Fatalf("CNS Pod IPConfig state count doesn't "+ + "match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) } - t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, "+ + "Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), + poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { @@ -96,7 +116,12 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, fakerc, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) // increase number of allocated IP's in CNS err := fakecns.SetNumberOfAllocatedIPs(8) @@ -124,11 +149,12 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { // ensure pool monitor has reached quorum with cns if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v,"+ + " actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // request controller reconciles, carves new IP's from the test subnet and adds to CNS state - err = fakerc.Reconcile() + err = fakerc.Reconcile(true) if err != nil { t.Fatalf("Failed to reconcile fake requestcontroller with err: %v", err) } @@ -142,15 +168,18 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { // make sure IPConfig state size reflects the new pool size if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { - t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", + len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) } // ensure pool monitor has reached quorum with cns if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, "+ + "actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } - t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), + poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolSizeIncreaseIdempotency(t *testing.T) { @@ -162,7 +191,12 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, _, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -181,7 +215,8 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { // ensure pool monitor has increased batch size if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v,"+ + " actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // reconcile pool monitor a second time, then verify requested ip count is still the same @@ -192,7 +227,8 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { // ensure pool monitor requested pool size is unchanged as request controller hasn't reconciled yet if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v,"+ + " actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } } @@ -205,7 +241,12 @@ func TestPoolIncreasePastNodeLimit(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, _, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -224,7 +265,8 @@ func TestPoolIncreasePastNodeLimit(t *testing.T) { // ensure pool monitor has only requested the max pod ip count if poolmonitor.cachedNNC.Spec.RequestedIPCount != maxPodIPCount { - t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the max has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) + t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the max "+ + "has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) } } @@ -237,7 +279,12 @@ func TestPoolIncreaseBatchSizeGreaterThanMaxPodIPCount(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, _, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -256,7 +303,8 @@ func TestPoolIncreaseBatchSizeGreaterThanMaxPodIPCount(t *testing.T) { // ensure pool monitor has only requested the max pod ip count if poolmonitor.cachedNNC.Spec.RequestedIPCount != maxPodIPCount { - t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the max has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) + t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) "+ + "when the max has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) } } @@ -270,10 +318,12 @@ func TestPoolIncreaseMaxIPCountSetToZero(t *testing.T) { expectedMaxPodIPCount = defaultMaxIPCount ) - _, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, initialMaxPodIPCount) + _, _, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, + requestThresholdPercent, releaseThresholdPercent, initialMaxPodIPCount) if poolmonitor.getMaxIPCount() != expectedMaxPodIPCount { - t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the MaxIPCount field in the CRD is zero", poolmonitor.getMaxIPCount(), expectedMaxPodIPCount) + t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) "+ + "when the MaxIPCount field in the CRD is zero", poolmonitor.getMaxIPCount(), expectedMaxPodIPCount) } } @@ -286,7 +336,8 @@ func TestPoolDecrease(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, + requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) @@ -317,21 +368,22 @@ func TestPoolDecrease(t *testing.T) { // ensure that the adjusted spec is smaller than the initial pool size if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", + (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // reconcile the fake request controller - err = fakerc.Reconcile() + err = fakerc.Reconcile(true) if err != nil { t.Fatal(err) } - // Ensure the size of the requested spec is still the same - if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != 0 { - t.Fatalf("Expected IPsNotInUse to be 0 after request controller reconcile, actual %v", poolmonitor.cachedNNC.Spec.IPsNotInUse) + // CNS won't actually clean up the IPsNotInUse until it changes the spec for some other reason (i.e. scale up) + // so instead we should just verify that the CNS state has no more PendingReleaseIPConfigs, + // and that they were cleaned up. + if len(fakecns.GetPendingReleaseIPConfigs()) != 0 { + t.Fatalf("expected 0 PendingReleaseIPConfigs, got %d", len(fakecns.GetPendingReleaseIPConfigs())) } - - return } func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { @@ -343,7 +395,8 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, + requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) @@ -362,12 +415,14 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { // Ensure the size of the requested spec is still the same if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected IP's not in use be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use be one batch size smaller after reconcile, expected %v,"+ + " actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v,"+ + " actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Update pods with IP count, ensure pool monitor stays the same until request controller reconciles @@ -378,15 +433,19 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { // Ensure the size of the requested spec is still the same if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected IP's not in use to be one batch size smaller after reconcile, and not change after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use to be one batch size smaller after reconcile, and not change"+ + " after reconcile, expected %v, actual %v", + (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, and not change after existing call, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, and not change after"+ + " existing call, expected %v, actual %v", (initialIPConfigCount - batchSize), + len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } - err = fakerc.Reconcile() + err = fakerc.Reconcile(true) if err != nil { t.Error(err) } @@ -398,8 +457,87 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { // Ensure the spec doesn't have any IPsNotInUse after request controller has reconciled if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != 0 { - t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", + (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + } +} + +func TestDecreaseAndIncreaseToSameCount(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 10 + requestThresholdPercent = 50 + releaseThresholdPercent = 150 + maxPodIPCount = int64(30) + ) + + fakecns, fakerc, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) + + log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) + log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) + + // initial pool count is 10, set 5 of them to be allocated + err := fakecns.SetNumberOfAllocatedIPs(7) + if err != nil { + t.Error(err) + } + + // Pool monitor will increase the count to 20 + t.Logf("Scaleup: Increase pool size to 20") + ReconcileAndValidate(context.Background(), t, poolmonitor, 20, 0) + + // Update the IPConfig state + t.Logf("Reconcile with PodIPState") + err = fakerc.Reconcile(true) + if err != nil { + t.Error(err) + } + + // Release all IPs + err = fakecns.SetNumberOfAllocatedIPs(0) + if err != nil { + t.Error(err) + } + + t.Logf("Scaledown: Decrease pool size to 10") + ReconcileAndValidate(context.Background(), t, poolmonitor, 10, 10) + + // Increase it back to 20 + // initial pool count is 10, set 5 of them to be allocated + t.Logf("Scaleup: pool size back to 20 without updating the PodIpState for previous scale down") + err = fakecns.SetNumberOfAllocatedIPs(7) + if err != nil { + t.Error(err) + } + ReconcileAndValidate(context.Background(), t, poolmonitor, 20, 10) + + // Update the IPConfig count and dont remove the pending IPs + t.Logf("Reconcile with PodIPState") + err = fakerc.Reconcile(false) + if err != nil { + t.Error(err) + } + + // reconcile again + t.Logf("Reconcole with pool monitor again, it should not cleanup ipsnotinuse") + ReconcileAndValidate(context.Background(), t, poolmonitor, 20, 10) + + t.Logf("Now update podipconfig state") + err = fakerc.Reconcile(true) + if err != nil { + t.Error(err) + } + + err = poolmonitor.Reconcile(context.Background()) + if err != nil { + t.Errorf("Expected no pool monitor failure after request controller reconcile: %v", err) } + ReconcileAndValidate(context.Background(), t, poolmonitor, 20, 0) } func TestPoolSizeDecreaseToReallyLow(t *testing.T) { @@ -411,7 +549,12 @@ func TestPoolSizeDecreaseToReallyLow(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, fakerc, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) @@ -443,12 +586,14 @@ func TestPoolSizeDecreaseToReallyLow(t *testing.T) { // Ensure the size of the requested spec is still the same if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != batchSize { - t.Fatalf("Expected IP's not in use is not correct, expected %v, actual %v", batchSize, len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use is not correct, expected %v, actual %v", + batchSize, len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, "+ + "actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Reconcile again, it should release the second batch @@ -460,16 +605,18 @@ func TestPoolSizeDecreaseToReallyLow(t *testing.T) { // Ensure the size of the requested spec is still the same if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != batchSize*2 { - t.Fatalf("Expected IP's not in use is not correct, expected %v, actual %v", batchSize*2, len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use is not correct, expected %v, actual %v", batchSize*2, + len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-(batchSize*2)) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, "+ + "actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } t.Logf("Update Request Controller") - err = fakerc.Reconcile() + err = fakerc.Reconcile(true) if err != nil { t.Error(err) } @@ -481,7 +628,8 @@ func TestPoolSizeDecreaseToReallyLow(t *testing.T) { // Ensure the spec doesn't have any IPsNotInUse after request controller has reconciled if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != 0 { - t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", + (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } } @@ -496,7 +644,12 @@ func TestDecreaseAfterNodeLimitReached(t *testing.T) { expectedDecreaseIP = int(maxPodIPCount) % batchSize ) - fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, _, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -524,12 +677,16 @@ func TestDecreaseAfterNodeLimitReached(t *testing.T) { // Ensure poolmonitor asked for a multiple of batch size if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(expectedRequestedIP) { - t.Fatalf("Expected requested ips to be %v when scaling by 1 batch size down from %v (max pod limit) but got %v", expectedRequestedIP, maxPodIPCount, poolmonitor.cachedNNC.Spec.RequestedIPCount) + t.Fatalf("Expected requested ips to be %v when scaling by 1 batch size down from %v "+ + "(max pod limit) but got %v", expectedRequestedIP, maxPodIPCount, + poolmonitor.cachedNNC.Spec.RequestedIPCount) } // Ensure we minused by the mod result if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != expectedDecreaseIP { - t.Fatalf("Expected to decrease requested IPs by %v (max pod count mod batchsize) to make the requested ip count a multiple of the batch size in the case of hitting the max before scale down, but got %v", expectedDecreaseIP, len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + t.Fatalf("Expected to decrease requested IPs by %v (max pod count mod batchsize) to "+ + "make the requested ip count a multiple of the batch size in the case of hitting "+ + "the max before scale down, but got %v", expectedDecreaseIP, len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } } @@ -542,7 +699,12 @@ func TestPoolDecreaseBatchSizeGreaterThanMaxPodIPCount(t *testing.T) { maxPodIPCount = int64(30) ) - fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent, maxPodIPCount) + fakecns, _, poolmonitor := initFakes(t, + batchSize, + initialIPConfigCount, + requestThresholdPercent, + releaseThresholdPercent, + maxPodIPCount) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -572,6 +734,32 @@ func TestPoolDecreaseBatchSizeGreaterThanMaxPodIPCount(t *testing.T) { // ensure pool monitor has only requested the max pod ip count if poolmonitor.cachedNNC.Spec.RequestedIPCount != maxPodIPCount { - t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the max has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) + t.Fatalf("Pool monitor target IP count (%v) should be the node limit (%v) when the max "+ + "has been reached", poolmonitor.cachedNNC.Spec.RequestedIPCount, maxPodIPCount) + } +} + +func ReconcileAndValidate(ctx context.Context, + t *testing.T, + poolmonitor *CNSIPAMPoolMonitor, + expectedRequestCount, + expectedIpsNotInUse int) { + err := poolmonitor.Reconcile(context.Background()) + if err != nil { + t.Errorf("Expected pool monitor to not fail after CNS set number of allocated IP's %v", err) + } + + // Increased the new count to be 20 + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(expectedRequestCount) { + t.Fatalf("RequestIPCount not same, expected %v, actual %v", + expectedRequestCount, + poolmonitor.cachedNNC.Spec.RequestedIPCount) + } + + // Ensure there is no pending release ips + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != expectedIpsNotInUse { + t.Fatalf("Expected IP's not in use, expected %v, actual %v", + expectedIpsNotInUse, + len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } } diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 6e27f72279..76d4c6e6ba 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -226,8 +226,8 @@ func (service *HTTPRestService) updateIPConfigsStateUntransacted( } // Validate TobeDeletedIps are ready to be deleted. - for ipId, _ := range tobeDeletedIpConfigs { - ipConfigStatus, exists := service.PodIPConfigState[ipId] + for ipID := range tobeDeletedIpConfigs { + ipConfigStatus, exists := service.PodIPConfigState[ipID] if exists { // pod ip exists, validate if state is not allocated, else fail if ipConfigStatus.State == cns.Allocated { @@ -238,8 +238,8 @@ func (service *HTTPRestService) updateIPConfigsStateUntransacted( } // now actually remove the deletedIPs - for ipId, _ := range tobeDeletedIpConfigs { - returncode, errMsg := service.removeToBeDeletedIPStateUntransacted(ipId, true) + for ipID := range tobeDeletedIpConfigs { + returncode, errMsg := service.removeToBeDeletedIPStateUntransacted(ipID, true) if returncode != types.Success { return returncode, errMsg } @@ -252,7 +252,8 @@ func (service *HTTPRestService) updateIPConfigsStateUntransacted( if hostNCVersionInInt, err = strconv.Atoi(hostVersion); err != nil { return types.UnsupportedNCVersion, fmt.Sprintf("Invalid hostVersion is %s, err:%s", hostVersion, err) } - service.addIPConfigStateUntransacted(req.NetworkContainerid, hostNCVersionInInt, req.SecondaryIPConfigs, existingSecondaryIPConfigs) + service.addIPConfigStateUntransacted(req.NetworkContainerid, hostNCVersionInInt, req.SecondaryIPConfigs, + existingSecondaryIPConfigs) return 0, "" } @@ -260,19 +261,25 @@ func (service *HTTPRestService) updateIPConfigsStateUntransacted( // addIPConfigStateUntransacted adds the IPConfigs to the PodIpConfigState map with Available state // If the IP is already added then it will be an idempotent call. Also note, caller will // acquire/release the service lock. -func (service *HTTPRestService) addIPConfigStateUntransacted(ncId string, hostVersion int, ipconfigs, existingSecondaryIPConfigs map[string]cns.SecondaryIPConfig) { +func (service *HTTPRestService) addIPConfigStateUntransacted(ncID string, hostVersion int, ipconfigs, + existingSecondaryIPConfigs map[string]cns.SecondaryIPConfig) { // add ipconfigs to state - for ipId, ipconfig := range ipconfigs { - // New secondary IP configs has new NC version however, CNS don't want to override existing IPs'with new NC version - // Set it back to previous NC version if IP already exist. - if existingIPConfig, existsInPreviousIPConfig := existingSecondaryIPConfigs[ipId]; existsInPreviousIPConfig { + for ipID, ipconfig := range ipconfigs { + // New secondary IP configs has new NC version however, CNS don't want to override existing IPs'with new + // NC version. Set it back to previous NC version if IP already exist. + if existingIPConfig, existsInPreviousIPConfig := existingSecondaryIPConfigs[ipID]; existsInPreviousIPConfig { ipconfig.NCVersion = existingIPConfig.NCVersion - ipconfigs[ipId] = ipconfig + ipconfigs[ipID] = ipconfig } - logger.Printf("[Azure-Cns] Set IP %s version to %d, programmed host nc version is %d", ipconfig.IPAddress, ipconfig.NCVersion, hostVersion) - if _, exists := service.PodIPConfigState[ipId]; exists { + + if ipState, exists := service.PodIPConfigState[ipID]; exists { + logger.Printf("[Azure-Cns] Set ipId %s, IP %s version to %d, programmed host nc version is %d, "+ + "ipState: %+v", ipID, ipconfig.IPAddress, ipconfig.NCVersion, hostVersion, ipState) continue } + + logger.Printf("[Azure-Cns] Set ipId %s, IP %s version to %d, programmed host nc version is %d", + ipID, ipconfig.IPAddress, ipconfig.NCVersion, hostVersion) // Using the updated NC version attached with IP to compare with latest nmagent version and determine IP statues. // When reconcile, service.PodIPConfigState doens't exist, rebuild it with the help of NC version attached with IP. var newIPCNSStatus string @@ -283,15 +290,15 @@ func (service *HTTPRestService) addIPConfigStateUntransacted(ncId string, hostVe } // add the new State ipconfigStatus := cns.IPConfigurationStatus{ - NCID: ncId, - ID: ipId, + NCID: ncID, + ID: ipID, IPAddress: ipconfig.IPAddress, State: newIPCNSStatus, PodInfo: nil, } logger.Printf("[Azure-Cns] Add IP %s as %s", ipconfig.IPAddress, newIPCNSStatus) - service.PodIPConfigState[ipId] = ipconfigStatus + service.PodIPConfigState[ipID] = ipconfigStatus // Todo Update batch API and maintain the count }