GCE provider: Limit Filter calls to regexps rather than large blobs #27741

zmerlynn · 2016-06-21T00:48:03Z

Filters can't exceed 4k, and GET requests against the GCE API are also limited, so these break down in different ways at different cluster counts. Fix it by introducing an advisory node-instance-prefix configuration in the GCE provider that can hint the EnsureLoadBalancer/UpdateLoadBalancer code (and the firewall creation/update code). If it's not there, or wrong (a hostname that's registered violates it), just ignore it and grab the whole project.

Fixes #27731

zmerlynn · 2016-06-21T00:54:25Z

cc @roberthbailey: This needs a v1.2 cherrypick as well, once we get it in master.

zmerlynn · 2016-06-21T05:53:38Z

After testing this on a 1000 node cluster, I found another nasty surprise lurking in the same APIs, so I pushed a new iteration that killed the Field() limiters after filing an internal bug.

wojtek-t · 2016-06-21T12:40:41Z

pkg/cloudprovider/providers/gce/gce.go

@@ -2467,6 +2492,13 @@ func (gce *GCECloud) getInstancesByNames(names []string) ([]*gceInstance, error)
 			pageToken = res.NextPageToken
 			for _, i := range res.Items {
 				name := i.Name
+				if inst, ok := instances[name]; !ok || inst != nil {
+					if inst != nil {
+						glog.Warningf("GCE returned duplicate result for instance %q", name)


Can this really happen? It seems to me a bit paranoid.

I added it when was debugging that internal bug. I was thinking there was instability across pages (I.e. with the wrong options it returned a,b,c then c,d,f) in some race condition, which would then show up as a missing instance a few lines below (so the idea is that the pair of lines would show the instability). That ended up not being the case, but it doesn't seem harmful to leave in.

I went ahead and took it out. If the other case ever comes up, hopefully it'll be obvious (or non-harmful, e.g. duplicate results but all the results).

wojtek-t · 2016-06-21T12:42:20Z

I added a minor comment. Other than that LGTM.
Feel free to self-apply label.

Filters can't exceed 4k, and GET requests against the GCE API are also limited, so these break down in different ways at different cluster counts. Fix it by introducing an advisory node-instance-prefix configuration in the GCE provider that can hint the EnsureLoadBalancer/UpdateLoadBalancer code (and the firewall creation/update code). If it's not there, or wrong (a hostname that's registered violates it), just ignore it and grab the whole project.

k8s-bot · 2016-06-21T16:13:06Z

GCE e2e build/test passed for commit dd4dae4.

k8s-github-robot · 2016-06-21T16:13:48Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

bprashanth · 2016-06-21T16:53:05Z

pkg/cloudprovider/providers/gce/gce.go

 		if cfg.Global.Multizone {
 			managedZones = nil // Use all zones in region
 		}
 	}

-	return CreateGCECloud(projectID, region, zone, managedZones, networkURL, nodeTags, tokenSource, true /* useMetadataServer */)
+	return CreateGCECloud(projectID, region, zone, managedZones, networkURL, nodeTags, nodeInstancePrefix, tokenSource, true /* useMetadataServer */)
 }

 // Creates a GCECloud object using the specified parameters.
 // If no networkUrl is specified, loads networkName via rest call.
 // If no tokenSource is specified, uses oauth2.DefaultTokenSource.
 // If managedZones is nil / empty all zones in the region will be managed.


can you add a comment describing the exact use of nodeInstancePrefix and impact of passing in an empty string?

Didn't see this before it merged. To clarify it here (and I'll have a PR later): It's an advisory prefix to help GCE searches for nodes in the cluster. If nodes happen to join that violate the nodeInstancePrefix, the nodeInstancePrefix is ignored and the entire GCE project is enumerated.

Oh, and if it's empty, we enumerate the whole project.

k8s-bot · 2016-06-21T16:53:55Z

GCE e2e build/test passed for commit dd4dae4.

k8s-github-robot · 2016-06-21T16:54:01Z

Automatic merge from submit-queue

…41-upstream-release-1.2 Automated cherry pick of #27741

Fix to kubernetes/kubernetes#27821. Borrows the necessary piece of kubernetes/kubernetes#27741 to parse the gce.conf. Also opened kubernetes-retired#1251 to fix Godeps correctly

…ck-of-#27741-upstream-release-1.2 Automated cherry pick of kubernetes#27741

Fix to kubernetes/kubernetes#27821. Borrows the necessary piece of kubernetes/kubernetes#27741 to parse the gce.conf. Also opened kubernetes-retired/contrib#1251 to fix Godeps correctly

zmerlynn added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/network Categorizes an issue or PR as relevant to SIG Network. area/controller-manager cherrypick-candidate labels Jun 21, 2016

zmerlynn added this to the v1.3 milestone Jun 21, 2016

zmerlynn assigned cjcullen and bprashanth Jun 21, 2016

googlebot added the cla: yes label Jun 21, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 21, 2016

zmerlynn added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 21, 2016

zmerlynn force-pushed the fix-gce-filters branch from 57085b6 to 6acb5b8 Compare June 21, 2016 05:52

zmerlynn force-pushed the fix-gce-filters branch from 6acb5b8 to 626263e Compare June 21, 2016 06:19

wojtek-t reviewed Jun 21, 2016
View reviewed changes

wojtek-t self-assigned this Jun 21, 2016

zmerlynn force-pushed the fix-gce-filters branch from 626263e to dd4dae4 Compare June 21, 2016 15:34

zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2016

zmerlynn mentioned this pull request Jun 21, 2016

Rev GCE provider code in L7LB kubernetes-retired/contrib#1246

Closed

bprashanth reviewed Jun 21, 2016
View reviewed changes

k8s-github-robot merged commit 6cccb87 into kubernetes:master Jun 21, 2016

zmerlynn deleted the fix-gce-filters branch June 21, 2016 16:56

zmerlynn modified the milestones: v1.2, v1.3 Jun 21, 2016

roberthbailey added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 21, 2016

This was referenced Jun 21, 2016

Automated cherry pick of #27741 #27799

Merged

AWS/GCE: Spread PetSet volume creation across zones, create GCE volumes in non-master zones #27553

Merged

roberthbailey added a commit that referenced this pull request Jun 21, 2016

Merge pull request #27799 from zmerlynn/automated-cherry-pick-of-#277…

581eb1d

…41-upstream-release-1.2 Automated cherry pick of #27741

zmerlynn mentioned this pull request Jun 21, 2016

Bump glbc version to 0.7.0 #27814

Merged

This was referenced Jun 21, 2016

Ingress tests broken on gke #27813

Closed

cluster autoscaling tests broken on gce #27821

Closed

zmerlynn mentioned this pull request Jun 22, 2016

Transition gce.conf (and any other gcfg users) to something like text proto #27827

Closed

zmerlynn removed the cherrypick-candidate label Jun 22, 2016

zmerlynn mentioned this pull request Jun 22, 2016

Autoscaler: Super hacky workaround for GCE provider crashloop kubernetes-retired/contrib#1252

Merged

bprashanth mentioned this pull request Jun 22, 2016

Ingress tests panic invoking framework.BeforeEach #27486

Closed

roberthbailey changed the title ~~GCE provider: Limit Filter calls to regexps rather than insane blobs~~ GCE provider: Limit Filter calls to regexps rather than large blobs Jun 24, 2016

zmerlynn mentioned this pull request Jun 28, 2016

GCE provider: Create TargetPool with 200 instances, then update with rest #27829

Merged

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#27799 from zmerlynn/automated-cherry-pi…

79b6e38

…ck-of-#27741-upstream-release-1.2 Automated cherry pick of kubernetes#27741

shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017

Merge pull request kubernetes#27799 from zmerlynn/automated-cherry-pi…

347bbff

…ck-of-#27741-upstream-release-1.2 Automated cherry pick of kubernetes#27741

jingxu97 mentioned this pull request Mar 3, 2017

Modify getInstanceByName to avoid calling getInstancesByNames #42452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE provider: Limit Filter calls to regexps rather than large blobs #27741

GCE provider: Limit Filter calls to regexps rather than large blobs #27741

zmerlynn commented Jun 21, 2016 •

edited

Loading

zmerlynn commented Jun 21, 2016

zmerlynn commented Jun 21, 2016

wojtek-t Jun 21, 2016

zmerlynn Jun 21, 2016

zmerlynn Jun 21, 2016

wojtek-t commented Jun 21, 2016

k8s-bot commented Jun 21, 2016

k8s-github-robot commented Jun 21, 2016

bprashanth Jun 21, 2016

zmerlynn Jun 21, 2016

zmerlynn Jun 21, 2016

k8s-bot commented Jun 21, 2016

k8s-github-robot commented Jun 21, 2016

GCE provider: Limit Filter calls to regexps rather than large blobs #27741

GCE provider: Limit Filter calls to regexps rather than large blobs #27741

Conversation

zmerlynn commented Jun 21, 2016 • edited Loading

zmerlynn commented Jun 21, 2016

zmerlynn commented Jun 21, 2016

wojtek-t Jun 21, 2016

Choose a reason for hiding this comment

zmerlynn Jun 21, 2016

Choose a reason for hiding this comment

zmerlynn Jun 21, 2016

Choose a reason for hiding this comment

wojtek-t commented Jun 21, 2016

k8s-bot commented Jun 21, 2016

k8s-github-robot commented Jun 21, 2016

bprashanth Jun 21, 2016

Choose a reason for hiding this comment

zmerlynn Jun 21, 2016

Choose a reason for hiding this comment

zmerlynn Jun 21, 2016

Choose a reason for hiding this comment

k8s-bot commented Jun 21, 2016

k8s-github-robot commented Jun 21, 2016

zmerlynn commented Jun 21, 2016 •

edited

Loading