Ensure pending programming IPs will be released before available IPs when scale down. (#750)

* Ensure pending programming IPs will be released first when scale down.

* Addressed feedbacks for testing and coding style.

Author: csfmomo

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.

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{} {

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
 	}

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.

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) {

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{