Refactor Backend GC #810

spencerhance · 2019-08-01T21:00:54Z

Refactor backends GC so that we don't need ListAllBackendServices() anymore. That way, if one of the api calls fails, it won't block the others.

k8s-ci-robot · 2019-08-01T21:01:02Z

Hi @spencerhance. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rramkumar1 · 2019-08-02T13:29:05Z

/ok-to-test

spencerhance · 2019-08-04T21:56:42Z

/assign @bowei

bowei · 2019-08-03T00:02:21Z

pkg/backends/syncer.go

+		}
+		ilbBackends, err := s.backendPool.List(key, lbfeatures.L7ILBVersion(lbfeatures.BackendService))
+		if err != nil {
+			return fmt.Errorf("error listing L7-ILB backends: %v", err)


More accurate to say "Error listing regional backends: %v"

pkg/backends/syncer_test.go

bowei · 2019-08-07T22:10:29Z

pkg/backends/syncer_test.go

+		if _, err := composite.GetBackendService(fakeGCE, key, features.VersionFromServicePort(&sp)); err != nil {
+			return fmt.Errorf("Expected to find backend for port %v, err: %v", sp.NodePort, err)
+		}
+	}


Don't we need to check things in p.all not in p.existing don't exist?

bowei · 2019-08-07T22:34:33Z

pkg/backends/syncer_test.go

