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

Task 3: Add MemoryPressure toleration for no BestEffort pod. #50180

Merged
merged 1 commit into from
Aug 14, 2017
Merged

Task 3: Add MemoryPressure toleration for no BestEffort pod. #50180

merged 1 commit into from
Aug 14, 2017

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Aug 5, 2017 •
edited
Loading

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

Release note:

After 1.8, admission controller will add 'MemoryPressure' toleration to Guaranteed and Burstable pods.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 5, 2017
@k82cn k82cn mentioned this pull request Aug 5, 2017
Closed
7 tasks
@k82cn
Copy link
Member Author

k82cn commented Aug 5, 2017

/cc @gmarek

gmarek
gmarek reviewed Aug 7, 2017
View reviewed changes
plugin/pkg/admission/podtolerationrestriction/admission.go Outdated
@@ -162,6 +166,23 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
}
}

if utilfeature.DefaultFeatureGate.Enabled(features.TaintNodesByCondition) {
Copy link
Contributor

@gmarek gmarek Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Similarly as in other PR - I think it's fine to add those tolerations even if feature is not enabled. We do this for NotReady taints.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

gmarek
gmarek reviewed Aug 7, 2017
View reviewed changes
plugin/pkg/admission/podtolerationrestriction/admission_test.go
@@ -148,6 +224,10 @@ func TestPodAdmission(t *testing.T) {
informerFactory.Core().InternalVersion().Namespaces().Informer().GetStore().Update(namespace)

handler.pluginConfig = &pluginapi.Configuration{Default: test.defaultClusterTolerations, Whitelist: test.clusterWhitelist}
pod := test.pod
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this explicitly to all test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@k82cn k82cn changed the title Add MemoryPressure/DiskPressure toleration for no BestEffort pod. Task 3: Add MemoryPressure/DiskPressure toleration for no BestEffort pod. Aug 8, 2017
@gmarek
Copy link
Contributor

gmarek commented Aug 8, 2017

/lgtm

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

k82cn commented Aug 8, 2017

@derekwaynecarr , can you approve that ?

derekwaynecarr
derekwaynecarr reviewed Aug 9, 2017
View reviewed changes
plugin/pkg/admission/podtolerationrestriction/admission.go
@@ -162,6 +164,21 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
}
}

if qoshelper.GetPodQOS(pod) != api.PodQOSBestEffort {
Copy link
Member

Choose a reason for hiding this comment

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

we need to ensure that this admission controller is run after all resource defaulting controllers.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm... , do you means this admission controller need to be the latest one in apiserver's args?

Copy link
Member

Choose a reason for hiding this comment

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

its a doc issue. you basically will want this controller to be the one that appears right before ResourceQuota

derekwaynecarr
derekwaynecarr suggested changes Aug 9, 2017
View reviewed changes
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

i think the disk pressure toleration is more naunced.

i am fine with the memory pressure toleration.

plugin/pkg/admission/podtolerationrestriction/admission.go Outdated
Effect: api.TaintEffectNoSchedule,
},
{
Key: algorithm.TaintNodeDiskPressure,
Copy link
Member

Choose a reason for hiding this comment

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

at this time, cpu and memory are the only supported qos compute resources.

i think application of this toleration for disk pressure needs to be more nuanced.

/cc @vishh @jingxu97

Copy link
Member Author

Choose a reason for hiding this comment

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

@derekwaynecarr , thanks very much for your input. I also found this when handling taint for scheduler: the scheduler will filter out disk pressure nodes, but check QoS for memory pressure. Will remove DiskPressure toleration in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 'DiskPressure' toleration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vishh , @derekwaynecarr , is latest PR OK for you?

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017
@k82cn k82cn changed the title Task 3: Add MemoryPressure/DiskPressure toleration for no BestEffort pod. Task 3: Add MemoryPressure toleration for no BestEffort pod. Aug 10, 2017
vishh
vishh reviewed Aug 10, 2017
View reviewed changes
Copy link
Contributor

@vishh vishh left a comment

Choose a reason for hiding this comment

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

The current behavior for DiskPressure where no pods will be allowed on a node should continue to exist. So +1 on the decision of not tolerating DiskPressure for any pods.

plugin/pkg/admission/podtolerationrestriction/admission.go
@@ -162,6 +164,21 @@ func (p *podTolerationsPlugin) Admit(a admission.Attributes) error {
}
}

if qoshelper.GetPodQOS(pod) != api.PodQOSBestEffort {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1.

@k82cn
Copy link
Member Author

k82cn commented Aug 10, 2017

/retest

1 similar comment
@gmarek
Copy link
Contributor

gmarek commented Aug 10, 2017

/retest

@gmarek
Copy link
Contributor

gmarek commented Aug 10, 2017

it looks good to me on my side, but @derekwaynecarr had good comment about order of admission controllers (which I don't know anything about).

@derekwaynecarr
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, gmarek, k82cn

Associated issue: 42001

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

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f8eed14 into kubernetes:master Aug 14, 2017
@k82cn k82cn deleted the k8s_42001-2 branch May 16, 2022 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishh vishh vishh left review comments

@gmarek gmarek gmarek left review comments

@derekwaynecarr derekwaynecarr derekwaynecarr approved these changes

Assignees

@gmarek gmarek

@derekwaynecarr derekwaynecarr

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@k82cn @gmarek @derekwaynecarr @k8s-github-robot @vishh @k8s-ci-robot