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