Address comments

Signed-off-by: Quan Tian <qtian@vmware.com>

pkg/controller/egress/controller.go

@@ -206,8 +206,8 @@ func (c *EgressController) updateIPAllocation(egress *egressv1alpha2.Egress) {
 // TODO: Use validation webhook to ensure it.
 func (c *EgressController) createOrUpdateIPAllocator(ipPool *egressv1alpha2.ExternalIPPool) bool {
 	changed := false
-	c.ipAllocationMutex.Lock()
-	defer c.ipAllocationMutex.Unlock()
+	c.ipAllocatorMutex.Lock()
+	defer c.ipAllocatorMutex.Unlock()
 
 	existingIPRanges := sets.NewString()
 	multiIPAllocator, exists := c.ipAllocatorMap[ipPool.Name]
@@ -247,15 +247,15 @@ func (c *EgressController) createOrUpdateIPAllocator(ipPool *egressv1alpha2.Exte
 
 // deleteIPAllocator deletes the IP allocator of the given IP pool.
 func (c *EgressController) deleteIPAllocator(ipPoolName string) {
-	c.ipAllocationMutex.Lock()
-	defer c.ipAllocationMutex.Unlock()
+	c.ipAllocatorMutex.Lock()
+	defer c.ipAllocatorMutex.Unlock()
 	delete(c.ipAllocatorMap, ipPoolName)
 }
 
 // getIPAllocator gets the IP allocator of the given IP pool.
 func (c *EgressController) getIPAllocator(ipPoolName string) (ipallocator.MultiIPAllocator, bool) {
-	c.ipAllocationMutex.RLock()
-	defer c.ipAllocationMutex.RUnlock()
+	c.ipAllocatorMutex.RLock()
+	defer c.ipAllocatorMutex.RUnlock()
 	ipAllocator, exists := c.ipAllocatorMap[ipPoolName]
 	return ipAllocator, exists
 }
@@ -315,22 +315,22 @@ func (c *EgressController) syncEgressIP(egress *egressv1alpha2.Egress) (net.IP,
 	prevIP, prevIPPool, exists := c.getIPAllocation(egress.Name)
 	if exists {
 		_, ipAllocatorExists := c.getIPAllocator(prevIPPool)
-		// The EgressIP and the ExternalIPPool doesn't change, do nothing.
+		// The EgressIP and the ExternalIPPool don't change, do nothing.
 		if prevIP.String() == egress.Spec.EgressIP && prevIPPool == egress.Spec.ExternalIPPool && ipAllocatorExists {
 			return prevIP, nil
 		}
 		// Either EgressIP or ExternalIPPool changes, release the previous one first.
 		c.releaseEgressIP(egress.Name, prevIP, prevIPPool)
 	}
 
-	// Skip allocating EgressIP if ExternalIPPool is not specified and returns whatever user specifies.
+	// Skip allocating EgressIP if ExternalIPPool is not specified and return whatever user specifies.
 	if egress.Spec.ExternalIPPool == "" {
 		return net.ParseIP(egress.Spec.EgressIP), nil
 	}
 
 	ipAllocator, exists := c.ipAllocatorMap[egress.Spec.ExternalIPPool]
 	if !exists {
-		// The IP pool had been deleted, reclaim the IP from the Egress API.
+		// The IP pool has been deleted, reclaim the IP from the Egress API.
 		if egress.Spec.EgressIP != "" {
 			if err := c.updateEgressIP(egress, ""); err != nil {
 				return nil, err
@@ -406,7 +406,7 @@ func (c *EgressController) syncEgress(key string) error {
 
 	egress, err := c.egressLister.Get(key)
 	if err != nil {
-		// The Egress had been deleted, release its EgressIP if there was one.
+		// The Egress has been deleted, release its EgressIP if there was one.
 		if prevIP, prevIPPool, exists := c.getIPAllocation(key); exists {
 			c.releaseEgressIP(key, prevIP, prevIPPool)
 		}

pkg/controller/egress/controller_test.go

@@ -470,17 +470,35 @@ func TestSyncEgressIP(t *testing.T) {
 		expectErr                  bool
 	}{
 		{
-			name:                   "Egress with empty EgressIP and existing ExternalIPPool",
-			existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/24", "", ""),
+			name: "Egress with empty EgressIP and existing ExternalIPPool",
+			existingEgresses: []*v1alpha2.Egress{
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"},
+					Spec: v1alpha2.EgressSpec{
+						EgressIP:       "1.1.1.1",
+						ExternalIPPool: "ipPoolA",
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "egressB", UID: "uidB"},
+					Spec: v1alpha2.EgressSpec{
+						EgressIP:       "1.1.1.2",
+						ExternalIPPool: "ipPoolA",
+					},
+				},
+			},
+			// The first IPRange 1.1.1.0/30 should be occupied by the existing Egresses. The input Egress's IP should
+			// be allocated from the second IPRange 1.1.2.10-1.1.2.20.
+			existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/30", "1.1.2.10", "1.1.2.20"),
 			inputEgress: &v1alpha2.Egress{
-				ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"},
+				ObjectMeta: metav1.ObjectMeta{Name: "egressC", UID: "uidC"},
 				Spec: v1alpha2.EgressSpec{
 					EgressIP:       "",
 					ExternalIPPool: "ipPoolA",
 				},
 			},
-			expectedEgressIP:           "1.1.1.1",
-			expectedExternalIPPoolUsed: 1,
+			expectedEgressIP:           "1.1.2.10",
+			expectedExternalIPPoolUsed: 3,
 			expectErr:                  false,
 		},
 		{
@@ -572,7 +590,7 @@ func TestSyncEgressIP(t *testing.T) {
 			expectErr:                  false,
 		},
 		{
-			name: "Egress with conflicted EgressIP",
+			name: "Egress with conflicting EgressIP",
 			existingEgresses: []*v1alpha2.Egress{
 				{
 					ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"},