Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make NodeController recognize deletion tombstones #34786

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Make NodeController recognize deletion tombstones #34786

merged 1 commit into from
Oct 14, 2016

Conversation

davidopp
Copy link
Member

@davidopp davidopp commented Oct 14, 2016 •
edited by jessfraz
Loading

cc/ @lavalamp @gmarek

Make NodeController recognize deletion tombstones

This change is Reviewable

@davidopp davidopp added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 14, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Oct 14, 2016
wojtek-t
wojtek-t reviewed Oct 14, 2016
View reviewed changes
pkg/controller/node/nodecontroller.go
@@ -236,6 +236,7 @@ func NewNodeController(
podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: nc.maybeDeleteTerminatingPod,
UpdateFunc: func(_, obj interface{}) { nc.maybeDeleteTerminatingPod(obj) },
DeleteFunc: nc.maybeDeleteTerminatingPod,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this line. If we observed deletion of pod, why we would even want to try delete it again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I had added a pending comment that I wasn't sure this line was right, but I forgot to hit "review changes." (After un-learning the need to "send changes" now I am having trouble adapting to needing to do it...). I'll delete it.

@davidopp
Copy link
Member Author

OK, PTAL.

@wojtek-t
Copy link
Member

Can you please add release-note? (I think we need it for things to be cherrypicked).

LGTM

@davidopp davidopp 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 Oct 14, 2016
@davidopp
Copy link
Member Author

I've set release-note label just now, which will use the issue title as the release note, IIUC?

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit cd5779a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-cherrypick-bot
Copy link

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.

@davidopp davidopp added this to the v1.4 milestone Oct 14, 2016
@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Oct 14, 2016
@k8s-github-robot
Copy link

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

@gmarek
Copy link
Contributor

gmarek commented Oct 14, 2016

Actually I'm not a fan of this PR - I'd rather do it directly when handler is registered.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0b74e39 into kubernetes:master Oct 14, 2016
@gmarek gmarek mentioned this pull request Oct 14, 2016
Merged
@jessfraz jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 14, 2016
jessfraz added a commit that referenced this pull request Oct 14, 2016
@jessfraz
Merge pull request #34806 from gmarek/automated-cherry-pick-of-#34786…
ecaf61d 
…-upstream-release-1.4

Automated cherry pick of #34786
@k8s-cherrypick-bot
Copy link

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

@davidopp davidopp mentioned this pull request Oct 14, 2016
Merged
@davidopp davidopp mentioned this pull request Nov 2, 2016
Closed
8 tasks
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@jessfraz
Merge pull request kubernetes#34806 from gmarek/automated-cherry-pick…
9848126 
…-of-#34786-upstream-release-1.4

Automated cherry pick of kubernetes#34786
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

Assignees

@gmarek gmarek

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@davidopp @wojtek-t @k8s-ci-robot @k8s-cherrypick-bot @k8s-github-robot @gmarek @jessfraz @googlebot