diff --git a/pkg/instances/instances.go b/pkg/instances/instances.go index 96f3ea7682..2323c34813 100644 --- a/pkg/instances/instances.go +++ b/pkg/instances/instances.go @@ -117,11 +117,12 @@ func (i *Instances) ensureInstanceGroupAndPorts(name, zone string, ports []int64 existingPorts[np.Port] = true } - // Determine which ports need to be added. Note that this adds existing ports as well. + // Determine which ports need to be added 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) } @@ -133,8 +134,8 @@ func (i *Instances) ensureInstanceGroupAndPorts(name, zone string, ports []int64 } if len(newNamedPorts) > 0 { - 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 { + 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 { return nil, err } } diff --git a/pkg/instances/instances_test.go b/pkg/instances/instances_test.go index c132582f21..e39dabb5d3 100644 --- a/pkg/instances/instances_test.go +++ b/pkg/instances/instances_test.go @@ -86,39 +86,43 @@ 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 { - desc string + activePorts []int64 expectedPorts []int64 }{ { - desc: "Set single port", - expectedPorts: []int64{80}, + // Verify setting a port works as expected. + []int64{80}, + []int64{80}, }, { - desc: "Two new ports + existing port", - expectedPorts: []int64{80, 81, 82}, + // Utilizing multiple new ports + []int64{81, 82}, + []int64{80, 81, 82}, }, { - desc: "Utilize all existing ports", - expectedPorts: []int64{80, 81, 82}, + // Utilizing existing ports + []int64{80, 82}, + []int64{80, 81, 82}, }, { - desc: "Two new ports + remove unused port", - expectedPorts: []int64{81, 82, 83, 84}, + // Utilizing a new port and an old port + []int64{80, 83}, + []int64{80, 81, 82, 83}, }, + // TODO: Add tests to remove named ports when we support that. } for _, test := range testCases { - igs, err := pool.EnsureInstanceGroupsAndPorts("ig", test.expectedPorts) + igs, err := pool.EnsureInstanceGroupsAndPorts("ig", test.activePorts) if err != nil { - t.Fatalf("unexpected error in setting ports %v to instance group: %s", test.expectedPorts, err) + t.Fatalf("unexpected error in setting ports %v to instance group: %s", test.activePorts, 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 number of named ports on instance group. expected: %v, got: %v", len(test.expectedPorts), len(actualPorts)) + t.Fatalf("unexpected named ports on instance group. expected: %v, got: %v", test.expectedPorts, actualPorts) } for i, p := range igs[0].NamedPorts { if p.Port != test.expectedPorts[i] {