Skip to content

Delete pods assigned to not existing nodes. #73401

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
Merged
k8s-ci-robot merged 1 commit into from
Jan 30, 2019
Merged

Delete pods assigned to not existing nodes. #73401

k8s-ci-robot merged 1 commit into from
Jan 30, 2019

Conversation

@krzysztof-jastrzebski
Copy link
Contributor

@krzysztof-jastrzebski krzysztof-jastrzebski commented Jan 28, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
PR adds deleting pods created by DaemonSet assigned to not existing nodes.
Which issue(s) this PR fixes:
Fixes #71349

Does this PR introduce a user-facing change?:

Adds deleting pods created by DaemonSet assigned to not existing nodes.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 28, 2019
@krzysztof-jastrzebski
Copy link
Contributor Author

krzysztof-jastrzebski commented Jan 28, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 28, 2019
@krzysztof-jastrzebski
Copy link
Contributor Author

krzysztof-jastrzebski commented Jan 28, 2019

/sig apps

@krzysztof-jastrzebski
Copy link
Contributor Author

krzysztof-jastrzebski commented Jan 28, 2019

/test pull-kubernetes-node-e2e

@krzysztof-jastrzebski
Copy link
Contributor Author

krzysztof-jastrzebski commented Jan 28, 2019

/test pull-kubernetes-node-e2e
/test pull-kubernetes-e2e-gce

@krzysztof-jastrzebski
Copy link
Contributor Author

krzysztof-jastrzebski commented Jan 28, 2019

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-node-e2e

@k82cn
Copy link
Contributor

k82cn commented Jan 29, 2019

/assign

k82cn
k82cn reviewed Jan 29, 2019
View reviewed changes
k82cn
k82cn reviewed Jan 29, 2019
View reviewed changes
pkg/controller/daemon/daemon_controller.go

// getPodsWithoutNode returns list of pods assigned to not existing nodes.
func getPodsWithoutNode(
runningNodesList []*v1.Node, nodeToDaemonPods map[string][]*v1.Pod) []string {
Copy link
Contributor

@k82cn k82cn Jan 29, 2019

Choose a reason for hiding this comment

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

move to a single line :)

@k82cn
Copy link
Contributor

k82cn commented Jan 29, 2019

LGTM ; let's get it merged when CI's happy :)

@k82cn
Copy link
Contributor

k82cn commented Jan 29, 2019

/retest

@k82cn
Copy link
Contributor

k82cn commented Jan 30, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, krzysztof-jastrzebski

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019
@k8s-ci-robot k8s-ci-robot merged commit f787705 into kubernetes:master Jan 30, 2019
@mikedanese
Copy link
Member

mikedanese commented Jan 30, 2019

isn't this smoothing that the node controller should handle in general? was the problem that they were critical pods?

@seh
Copy link
Contributor

seh commented Jan 30, 2019

These pods weren't assigned to nodes by the scheduler yet; they were created with affinity that will only allow them to be placed on a single node—in each case, a node that no longer exists.

This happens with pods regardless of whether they're critical or not. We see it happen with pods that need to wait until the node is ready, where the node never became ready before it got deleted.

@k82cn
Copy link
Contributor

k82cn commented Jan 31, 2019 •
edited
Loading

isn't this smoothing that the node controller should handle in general? was the problem that they were critical pods?

PodGC will check pod's node name to clean them up. After ScheduleDaemonPod (schedule daemonset pods by scheduler), DemonSetController will only append node affinity with target node and let scheduler to do binding. So it's better to handle it in DaemonSetController.

BTW, @krzysztof-jastrzebski , would you help to add feature-gate checking there? ScheduleDaemonPod is still beta now; if anyone disable it, we don-t need to check all-nodes and all-pods :)

This was referenced Feb 4, 2019
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Feb 12, 2019
@k8s-ci-robot
Merge pull request #73710 from krzysztof-jastrzebski/automated-cherry…
fe71243 
…-pick-of-#73401-#73606-upstream-release-1.12

Automated cherry pick of #73401: Delete pods assigned to not existing nodes. #73606: Deleting pods assigned to not existing nodes only if
k8s-ci-robot added a commit that referenced this pull request Feb 22, 2019
@k8s-ci-robot
Merge pull request #73709 from krzysztof-jastrzebski/automated-cherry…
2013533 
…-pick-of-#73401-#73606-upstream-release-1.13

Automated cherry pick of #73401: Delete pods assigned to not existing nodes. #73606: Deleting pods assigned to not existing nodes only if
@feiskyer feiskyer mentioned this pull request Mar 20, 2019
Closed
4 tasks
@bsalamat bsalamat mentioned this pull request Apr 2, 2019
Closed
@krzysztof-jastrzebski krzysztof-jastrzebski mentioned this pull request Apr 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikedanese mikedanese Awaiting requested review from mikedanese

1 more reviewer

@k82cn k82cn k82cn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@k82cn k82cn

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DaemonSet Controller doesn't delete orphaned pods

5 participants

@krzysztof-jastrzebski @k82cn @k8s-ci-robot @mikedanese @seh