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

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

Merged
merged 1 commit into from Apr 21, 2015

Conversation

Projects
None yet
3 participants
@yujuhong
Copy link
Member

yujuhong commented Apr 17, 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.

This fixes #3948.

@googlebot googlebot added the cla: yes label Apr 17, 2015

@yujuhong yujuhong force-pushed the yujuhong:kill_pods branch from 8ae66fe to 9c8063e Apr 17, 2015

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

This comment has been minimized.

Copy link
@bgrant0607

bgrant0607 Apr 17, 2015

Member

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

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 20, 2015

Author Member

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 yujuhong:kill_pods branch 2 times, most recently from 3fa44ac to dd4527a Apr 20, 2015

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Apr 20, 2015

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]) {

This comment has been minimized.

Copy link
@bgrant0607

bgrant0607 Apr 21, 2015

Member

I would order the checks as follows:

  1. host
  2. phase
  3. readiness

but it shouldn't make a lot of difference.

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 21, 2015

Author Member

Done.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Apr 21, 2015

LGTM

@bgrant0607 bgrant0607 added the lgtm label Apr 21, 2015

Prioritize deleting the non-running pods when reducing replicas
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 yujuhong force-pushed the yujuhong:kill_pods branch from dd4527a to df2cbd4 Apr 21, 2015

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Apr 21, 2015

Addressed the comments and rebased.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Apr 21, 2015

Thanks. LGTM.

bgrant0607 added a commit that referenced this pull request Apr 21, 2015

Merge pull request #6992 from yujuhong/kill_pods
Prioritize deleting the non-running pods when reducing replicas

@bgrant0607 bgrant0607 merged commit 8ae4cb3 into kubernetes:master Apr 21, 2015

4 checks passed

Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 53.44%
Details

@yujuhong yujuhong deleted the yujuhong:kill_pods branch May 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.