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.

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

Kubelet: filter out terminated pods in SyncPods #7301

Merged
merged 1 commit into from Apr 24, 2015

Conversation

yujuhong
Copy link
Contributor

Once a pod reaches a terminated state (whether failed or succeeded), it should
not transit out ever again. Currently, kubelet relies on examining the dead
containers to verify that the container has already been run. This is fine
in most cases, but if the dead containers were garbage collected, kubelet may
falsely concluded that the pod has never been run. It would then try to restart
all the containers.

This change eliminates most of such possibilities by pre-filtering out the pods
in the final states before sending updates to per-pod workers.

@yujuhong
Copy link
Contributor Author

This fixes #7250

Note that this PR handles most cases, but not all:

  • If the containers were removed immediately after they died, even generating correct status could fail.
  • If kubelet restarts before writing the terminated status for a pod to the apiserver, it could still restart a terminated pod (even though it'd not be reflected via the pod phases).

I think it's reasonable to assume that containers should only be garbage collected after a reasonable amount of time. As for the restart, we'd need a checkpoint of kubelet to handle that smoothly.

One upside of this PR is that kubelet would stop wasting resources on terminated pods (examining containers, generating statuses and sending them to the apiserver). It would also avoid (potential) post-mortem status modifications.

@yujuhong
Copy link
Contributor Author

passed e2e

@yujuhong
Copy link
Contributor Author

/cc @dchen1107, @vmarmol

@dchen1107 dchen1107 self-assigned this Apr 24, 2015
// Pod phase progresses monotonically. Once a pod has reach a final state,
// it should never leave irregardless of the restart policy. The statuses
// of such pods should not be changed, and there is no need to sync them.
pods := kl.FilterOutTerminatedPods(allPods)
Copy link
Member

Choose a reason for hiding this comment

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

Please also add TODO to explain those uncovered cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dchen1107
Copy link
Member

LGTM overall except one nit.

@vmarmol
Copy link
Contributor

vmarmol commented Apr 24, 2015

LGTM

@dchen1107
Copy link
Member

LGTM and will merge once it is green.

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2015
Once a pod reaches a terminated state (whether failed or succeeded), it should
not transit out ever again. Currently, kubelet relies on examining the dead
containers to verify that the container has already been run. This is fine
in most cases, but if the dead containers were garbage collected, kubelet may
falsely concluded that the pod has never been run. It would then try to restart
all the containers.

This change eliminates most of such possibilities by pre-filtering out the pods
in the final states before sending updates to per-pod workers.
vmarmol added a commit that referenced this pull request Apr 24, 2015
Kubelet: filter out terminated pods in SyncPods
@vmarmol vmarmol merged commit d0288f7 into kubernetes:master Apr 24, 2015
@yujuhong yujuhong deleted the no_resurrection branch May 8, 2015 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants