GCE: Updating instance groups interface to accept all named ports at once #1463

nikhiljindal · 2017-10-03T04:41:35Z

Follow up from #1033.

Updating the BackendPool (for backend services) and NodePool (for instance groups) interfaces so that all named ports can be added to an instance group at once. This removes unnecessary API calls where for each named port, we were first fetching the instance group and then setting the named port on it. With this change, we should fetch the instance group once and then set all named ports in a single API call.

Have also removed BackendPool.Sync method since Add does the same now.
Sync was also misleading since it only added backend services and didnt delete them. Add is more explicit.

cc @nicksardo @csbell @G-Harmon

k8s-reviewable · 2017-10-03T04:41:41Z

This change is

coveralls · 2017-10-03T04:58:29Z

Coverage increased (+0.05%) to 43.618% when pulling 9a8789d on nikhiljindal:refactorBackendSync into abc8b9d on kubernetes:master.

G-Harmon · 2017-10-04T16:47:19Z

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

controllers/gce/backends/backends_test.go, line 264 at r1 (raw file):

	pool, _ := newTestJig(f, fakeIGs, true)
	pool.Add([]ServicePort{ServicePort{Port: 81}}, nil)
	pool.Add([]ServicePort{ServicePort{Port: 90}}, nil)

Can you please update the unit test to exercise your new code using multiple ports? Could you combine these two pool.Add() calls?

