Ensure virtual nodes aren't stranded in GC graph #57503

liggitt · 2017-12-21T07:59:25Z

See #56121 (comment) for details on the sequence of events that can lead to virtual nodes getting stranded in the graph

Fixed garbage collection hang

(a branch with a commit that reliably triggers the cascading deletion test failure is at https://github.com/liggitt/kubernetes/commits/gc-debug-cascading... it's not easily made into a permanent test case because it only works when that test is run in isolation, and requires plumbing test hooks deep into the watch cache layer)

liggitt · 2017-12-21T08:01:34Z

@kubernetes/sig-api-machinery-pr-reviews

deads2k · 2017-12-21T14:12:32Z

pkg/controller/garbagecollector/garbagecollector.go

@@ -259,6 +259,12 @@ func (gc *GarbageCollector) attemptToDeleteWorker() bool {
 		}
 		// retry if garbage collection of an object failed.
 		gc.attemptToDelete.AddRateLimited(item)
+	} else if !n.isObserved() {
+		// requeue if item hasn't been observed yet.
+		// otherwise a virtual node for an item added/removed during a watch outage can get orphaned.


Please don't use orphaned. That does other things in this area.

reworded comment

deads2k · 2017-12-21T14:42:58Z

pkg/controller/garbagecollector/graph_builder.go

@@ -588,6 +592,9 @@ func (gb *GraphBuilder) processGraphChanges() bool {
 	glog.V(5).Infof("GraphBuilder process object: %s/%s, namespace %s, name %s, uid %s, event type %v", event.gvk.GroupVersion().String(), event.gvk.Kind, accessor.GetNamespace(), accessor.GetName(), string(accessor.GetUID()), event.eventType)
 	// Check if the node already exsits
 	existingNode, found := gb.uidToNode.Read(accessor.GetUID())
+	if found {
+		existingNode.markObserved()


Ask discussed on slack, this is only safe because of callsite behavior. The method and workqueue are private and its not unusual in this area of code, but it is comment worthy.

This is what triggers marking the node observed in the "good" case when the informer sees the object.

added comments, and tightened up call sites to only expose enqueuing virtual delete events into graphChanges outside the normal informer paths

deads2k · 2017-12-21T14:43:11Z

lgtm

deads2k · 2017-12-21T14:43:20Z

/approve

liggitt · 2017-12-21T19:00:43Z

comments addressed

jennybuckley · 2017-12-21T21:09:13Z

cc @jennybuckley

BenTheElder · 2017-12-23T00:12:51Z

Can we make this one happen? (regarding the milestone spam above)
Also can / should we cherry pick this pack to previous releases?

k8s-cherrypick-bot · 2017-12-25T08:58:18Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

k8s-github-robot · 2017-12-31T08:20:44Z

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@caesarxuchao @ironcladlou @liggitt

Important: This pull request was missing labels required for the v1.9 milestone for more than 3 days:

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

k8s-cherrypick-bot · 2017-12-31T08:22:19Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

deads2k · 2018-01-02T15:07:45Z

/lgtm

k8s-ci-robot · 2018-01-02T15:07:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

Associated issue: #56121

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/controller/garbagecollector/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-02T15:08:10Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-ci-robot · 2018-01-02T15:43:05Z

@liggitt: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`df60789`	link	`/test pull-kubernetes-verify`
pull-kubernetes-bazel-test	`df60789`	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-github-robot · 2018-01-02T15:51:29Z

Automatic merge from submit-queue (batch tested with PRs 57735, 57503). If you want to cherry-pick this change to another branch, please follow the instructions here.

…3-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #57503: Ensure dependents are added to virtual node before Cherry pick of #57503 on release-1.9. #57503: Ensure dependents are added to virtual node before

k8s-cherrypick-bot · 2018-01-03T07:19:24Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Ensure dependents are added to virtual node before attemptToDelete

a7c7da7

liggitt mentioned this pull request Dec 21, 2017

TestCascadingDeletion flaky [timed out waiting for the condition] #56121

Closed

liggitt assigned ironcladlou and caesarxuchao Dec 21, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 21, 2017

liggitt added this to the v1.9 milestone Dec 21, 2017

liggitt added the cherrypick-candidate label Dec 21, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 21, 2017

liggitt added kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Dec 21, 2017

k8s-github-robot added the milestone/incomplete-labels label Dec 21, 2017

deads2k reviewed Dec 21, 2017

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2017

liggitt force-pushed the gc-virtual-node-fix branch from 74b8db2 to 4d06356 Compare December 21, 2017 18:51

liggitt changed the title ~~Ensure virtual nodes aren't orphaned in GC graph~~ Ensure virtual nodes aren't stranded in GC graph Dec 21, 2017

Requeue unobserved nodes in attemptToDelete

df60789

liggitt force-pushed the gc-virtual-node-fix branch from 4d06356 to df60789 Compare December 21, 2017 19:00

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels Dec 25, 2017

k8s-github-robot removed this from the v1.9 milestone Dec 25, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 25, 2017

liggitt added this to the v1.9 milestone Dec 28, 2017

liggitt added the cherrypick-candidate label Dec 28, 2017

k8s-github-robot added milestone/incomplete-labels and removed milestone/removed labels Dec 28, 2017

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels Dec 31, 2017

k8s-github-robot removed this from the v1.9 milestone Dec 31, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 31, 2017

k8s-ci-robot assigned deads2k Jan 2, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2018

k8s-github-robot merged commit ff58401 into kubernetes:master Jan 2, 2018

liggitt mentioned this pull request Jan 2, 2018

Automated cherry pick of #57503: Ensure dependents are added to virtual node before #57755

Merged

liggitt added cherrypick-candidate and removed milestone/removed labels Jan 2, 2018

liggitt added this to the v1.9 milestone Jan 2, 2018

liggitt deleted the gc-virtual-node-fix branch January 3, 2018 04:06

k8s-cherrypick-bot removed the cherrypick-candidate label Jan 3, 2018

mborsz mentioned this pull request Nov 20, 2020

Increase supported pod deletion rate #96752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure virtual nodes aren't stranded in GC graph #57503

Ensure virtual nodes aren't stranded in GC graph #57503

liggitt commented Dec 21, 2017 •

edited

liggitt commented Dec 21, 2017

deads2k Dec 21, 2017

liggitt Dec 21, 2017

deads2k Dec 21, 2017

liggitt Dec 21, 2017

deads2k commented Dec 21, 2017

deads2k commented Dec 21, 2017

liggitt commented Dec 21, 2017

jennybuckley commented Dec 21, 2017

BenTheElder commented Dec 23, 2017 •

edited

k8s-cherrypick-bot commented Dec 25, 2017

k8s-github-robot commented Dec 31, 2017

k8s-cherrypick-bot commented Dec 31, 2017

deads2k commented Jan 2, 2018

k8s-ci-robot commented Jan 2, 2018

k8s-github-robot commented Jan 2, 2018

k8s-ci-robot commented Jan 2, 2018 •

edited

k8s-github-robot commented Jan 2, 2018

k8s-cherrypick-bot commented Jan 3, 2018

Ensure virtual nodes aren't stranded in GC graph #57503

Ensure virtual nodes aren't stranded in GC graph #57503

Conversation

liggitt commented Dec 21, 2017 • edited

liggitt commented Dec 21, 2017

deads2k Dec 21, 2017

Choose a reason for hiding this comment

liggitt Dec 21, 2017

Choose a reason for hiding this comment

deads2k Dec 21, 2017

Choose a reason for hiding this comment

liggitt Dec 21, 2017

Choose a reason for hiding this comment

deads2k commented Dec 21, 2017

deads2k commented Dec 21, 2017

liggitt commented Dec 21, 2017

jennybuckley commented Dec 21, 2017

BenTheElder commented Dec 23, 2017 • edited

k8s-cherrypick-bot commented Dec 25, 2017

k8s-github-robot commented Dec 31, 2017

k8s-cherrypick-bot commented Dec 31, 2017

deads2k commented Jan 2, 2018

k8s-ci-robot commented Jan 2, 2018

k8s-github-robot commented Jan 2, 2018

k8s-ci-robot commented Jan 2, 2018 • edited

k8s-github-robot commented Jan 2, 2018

k8s-cherrypick-bot commented Jan 3, 2018

liggitt commented Dec 21, 2017 •

edited

BenTheElder commented Dec 23, 2017 •

edited

k8s-ci-robot commented Jan 2, 2018 •

edited