do not sync IG when not necessary #1105

freehan · 2020-05-19T18:34:57Z

No description provided.

bowei · 2020-05-19T18:40:05Z

Can you fill out the commit message with details about what is being fixed?
How was this tested?

pkg/controller/controller.go

wojtek-t · 2020-05-19T18:41:46Z

LGTM

But I will let @bowei to tag it - he is more familiar with the codebase than me.

freehan · 2020-05-19T18:51:06Z

@bowei Updated the commit message.

freehan · 2020-05-19T19:22:03Z

Added testing. Ready for another round.

bowei · 2020-05-20T05:46:52Z

pkg/controller/controller.go

@@ -398,6 +373,9 @@ func (lbc *LoadBalancerController) SyncBackends(state interface{}) error {

 	// Get the zones our groups live in.
 	zones, err := lbc.Translator.ListZones()
+	if err != nil {
+		klog.Errorf("Failed to list zones: %v", err)


Isn't this a problem to go onto the rest of the flow if we aren't able to get a list of the zones?

bowei · 2020-05-20T16:38:50Z

BTW -- can we have commits message that follow this: https://chris.beams.io/posts/git-commit/

For trick change like this, it's worthwhile explaining a bit more about these changes.
The commit messages help the reviewer as well as those who are going to be maintaining the code

bowei · 2020-05-20T16:38:57Z

/hold

freehan · 2020-05-20T22:37:22Z

Updated commit message.
Fix the comments.
Ready for another round.

pkg/controller/controller.go

bowei · 2020-05-21T20:42:09Z

pkg/controller/controller.go

-		if err = updateAnnotations(lbc.ctx.KubeClient, ing.Name, ing.Namespace, ing.Annotations); err != nil {
+	// Only sync instance group when IG is used for this ingress
+	if len(nodePorts(ingSvcPorts)) > 0 {
+		if err := lbc.syncInstanceGroup(syncState.ing, ingSvcPorts); err != nil {


log which ingress this was looking at syncState.ing.

fix this later, you should always use the right type vs %v (%s:%s)

pkg/controller/controller.go

bowei · 2020-05-21T20:43:38Z

pkg/controller/controller.go

@@ -421,6 +399,40 @@ func (lbc *LoadBalancerController) SyncBackends(state interface{}) error {
 	return nil
 }

+// syncInstanceGroup creates instance groups, syncs instances, sets named ports and updates instance group annotation
+func (lbc *LoadBalancerController) syncInstanceGroup(ing *v1beta1.Ingress, ingSvcPorts []utils.ServicePort) error {
+	igs, err := lbc.instancePool.EnsureInstanceGroupsAndPorts(lbc.ctx.ClusterNamer.InstanceGroup(), nodePorts(ingSvcPorts))


log the action taking place here

klog.V(2).Infof(

ns, name of ingress
svc ports,

pkg/controller/controller.go

bowei · 2020-05-21T20:45:04Z

need to log major actions

freehan · 2020-05-21T21:03:52Z

Fix the comments.
Ready for another round.

bowei · 2020-05-26T19:54:19Z

pkg/controller/controller.go

-		// This short-circuit will stop the syncer from moving to next step.
-		return ingsync.ErrSkipBackendsSync
+	} else {
+		klog.V(2).Infof("Skip syncing instance groups")


log the identity of the ingress object that this is considering

syncState.ing

bowei · 2020-05-26T19:54:39Z

pkg/controller/controller.go

-		if err = updateAnnotations(lbc.ctx.KubeClient, ing.Name, ing.Namespace, ing.Annotations); err != nil {
+	// Only sync instance group when IG is used for this ingress
+	if len(nodePorts(ingSvcPorts)) > 0 {
+		if err := lbc.syncInstanceGroup(syncState.ing, ingSvcPorts); err != nil {


log which ingress this was looking at syncState.ing.

bowei · 2020-05-26T19:55:15Z

pkg/controller/controller.go

+
+	// TODO: Remove this after deprecation
+	if utils.IsGCEMultiClusterIngress(ing) {
+		klog.Warningf("kubemci is used for ingress %v+", ing)


What is with the +

bowei · 2020-05-26T19:57:09Z

pkg/controller/controller_test.go

@@ -176,6 +178,37 @@ func TestIngressSyncError(t *testing.T) {
 	}
 }

+// TestNegOnlyIngress asserts that `sync` will not create IG when there is only NEG backends for the ingress
+func TestNegOnlyIngress(t *testing.T) {


TestNEGOnlyIngress

bowei · 2020-05-26T19:57:54Z

pkg/controller/controller_test.go

+		Ports: []api_v1.ServicePort{{Port: 80}},
+	})
+	negAnnotation := annotations.NegAnnotation{Ingress: true}
+	svc.Annotations = map[string]string{}


combine

svc.Annotations = map[string]string{ annotations.NEGAnnotationKey: annotations.NegAnnotation{Ingress: true}.String(), }

bowei · 2020-05-26T19:59:00Z

pkg/controller/controller_test.go

+
+	ig, err := lbc.instancePool.Get(lbc.ctx.ClusterNamer.InstanceGroup(), fakeZone)
+	if err != nil && !utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+		t.Errorf("lbc.ctx.Cloud.ListInstanceGroups(%v) = %v, %v, want [], nil", fakeZone, ig, err)


want [], http.StatusNotFound

freehan · 2020-05-27T00:22:18Z

Fixed. Ready.

bowei · 2020-05-27T21:08:14Z

/lgtm
/approve

k8s-ci-robot · 2020-05-27T21:08:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, freehan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bowei,freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Cherrypick #1105 into Release-1.9

freehan assigned bowei, wojtek-t and rramkumar1 May 19, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2020

k8s-ci-robot requested review from MrHohn and rramkumar1 May 19, 2020 18:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2020

bowei reviewed May 19, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

freehan force-pushed the ig-bug-fix branch from c9d32c0 to 5780414 Compare May 19, 2020 18:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2020

bowei reviewed May 20, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020

freehan force-pushed the ig-bug-fix branch from a8973c1 to e3f5cd4 Compare May 20, 2020 22:36

bowei reviewed May 21, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

bowei reviewed May 21, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

bowei reviewed May 21, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

bowei reviewed May 21, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

freehan force-pushed the ig-bug-fix branch from e3f5cd4 to 106afb5 Compare May 21, 2020 21:03

bowei reviewed May 26, 2020

View reviewed changes

freehan added 2 commits May 26, 2020 17:21

Fix a bug where IG is synced for Ingress with NEG only backends.

235f1ca

Add unit test to validate if IG is created for NEG only ingress

a00ee2f

freehan force-pushed the ig-bug-fix branch from 106afb5 to a00ee2f Compare May 27, 2020 00:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2020

freehan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 27, 2020

k8s-ci-robot merged commit 26a9897 into kubernetes:master May 27, 2020

k8s-ci-robot added a commit that referenced this pull request May 27, 2020

Merge pull request #1121 from freehan/cherrypick

d35d927

Cherrypick #1105 into Release-1.9

freehan mentioned this pull request Aug 25, 2020

Ingress stil trying to add nodes to instance groups when using NEGs #1177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not sync IG when not necessary #1105

do not sync IG when not necessary #1105

freehan commented May 19, 2020

bowei commented May 19, 2020

wojtek-t commented May 19, 2020

freehan commented May 19, 2020

freehan commented May 19, 2020

bowei May 20, 2020

freehan May 20, 2020

bowei commented May 20, 2020

bowei commented May 20, 2020

freehan commented May 20, 2020

bowei May 21, 2020

bowei May 26, 2020

bowei May 27, 2020

bowei May 21, 2020

bowei commented May 21, 2020

freehan commented May 21, 2020

bowei May 26, 2020

bowei May 26, 2020

bowei May 26, 2020

bowei May 26, 2020

bowei May 26, 2020

bowei May 26, 2020

freehan commented May 27, 2020

bowei commented May 27, 2020

k8s-ci-robot commented May 27, 2020

do not sync IG when not necessary #1105

do not sync IG when not necessary #1105

Conversation

freehan commented May 19, 2020

bowei commented May 19, 2020

wojtek-t commented May 19, 2020

freehan commented May 19, 2020

freehan commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented May 20, 2020

bowei commented May 20, 2020

freehan commented May 20, 2020

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

bowei commented May 21, 2020

freehan commented May 21, 2020

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

freehan commented May 27, 2020

bowei commented May 27, 2020

k8s-ci-robot commented May 27, 2020