controllers/gce/controller/utils_test.go, line 81 at r1 (raw file):

	gotZonesToNode := cm.fakeIGs.GetInstancesByZone()

	if cm.fakeIGs.Ports[0] != testPort[0] {

Is it useful or valuable to verify the length of cm.fakeIGs.Ports?

controllers/gce/instances/instances_test.go, line 72 at r1 (raw file):

	f = NewFakeInstanceGroups(sets.NewString([]string{"n1", "n2"}...))
	pool = newNodePool(f, defaultZone)
	pool.AddInstanceGroup("test", []int64{80})

same here, perhaps add a test that exercises multiple ports.

Comments from Reviewable

nikhiljindal · 2017-10-05T20:59:00Z

Review status: 8 of 11 files reviewed at latest revision, 3 unresolved discussions.

controllers/gce/backends/backends_test.go, line 264 at r1 (raw file):

Previously, G-Harmon wrote…

Can you please update the unit test to exercise your new code using multiple ports? Could you combine these two pool.Add() calls?

The line below this is calling Add with multiple ports.
There were already tests here for Add (which took a single port) and Sync (which took multiple ports) before this change.
I have updated both to call Add now, with single port or multiple ports - so we have tests for both.

controllers/gce/controller/utils_test.go, line 81 at r1 (raw file):

Previously, G-Harmon wrote…

Is it useful or valuable to verify the length of cm.fakeIGs.Ports?

Good point, done.

controllers/gce/instances/instances_test.go, line 72 at r1 (raw file):

Previously, G-Harmon wrote…

same here, perhaps add a test that exercises multiple ports.

Done. Figured there were no port specific tests.
Have added a separate TestSetNamedPorts with multiple test cases.

Comments from Reviewable

nikhiljindal · 2017-10-05T20:59:31Z

Thanks @G-Harmon for the review. Have updated code as per comments. PTAL

nicksardo · 2017-10-05T21:06:46Z

controllers/gce/backends/backends.go

-	defer func() { b.snapshotter.Add(portKey(p.Port), be) }()
-
-	var err error
+func (b *Backends) Add(svcPorts []ServicePort, igs []*compute.InstanceGroup) error {


What do you think about renaming this func to 'Sync'?

Sync will imply that we will add missing ones and delete extras. But this one adds and not deletes. Thats the reason I prefer add over sync

But we also update backend services.

nicksardo · 2017-10-05T21:10:21Z

controllers/gce/backends/backends.go

+// addWithIGs will get or create a Backend for the given port.
+// It assumes that the instance groups have been created and required named port has been added.
+// If not, then Add should be called instead.
+func (b *Backends) addWithIGs(p ServicePort, igs []*compute.InstanceGroup) error {


How about naming "syncBackendService"?

renamed to addBackendService. Same argument as above about sync vs add.

nicksardo · 2017-10-05T21:18:00Z

controllers/gce/instances/fakes.go

@@ -151,20 +150,19 @@ func (f *FakeInstanceGroups) RemoveInstancesFromInstanceGroup(name, zone string,

 func (f *FakeInstanceGroups) SetNamedPortsOfInstanceGroup(igName, zone string, namedPorts []*compute.NamedPort) error {
 	found := false
-	for _, ig := range f.instanceGroups {
-		if ig.Name == igName && ig.Zone == zone {
+	var ig *compute.InstanceGroup


Can remove the above found variable and use ig == nil for the "failed to find..." error.

nicksardo · 2017-10-05T21:21:46Z

controllers/gce/instances/instances.go

 				glog.V(3).Infof("Instance group %v already has named port %+v", ig.Name, np)
-				found = true
-				break
+				continue


glog.V(3).Infof("Instance group %v already has named port %+v", ig.Name, np) occurs very frequently. Recommend we bump the level to 5. Thoughts?

Sure. You look at logs a lot more than me, so happy to do whatever you say :)
Updated to 5.

nicksardo · 2017-10-05T21:22:50Z

controllers/gce/instances/instances.go

-			if np.Port == port {
+			existingPorts[np.Port] = true
+		}
+		newPorts := []*compute.NamedPort{}


nit: var newPorts []*compute.NamedPort

nicksardo · 2017-10-05T21:26:11Z

Done reviewing. PTAL at my comments but looks great otherwise.

nicksardo · 2017-10-05T21:28:48Z

Looks like CI failed building the tests: https://travis-ci.org/kubernetes/ingress/jobs/283895016

nikhiljindal · 2017-10-05T23:02:38Z

controllers/gce/controller/utils_test.go


 	// node pool syncs kube-nodes, this will add them to both igs.
 	lbc.CloudClusterManager.instancePool.Sync([]string{"n1", "n2", "n3"})
 	gotZonesToNode := cm.fakeIGs.GetInstancesByZone()

-	if cm.fakeIGs.Ports[0] != testPort {


Removed this since we already have a dedicated test for testing ports on instance groups. No need to duplicate the check here.

nikhiljindal · 2017-10-05T23:03:16Z

Thanks @nicksardo !
Updated code as per comments. PTAL

nikhiljindal · 2017-10-06T00:45:41Z

Renamed Add to Ensure as discussed

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 3, 2017

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2017

nikhiljindal force-pushed the refactorBackendSync branch from 9a8789d to 456c558 Compare October 5, 2017 20:58

nicksardo reviewed Oct 5, 2017

View reviewed changes

nicksardo added the backend/gce label Oct 5, 2017

nikhiljindal force-pushed the refactorBackendSync branch from ef37b4e to 4c463a7 Compare October 5, 2017 23:00

nikhiljindal commented Oct 5, 2017

View reviewed changes

Updating instances interface to accept all named ports at once

3dcdc26

nikhiljindal force-pushed the refactorBackendSync branch from 4c463a7 to 3dcdc26 Compare October 6, 2017 00:45

nicksardo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2017

nicksardo merged commit d662620 into kubernetes:master Oct 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: Updating instance groups interface to accept all named ports at once #1463

GCE: Updating instance groups interface to accept all named ports at once #1463

nikhiljindal commented Oct 3, 2017

k8s-reviewable commented Oct 3, 2017

coveralls commented Oct 3, 2017

G-Harmon commented Oct 4, 2017

nikhiljindal commented Oct 5, 2017

nikhiljindal commented Oct 5, 2017

nicksardo Oct 5, 2017

nikhiljindal Oct 5, 2017

nicksardo Oct 5, 2017

nicksardo Oct 5, 2017 •

edited

Loading

nikhiljindal Oct 5, 2017

nicksardo Oct 5, 2017

nikhiljindal Oct 5, 2017

nicksardo Oct 5, 2017

nikhiljindal Oct 5, 2017

nicksardo Oct 5, 2017

nikhiljindal Oct 5, 2017

nicksardo commented Oct 5, 2017

nicksardo commented Oct 5, 2017

nikhiljindal Oct 5, 2017

nikhiljindal commented Oct 5, 2017 •

edited

Loading

nikhiljindal commented Oct 6, 2017

GCE: Updating instance groups interface to accept all named ports at once #1463

GCE: Updating instance groups interface to accept all named ports at once #1463

Conversation

nikhiljindal commented Oct 3, 2017

k8s-reviewable commented Oct 3, 2017

coveralls commented Oct 3, 2017

G-Harmon commented Oct 4, 2017

nikhiljindal commented Oct 5, 2017

nikhiljindal commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo commented Oct 5, 2017

nicksardo commented Oct 5, 2017

Choose a reason for hiding this comment

nikhiljindal commented Oct 5, 2017 • edited Loading

nikhiljindal commented Oct 6, 2017

nicksardo Oct 5, 2017 •

edited

Loading

nikhiljindal commented Oct 5, 2017 •

edited

Loading