From fbe88aa88b961ab683af8486da6e040cbd75584c Mon Sep 17 00:00:00 2001
From: Rohit Ramkumar <rramkumar@google.com>
Date: Mon, 13 Aug 2018 10:27:41 -0700
Subject: [PATCH] Remove unused named ports from instance group's

---
 pkg/instances/instances.go      |  7 +++----
 pkg/instances/instances_test.go | 30 +++++++++++++-----------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/pkg/instances/instances.go b/pkg/instances/instances.go
index 2323c34813..96f3ea7682 100644
--- a/pkg/instances/instances.go
+++ b/pkg/instances/instances.go
@@ -117,12 +117,11 @@ func (i *Instances) ensureInstanceGroupAndPorts(name, zone string, ports []int64
 		existingPorts[np.Port] = true
 	}
 
-	// Determine which ports need to be added
+	// Determine which ports need to be added. Note that this adds existing ports as well.
 	var newPorts []int64
 	for _, p := range ports {
 		if existingPorts[p] {
 			glog.V(5).Infof("Instance group %v/%v already has named port %v", zone, ig.Name, p)
-			continue
 		}
 		newPorts = append(newPorts, p)
 	}
@@ -134,8 +133,8 @@ func (i *Instances) ensureInstanceGroupAndPorts(name, zone string, ports []int64
 	}
 
 	if len(newNamedPorts) > 0 {
-		glog.V(3).Infof("Instance group %v/%v does not have ports %+v, adding them now.", zone, name, newPorts)
-		if err := i.cloud.SetNamedPortsOfInstanceGroup(ig.Name, zone, append(ig.NamedPorts, newNamedPorts...)); err != nil {
+		glog.V(3).Infof("Setting named ports %+v on instance group %v/%v", zone, name, newPorts)
+		if err := i.cloud.SetNamedPortsOfInstanceGroup(ig.Name, zone, newNamedPorts); err != nil {
 			return nil, err
 		}
 	}
diff --git a/pkg/instances/instances_test.go b/pkg/instances/instances_test.go
index e39dabb5d3..c132582f21 100644
--- a/pkg/instances/instances_test.go
+++ b/pkg/instances/instances_test.go
@@ -86,43 +86,39 @@ func TestSetNamedPorts(t *testing.T) {
 	f := NewFakeInstanceGroups(sets.NewString([]string{"ig"}...), defaultNamer)
 	pool := newNodePool(f, defaultZone)
 
+	// Note: each test case is dependent on the previous.
 	testCases := []struct {
-		activePorts   []int64
+		desc          string
 		expectedPorts []int64
 	}{
 		{
-			// Verify setting a port works as expected.
-			[]int64{80},
-			[]int64{80},
+			desc:          "Set single port",
+			expectedPorts: []int64{80},
 		},
 		{
-			// Utilizing multiple new ports
-			[]int64{81, 82},
-			[]int64{80, 81, 82},
+			desc:          "Two new ports + existing port",
+			expectedPorts: []int64{80, 81, 82},
 		},
 		{
-			// Utilizing existing ports
-			[]int64{80, 82},
-			[]int64{80, 81, 82},
+			desc:          "Utilize all existing ports",
+			expectedPorts: []int64{80, 81, 82},
 		},
 		{
-			// Utilizing a new port and an old port
-			[]int64{80, 83},
-			[]int64{80, 81, 82, 83},
+			desc:          "Two new ports + remove unused port",
+			expectedPorts: []int64{81, 82, 83, 84},
 		},
-		// TODO: Add tests to remove named ports when we support that.
 	}
 	for _, test := range testCases {
-		igs, err := pool.EnsureInstanceGroupsAndPorts("ig", test.activePorts)
+		igs, err := pool.EnsureInstanceGroupsAndPorts("ig", test.expectedPorts)
 		if err != nil {
-			t.Fatalf("unexpected error in setting ports %v to instance group: %s", test.activePorts, err)
+			t.Fatalf("unexpected error in setting ports %v to instance group: %s", test.expectedPorts, err)
 		}
 		if len(igs) != 1 {
 			t.Fatalf("expected a single instance group, got: %v", igs)
 		}
 		actualPorts := igs[0].NamedPorts
 		if len(actualPorts) != len(test.expectedPorts) {
-			t.Fatalf("unexpected named ports on instance group. expected: %v, got: %v", test.expectedPorts, actualPorts)
+			t.Fatalf("unexpected number of named ports on instance group. expected: %v, got: %v", len(test.expectedPorts), len(actualPorts))
 		}
 		for i, p := range igs[0].NamedPorts {
 			if p.Port != test.expectedPorts[i] {