+		}
+		// Make sure it was deleted
+		if len(p.all) == beginLen {
+			return fmt.Errorf("Error: %v does not exist in p.all", dp)


Error: is redundant -- fmt.Errof("%v does not exist in p.all", dp)

bowei · 2019-08-07T22:34:50Z

pkg/backends/syncer_test.go

+// portset helps keep track of service ports during GC tests
+type portset struct {
+	all      []utils.ServicePort
+	existing []utils.ServicePort


might want to make this a map

bowei · 2019-08-07T22:36:07Z

pkg/backends/syncer_test.go

+
+// Delete ports from existing that are not in 'all'
+func (p *portset) gc() {
+	p.existing = p.all


p.existing = []utils.ServicePort{}? It's kind of strange for gc() to /add/ stuff

Ended up removing gc() since I think it's less confusing to just use add() and delete()

bowei · 2019-08-07T22:36:24Z

pkg/backends/syncer_test.go

+		for i, sp := range p.all {
+			if dp == sp {
+				// Delete it
+				p.all = append(p.all[:i], p.all[i:]...)


Why does all change? doesn't it never change?

bowei · 2019-08-07T22:39:23Z

pkg/backends/syncer_test.go

-	if err := syncer.Sync(svcNodePorts); err != nil {
-		t.Fatalf("Expected syncer to add backends with error, err: %v", err)
+	if err := syncer.Sync(ps.all); err != nil {
+		t.Fatalf("error syncing backends: %v", err)


t.Fatalf("syncer.Sync(%+v) = %v; want nil", ps.all, err)

You will want to follow this format for most of the unit test checks

bowei · 2019-08-08T18:21:47Z

pkg/backends/syncer.go

+		if err != nil {
+			return fmt.Errorf("error GCing regional Backends: %v", err)
+		}
+	}


this is a TODO, but we may want to consider continuing on error so a single error does not block all GC operations

tracking here: #797

bowei · 2019-08-12T06:06:36Z

pkg/backends/syncer_test.go

+// portset helps keep track of service ports during GC tests
+type portset struct {
+	// all represents the set all of service ports in the test
+	all map[utils.ServicePort]struct{}


usually sets are represented by map[T]bool

then map[X] == false is natural

done. Also i'm a fan, I didn't realize how go handled keys that aren't present.

bowei · 2019-08-12T06:07:54Z

pkg/backends/syncer_test.go

+			return fmt.Errorf("Error creating key for backend service %s: %v", beName, err)
+		}
+
+		// Make sure it exists


comment is probably redundant, but if you are going to put this, put it inside the brace so that the comment for the different cases are at the same indent level.

bowei · 2019-08-12T06:09:03Z

pkg/backends/syncer_test.go

+		// Make sure it exists
+		if found {
+			if _, err := composite.GetBackendService(fakeGCE, key, features.VersionFromServicePort(&sp)); err != nil {
+				return fmt.Errorf("Expected to find backend for port %+v, err: %v", sp.NodePort, err)


"backend for port %+v should exist, but got %v"
"backend for port %+v should not exist, but get %v"

bowei · 2019-08-12T06:09:47Z

pkg/backends/syncer_test.go

+	return &ps
+}
+
+func (p *portset) existingSlice() []utils.ServicePort {


existingPorts

bowei · 2019-08-12T06:12:00Z

pkg/healthchecks/healthchecks.go

 	if err != nil {
 		return fmt.Errorf("Error converting newHC to composite: %v", err)
 	}
-	key, err := composite.CreateKey(cloud, newHC.Name, features.L7ILBScope())
+	key, err := composite.CreateKey(cloud, mergedHC.Name, meta.Regional)


is there a reason why we went back to meta.Regional?

I thought it seemed redundant but I reverted it back

bowei · 2019-08-12T06:12:49Z

pkg/healthchecks/healthchecks.go

@@ -314,7 +313,17 @@ func (h *HealthChecks) getILB(name string) (*HealthCheck, error) {
 	if err != nil {
 		return nil, err
 	}
-	return NewHealthCheck(alphaHC)
+
+	newHC, err := NewHealthCheck(alphaHC)


call alphaHC gceHC and you won't have to change the name again when the version changes.

- Refactor backends GC so that we don't need ListAllBackendServices() anymore. That way, if one of the api calls fails, it won't block the others. - Refactor backends GC unit tests - Fix a ILB health check update() bug

bowei · 2019-08-12T20:34:17Z

/lgtm
/approve

k8s-ci-robot · 2019-08-12T20:34:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

bowei · 2019-08-13T05:11:17Z

looks like the CI had issues.

spencerhance · 2019-08-13T21:00:30Z

/retest

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 1, 2019

k8s-ci-robot requested review from MrHohn and rramkumar1 August 1, 2019 21:01

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2019

k8s-ci-robot assigned bowei Aug 4, 2019

bowei reviewed Aug 5, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2019

spencerhance force-pushed the l7-ilb-backends-gc branch from ba8ffd7 to 48a133c Compare August 6, 2019 01:47

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2019

spencerhance force-pushed the l7-ilb-backends-gc branch 3 times, most recently from 05a57f1 to 6ff08ff Compare August 6, 2019 22:34

bowei reviewed Aug 8, 2019

View reviewed changes

bowei reviewed Aug 12, 2019

View reviewed changes

spencerhance force-pushed the l7-ilb-backends-gc branch from 045c134 to d78aecc Compare August 12, 2019 20:28

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2019

Refactor GC backends

462b73c

- Refactor backends GC so that we don't need ListAllBackendServices() anymore. That way, if one of the api calls fails, it won't block the others. - Refactor backends GC unit tests - Fix a ILB health check update() bug

spencerhance force-pushed the l7-ilb-backends-gc branch from d78aecc to 462b73c Compare August 12, 2019 20:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2019

k8s-ci-robot merged commit 69c12a6 into kubernetes:master Aug 13, 2019

spencerhance deleted the l7-ilb-backends-gc branch August 13, 2019 21:21

Refactor Backend GC #810

Refactor Backend GC #810

Conversation

spencerhance commented Aug 1, 2019 • edited Loading

k8s-ci-robot commented Aug 1, 2019

rramkumar1 commented Aug 2, 2019

spencerhance commented Aug 4, 2019

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

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

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

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 Aug 12, 2019

k8s-ci-robot commented Aug 12, 2019

bowei commented Aug 13, 2019

spencerhance commented Aug 13, 2019

spencerhance commented Aug 1, 2019 •

edited

Loading