Skip to content

Prioritize deleting the non-running pods when reducing replicas #6992

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

Merged
merged 1 commit into from
Apr 21, 2015

Conversation

yujuhong
Copy link
Contributor

This changes instructs the replication controller to delete replicas in the
order of "unscheduled (pending)", "scheduled (pending)", and "scheduled
(running)" pods. This is less disruptive than deleting random pods.

This fixes #3948.

// Unassigned < assigned
if s[i].Spec.Host == "" && s[j].Spec.Host != "" {
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

This is great. You could also check readiness (not ready < ready). The endpoints controller check could be factored out into a function:
https://github.com/GoogleCloudPlatform/kubernetes/blob/e1b76b922b3f7aec73715cc53cb6f8e9180e5c26/pkg/service/endpoints_controller.go#L99

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Factored out the pod ready logic to api.IsPodReady. Added this new condition to pod sorting.

@bgrant0607 bgrant0607 self-assigned this Apr 17, 2015
@yujuhong yujuhong force-pushed the kill_pods branch 2 times, most recently from 3fa44ac to dd4527a Compare April 20, 2015 17:14
@yujuhong
Copy link
Contributor Author

Comments addressed in the new patch. Thanks for the suggestion!


func (s activePods) Less(i, j int) bool {
// Not ready < ready
if !api.IsPodReady(&s[i]) && api.IsPodReady(&s[j]) {
Copy link
Member

Choose a reason for hiding this comment

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

I would order the checks as follows:

  1. host
  2. phase
  3. readiness

but it shouldn't make a lot of difference.

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.

@bgrant0607
Copy link
Member

LGTM

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2015
This changes instructs the replication controller to delete replicas in the
order of "unscheduled (pending)", "scheduled (pending)", and "scheduled
(running)" pods. This is less disruptive than deleting random pods.
@yujuhong
Copy link
Contributor Author

Addressed the comments and rebased.

@bgrant0607
Copy link
Member

Thanks. LGTM.

bgrant0607 added a commit that referenced this pull request Apr 21, 2015
Prioritize deleting the non-running pods when reducing replicas
@bgrant0607 bgrant0607 merged commit 8ae4cb3 into kubernetes:master Apr 21, 2015
@yujuhong yujuhong deleted the kill_pods 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.

Kubernetes killing running pods instead of unassigned when reducing replicas
3 participants