diff --git a/cns/api.go b/cns/api.go index 61d8ec82e3..dab67d9bd8 100644 --- a/cns/api.go +++ b/cns/api.go @@ -43,7 +43,7 @@ type HTTPService interface { GetAllocatedIPConfigs() []IPConfigurationStatus GetPendingReleaseIPConfigs() []IPConfigurationStatus GetPodIPConfigState() map[string]IPConfigurationStatus - MarkIPsAsPending(numberToMark int) (map[string]IPConfigurationStatus, error) + MarkIPAsPendingRelease(numberToMark int) (map[string]IPConfigurationStatus, error) } // This is used for KubernetesCRD orchastrator Type where NC has multiple ips. diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 3177b1d679..41bf451c19 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -132,7 +132,7 @@ func (ipm *IPStateManager) ReleaseIPConfig(ipconfigID string) (cns.IPConfigurati return ipm.AvailableIPConfigState[ipconfigID], nil } -func (ipm *IPStateManager) MarkIPsAsPending(numberOfIPsToMark int) (map[string]cns.IPConfigurationStatus, error) { +func (ipm *IPStateManager) MarkIPAsPendingRelease(numberOfIPsToMark int) (map[string]cns.IPConfigurationStatus, error) { ipm.Lock() defer ipm.Unlock() @@ -292,8 +292,8 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio } // TODO: Populate on scale down -func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { - return fake.IPStateManager.MarkIPsAsPending(numberToMark) +func (fake *HTTPServiceFake) MarkIPAsPendingRelease(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { + return fake.IPStateManager.MarkIPAsPendingRelease(numberToMark) } func (fake *HTTPServiceFake) GetOption(string) interface{} { diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index b2dcadf251..df54783a76 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -124,7 +124,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { pm.cachedNNC.Spec.RequestedIPCount -= pm.scalarUnits.BatchSize // mark n number of IP's as pending - pendingIpAddresses, err := pm.cns.MarkIPsAsPending(int(pm.scalarUnits.BatchSize)) + pendingIpAddresses, err := pm.cns.MarkIPAsPendingRelease(int(pm.scalarUnits.BatchSize)) if err != nil { return err } diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 843971a207..d2960b90a7 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -92,26 +92,40 @@ func (service *HTTPRestService) releaseIPConfigHandler(w http.ResponseWriter, r return } -func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { - pendingReleaseIPs := make(map[string]cns.IPConfigurationStatus) - markedIPCount := 0 +// MarkIPAsPendingRelease will mark IPs to pending release state. +func (service *HTTPRestService) MarkIPAsPendingRelease(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { + allReleasedIPs := make(map[string]cns.IPConfigurationStatus) + // Ensure PendingProgramming IPs will be release before Available ones. + ipStateTypes := [2]string{cns.PendingProgramming, cns.Available} service.Lock() defer service.Unlock() - for uuid, _ := range service.PodIPConfigState { - mutableIPConfig := service.PodIPConfigState[uuid] - if mutableIPConfig.State == cns.Available || mutableIPConfig.State == cns.PendingProgramming { + for _, ipStateType := range ipStateTypes { + pendingReleaseIPs := service.markSpecificIPTypeAsPending(numberToMark, ipStateType) + for uuid, pependingReleaseIP := range pendingReleaseIPs { + allReleasedIPs[uuid] = pependingReleaseIP + } + numberToMark -= len(pendingReleaseIPs) + if numberToMark == 0 { + return allReleasedIPs, nil + } + } + return nil, fmt.Errorf("Failed to mark %d IP's as pending, only marked %d IP's", numberToMark, len(allReleasedIPs)) +} + +func (service *HTTPRestService) markSpecificIPTypeAsPending(numberToMark int, ipStateType string) map[string]cns.IPConfigurationStatus { + pendingReleaseIPs := make(map[string]cns.IPConfigurationStatus) + for uuid, mutableIPConfig := range service.PodIPConfigState { + if mutableIPConfig.State == ipStateType { mutableIPConfig.State = cns.PendingRelease service.PodIPConfigState[uuid] = mutableIPConfig pendingReleaseIPs[uuid] = mutableIPConfig - markedIPCount++ - if markedIPCount == numberToMark { - return pendingReleaseIPs, nil + if len(pendingReleaseIPs) == numberToMark { + return pendingReleaseIPs } } } - - return nil, fmt.Errorf("Failed to mark %d IP's as pending, only marked %d IP's", numberToMark, len(pendingReleaseIPs)) + return pendingReleaseIPs } // MarkIpsAsAvailableUntransacted will update pending programming IPs to available if NMAgent side's programmed nc version keep up with nc version. diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 9b4cd89304..80d03023df 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "reflect" + "strconv" "testing" "github.com/Azure/azure-container-networking/cns" @@ -37,6 +38,9 @@ var ( PodName: "testpod3", PodNamespace: "testpod3namespace", } + + testIP4 = "10.0.0.4" + testPod4GUID = "718e04ac-5a13-4dce-84b3-040accaa9b42" ) func getTestService() *HTTPRestService { @@ -550,7 +554,7 @@ func TestIPAMMarkIPCountAsPending(t *testing.T) { } // Release Test Pod 1 - ips, err := svc.MarkIPsAsPending(1) + ips, err := svc.MarkIPAsPendingRelease(1) if err != nil { t.Fatalf("Unexpected failure releasing IP: %+v", err) } @@ -575,6 +579,93 @@ func TestIPAMMarkIPCountAsPending(t *testing.T) { if err != nil { t.Fatalf("Unexpected failure releasing IP: %+v", err) } + + // Try to release IP when no IP can be released. It should return error and ips will be nil + ips, err = svc.MarkIPAsPendingRelease(1) + if err == nil || ips != nil { + t.Fatalf("We are expecting err and ips should be nil, however, return these IP %v", ips) + } +} + +func TestIPAMMarkIPAsPendingWithPendingProgrammingIPs(t *testing.T) { + svc := getTestService() + + secondaryIPConfigs := make(map[string]cns.SecondaryIPConfig) + // Default Programmed NC version is -1, set nc version as 0 will result in pending programming state. + constructSecondaryIPConfigs(testIP1, testPod1GUID, 0, secondaryIPConfigs) + constructSecondaryIPConfigs(testIP3, testPod3GUID, 0, secondaryIPConfigs) + // Default Programmed NC version is -1, set nc version as -1 will result in available state. + constructSecondaryIPConfigs(testIP2, testPod2GUID, -1, secondaryIPConfigs) + constructSecondaryIPConfigs(testIP4, testPod4GUID, -1, secondaryIPConfigs) + + // createNCRequest with NC version 0 + req := generateNetworkContainerRequest(secondaryIPConfigs, testNCID, strconv.Itoa(0)) + returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) + if returnCode != 0 { + t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) + } + + // Release pending programming IPs + ips, err := svc.MarkIPAsPendingRelease(2) + if err != nil { + t.Fatalf("Unexpected failure releasing IP: %+v", err) + } + // Check returning released IPs are from pod 1 and 3 + if _, exists := ips[testPod1GUID]; !exists { + t.Fatalf("Expected ID not marked as pending: %+v, ips is %s", err, ips) + } + if _, exists := ips[testPod3GUID]; !exists { + t.Fatalf("Expected ID not marked as pending: %+v, ips is %s", err, ips) + } + + pendingRelease := svc.GetPendingReleaseIPConfigs() + if len(pendingRelease) != 2 { + t.Fatalf("Expected 2 pending release IPs but got %d pending release IP", len(pendingRelease)) + } + // Check pending release IDs are from pod 1 and 3 + for _, config := range pendingRelease { + if config.ID != testPod1GUID && config.ID != testPod3GUID { + t.Fatalf("Expected pending release ID is either from pod 1 or pod 3 but got ID as %s ", config.ID) + } + } + + available := svc.GetAvailableIPConfigs() + if len(available) != 2 { + t.Fatalf("Expected 1 available IP with test pod 2 but got available %d IP", len(available)) + } + + // Call release again, should be fine + err = svc.releaseIPConfig(testPod1Info) + if err != nil { + t.Fatalf("Unexpected failure releasing IP: %+v", err) + } + + // Release 2 more IPs + ips, err = svc.MarkIPAsPendingRelease(2) + if err != nil { + t.Fatalf("Unexpected failure releasing IP: %+v", err) + } + // Make sure newly released IPs are from pod 2 and pod 4 + if _, exists := ips[testPod2GUID]; !exists { + t.Fatalf("Expected ID not marked as pending: %+v, ips is %s", err, ips) + } + if _, exists := ips[testPod4GUID]; !exists { + t.Fatalf("Expected ID not marked as pending: %+v, ips is %s", err, ips) + } + + // Get all pending release IPs and check total number is 4 + pendingRelease = svc.GetPendingReleaseIPConfigs() + if len(pendingRelease) != 4 { + t.Fatalf("Expected 4 pending release IPs but got %d pending release IP", len(pendingRelease)) + } +} + +func constructSecondaryIPConfigs(ipAddress, uuid string, ncVersion int, secondaryIPConfigs map[string]cns.SecondaryIPConfig) { + secIpConfig := cns.SecondaryIPConfig{ + IPAddress: ipAddress, + NCVersion: ncVersion, + } + secondaryIPConfigs[uuid] = secIpConfig } func TestIPAMMarkExistingIPConfigAsPending(t *testing.T) { diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 5fcfda7dc1..a8d24fff9b 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -258,7 +258,6 @@ func (service *HTTPRestService) updateIpConfigsStateUntransacted(req cns.CreateN // 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) { - newIPCNSStatus := cns.Available // 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 @@ -274,8 +273,11 @@ func (service *HTTPRestService) addIPConfigStateUntransacted(ncId string, hostVe } // 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 if hostVersion < ipconfig.NCVersion { newIPCNSStatus = cns.PendingProgramming + } else { + newIPCNSStatus = cns.Available } // add the new State ipconfigStatus := cns.IPConfigurationStatus{