Use NEG CRs for NEG Garbage Collection #1197

swetharepakula · 2020-07-29T18:25:56Z

Add finalizer to NEG CRs to ensure CR exists until NEG is deleted
during GC
When neg crd is enabled, GC will use neg crs, otherwise will fallback
to regular GC using the naming to determine which NEGs are to be
deleted

k8s-ci-robot · 2020-07-29T18:26:04Z

Hi @swetharepakula. 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.

freehan · 2020-07-29T21:32:47Z

/ok-to-test

freehan · 2020-07-29T22:58:32Z

pkg/neg/manager.go

@@ -370,6 +377,62 @@ func (manager *syncerManager) garbageCollectNEG() error {
 	return nil
 }

+// garbageCollectNEGWithCRD uses the NEG CRs and the svcPortMap to determine which NEGs
+// need to be garbage collected. NEGs that have a corresponding CR with a deletion
+// timestamp, don't exist in the svcPortMap or don't have a corresponding NEG in the


Complete the sentence ?

freehan · 2020-07-29T23:37:23Z

pkg/neg/manager.go

+	}
+	for _, cr := range deletionCandidates {
+		cr.Finalizers = []string{}
+		if cr.GetDeletionTimestamp().IsZero() {


probably need to send a delete call

freehan · 2020-07-29T23:38:15Z

pkg/neg/manager.go

+		}
+		if _, ok := deletionCandidates[neg.Name]; ok {
+			if err := manager.ensureDeleteNetworkEndpointGroup(neg.Name, key.Zone); err != nil {
+				return fmt.Errorf("failed to delete NEG %s in %s: %s", neg.Name, key.Zone, err)


cannot return here.
Instead it should record an event to the NEG CR if it is there

freehan · 2020-07-29T23:43:58Z

pkg/neg/manager.go

+// need to be garbage collected. NEGs that have a corresponding CR with a deletion
+// timestamp, don't exist in the svcPortMap or don't have a corresponding NEG in the
+// will all be garbage collected
+func (manager *syncerManager) garbageCollectNEGWithCRD() error {


Assumption is that NEG CR exists then NEG must not be fully GCed.
Try to report event to NEG CR if NEG cannot be GCed.

Case 1: NEG CR with deletion timestamp + Manager PortInfo Map does not have key

Delete NEG

Remove Finalizer

Case 2: NEG CR has deletion timestamp + Manager PortInfo Map has the key

Don't Remove Finalizer until it becomes to Case 1

Case 3: NEG CR does not have deletion timestamp + Manager PortInfoMap does not have key (controller is down when the service or annotation is removed)

Delete NEG

Delete NEG CR

Remove Finalzier

func A: Delete NEG
func B: Delete NEG CR (Delete and remove Finalzier)

For each deletion candidate (bool shouldDeleteNEG):

if shouldDeleteNEG, then try delete NEG

Delete NEG CR

Deletion candidate:

case 1: shouldDeleteNEG = true
case 2: shouldDeleteNEG= false
case 3: shouldDeleteNEG = true

freehan · 2020-07-29T23:57:00Z

pkg/neg/manager.go

+	}
+
+	var errList []error
+	for key, neg := range negList {


one corner to watch out:

cluster A -> try to create NEG foo. (NEG foo belongs cluster B)

when service or annotation got updated, and cluster A wants to GC NEG foo. Will it GC NEG foo?

check description or initialized condition

freehan

LGTM overall
just nits.

pkg/neg/manager.go

freehan · 2020-07-31T20:22:14Z

pkg/neg/manager.go

+		defer manager.mu.Unlock()
+		for _, portInfoMap := range manager.svcPortMap {
+			for _, portInfo := range portInfoMap {
+				// only keep in deletionCandidates if neg does not exist in svcPortMap


add a comment regarding the 2 corner cases:

NEG CR has deletionTimestamp but svcPortMap has the svc

NEG CR does not have deletionTImeStamp but svcPortMap does not has the svc.

- Add finalizer to NEG CRs to ensure CR exists until NEG is deleted during GC - When neg crd is enabled, GC will use neg crs, otherwise will fallback to regular GC using the naming to determine which NEGs are to be deleted

freehan · 2020-08-03T17:17:24Z

/lgtm
/approve

k8s-ci-robot · 2020-08-03T17:17:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [freehan]

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2020

k8s-ci-robot requested review from bowei and thockin July 29, 2020 18:26

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 Jul 29, 2020

swetharepakula force-pushed the neg-crd-gc branch from 0e4ecdc to a219968 Compare July 29, 2020 22:07

freehan reviewed Jul 29, 2020

View reviewed changes

swetharepakula force-pushed the neg-crd-gc branch 4 times, most recently from 65add2b to 49c862c Compare July 31, 2020 17:50

freehan reviewed Jul 31, 2020

View reviewed changes

Use NEG CRs for NEG Garbage Collection

0fb2ea4

- Add finalizer to NEG CRs to ensure CR exists until NEG is deleted during GC - When neg crd is enabled, GC will use neg crs, otherwise will fallback to regular GC using the naming to determine which NEGs are to be deleted

swetharepakula force-pushed the neg-crd-gc branch from 49c862c to 0fb2ea4 Compare July 31, 2020 21:32

k8s-ci-robot assigned freehan Aug 3, 2020

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

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

k8s-ci-robot merged commit c96b1d8 into kubernetes:master Aug 3, 2020

swetharepakula deleted the neg-crd-gc branch August 3, 2020 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use NEG CRs for NEG Garbage Collection #1197

Use NEG CRs for NEG Garbage Collection #1197

swetharepakula commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

freehan commented Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020 •

edited

Loading

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan Jul 29, 2020

freehan left a comment

freehan Jul 31, 2020

freehan commented Aug 3, 2020

k8s-ci-robot commented Aug 3, 2020

Use NEG CRs for NEG Garbage Collection #1197

Use NEG CRs for NEG Garbage Collection #1197

Conversation

swetharepakula commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

freehan commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan Jul 29, 2020 • 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

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Aug 3, 2020

k8s-ci-robot commented Aug 3, 2020

freehan Jul 29, 2020 •

edited

Loading