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

Added case on 'terminated-but-not-yet-deleted' for Admit. #48322

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Added case on 'terminated-but-not-yet-deleted' for Admit. #48322

merged 1 commit into from
Jul 12, 2017

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Jun 30, 2017 •
edited
Loading

What this PR does / why we need it:
Added case on 'terminated-but-not-yet-deleted' for Admit.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #47867

Release note:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 30, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 30, 2017
@k82cn
Copy link
Member Author

k82cn commented Jun 30, 2017

/assign

@k82cn
Copy link
Member Author

k82cn commented Jun 30, 2017

/unassign @sjpotter @feiskyer

Will re-assign to you after adding UT case.

@k82cn
Copy link
Member Author

k82cn commented Jun 30, 2017

cc/ @vishh @bsalamat @davidopp

@k82cn k82cn force-pushed the k8s_47867 branch 2 times, most recently from 97c2d21 to 2ba70b0 Compare June 30, 2017 07:13
@k82cn
Copy link
Member Author

k82cn commented Jun 30, 2017

/retest

bsalamat
bsalamat reviewed Jun 30, 2017
View reviewed changes
Copy link
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

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

My main concern is that I think the existing logic of Kubelet is correct. With your change, Kubelet would consider the terminated-but-not-yet-deleted pods as active and would account their resources when admitting a new pod. IMO we shouldn't account resources of terminated pods.

pkg/kubelet/kubelet_pods.go Outdated
@@ -743,6 +743,20 @@ func (kl *Kubelet) podIsTerminated(pod *v1.Pod) bool {
return status.Phase == v1.PodFailed || status.Phase == v1.PodSucceeded || (pod.DeletionTimestamp != nil && notRunning(status.ContainerStatuses))
}

// podIsFinished returns true if pod is in the finished state ("Failed" or "Succeeded").
func (kl *Kubelet) podIsFinished(pod *v1.Pod) bool {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I am not sure about podIsFinished. It is kind of confusing that we have both "podIsTerminated" and "podIsFinished". I don't have any better suggestion though.

pkg/kubelet/kubelet_pods.go Outdated
@@ -729,7 +729,7 @@ func (kl *Kubelet) getPullSecretsForPod(pod *v1.Pod) []v1.Secret {
return pullSecrets
}

// Returns true if pod is in the terminated state ("Failed" or "Succeeded").
// podIsTerminated returns true if pod is in the terminated state ("Failed", "Succeeded" or "DeletionTimestamp" not nil).
Copy link
Member

Choose a reason for hiding this comment

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

The new comment is not accurate. This method return true when "DeletionTimestamp" is not nil AND pod containers are not running.

pkg/kubelet/kubelet_pods.go Outdated
@@ -743,6 +743,20 @@ func (kl *Kubelet) podIsTerminated(pod *v1.Pod) bool {
return status.Phase == v1.PodFailed || status.Phase == v1.PodSucceeded || (pod.DeletionTimestamp != nil && notRunning(status.ContainerStatuses))
}

// podIsFinished returns true if pod is in the finished state ("Failed" or "Succeeded").
func (kl *Kubelet) podIsFinished(pod *v1.Pod) bool {
var status v1.PodStatus
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to declare this variable explicitly?

@k82cn
Copy link
Member Author

k82cn commented Jul 1, 2017 •
edited
Loading

@bsalamat , we should account terminated-but-not-yet-deleted for Admit, similar to scheduler. hm.. I think terminating is more accurate. But in kubelet, it has already considered this case by pod.DeletionTimestamp != nil && notRunning(status.ContainerStatuses).

I updated comments to the issue, and update this PR to add a case on terminating-but-not-yet-deleted pod.

@k82cn k82cn changed the title [WIP] Did not filter out terminated-but-not-yet-deleted pods for Admit. [WIP] Added case on 'terminated-but-not-yet-deleted' for Admit. Jul 1, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2017
@k82cn k82cn changed the title [WIP] Added case on 'terminated-but-not-yet-deleted' for Admit. Added case on 'terminated-but-not-yet-deleted' for Admit. Jul 1, 2017
@k82cn
Copy link
Member Author

k82cn commented Jul 2, 2017

/retest

@k82cn
Copy link
Member Author

k82cn commented Jul 2, 2017

/assign @sjpotter @feiskyer

@k82cn
Copy link
Member Author

k82cn commented Jul 2, 2017

/unassign

@bsalamat
Copy link
Member

bsalamat commented Jul 3, 2017

we should account terminated-but-not-yet-deleted for Admit, similar to scheduler.

I am not sure if this is the right approach. IMO, kubelet admission logic will become unnecessarily more restrictive after this PR.
@davidopp What do you think?

@k82cn
Copy link
Member Author

k82cn commented Jul 4, 2017

IMO, kubelet admission logic will become unnecessarily more restrictive after this PR.

do you mean the latest version?

@davidopp
Copy link
Member

davidopp commented Jul 5, 2017

The current version of this PR doesn't seem to change the kubelet admission logic -- maybe a commit is missing?

@k82cn
Copy link
Member Author

k82cn commented Jul 5, 2017

I think Bobby is talking about previous version :).

@bsalamat
Copy link
Member

bsalamat commented Jul 5, 2017

Yes, I was talking about the previous version of this PR. This one only improves a test.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2017
@k82cn
Copy link
Member Author

k82cn commented Jul 5, 2017

/retest

@k82cn
Copy link
Member Author

k82cn commented Jul 6, 2017

/assign @timstclair

@k82cn
Copy link
Member Author

k82cn commented Jul 6, 2017

@timstclair , would you help to approve that?

@k82cn
Copy link
Member Author

k82cn commented Jul 10, 2017

@timstclair , more comments?

@tallclair tallclair assigned tallclair and unassigned timstclair Jul 10, 2017
@tallclair
Copy link
Member

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsalamat, k82cn, tallclair
We suggest the following additional approver: timstclair

Assign the PR to them by writing /assign @timstclair in a comment when ready.

Associated issue: 47867

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k82cn
Copy link
Member Author

k82cn commented Jul 10, 2017

/retest

@k82cn
Copy link
Member Author

k82cn commented Jul 11, 2017

@tallclair , I think you need to update OWNER in kubelet :).

@tallclair tallclair added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2017
@k82cn
Copy link
Member Author

k82cn commented Jul 11, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 48402, 47203, 47460, 48335, 48322)

@k8s-github-robot k8s-github-robot merged commit d68e737 into kubernetes:master Jul 12, 2017
@k82cn k82cn deleted the k8s_47867 branch September 15, 2017 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsalamat bsalamat bsalamat left review comments

Assignees

@feiskyer feiskyer

@sjpotter sjpotter

@bsalamat bsalamat

@tallclair tallclair

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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
No milestone
Development

Successfully merging this pull request may close these issues.

Scheduler and kubelet should consider resources of terminated-but-not-yet-deleted pods to still be in use.
9 participants
@k82cn @bsalamat @davidopp @tallclair @k8s-github-robot @feiskyer @sjpotter @timstclair @k8s-ci-robot