Skip to content

move feature gate checks inside IsCriticalPod#66082

Merged
k8s-github-robot merged 1 commit intokubernetes:masterfrom
sjenning:fix-is-critical-checks
Aug 1, 2018
Merged

move feature gate checks inside IsCriticalPod#66082
k8s-github-robot merged 1 commit intokubernetes:masterfrom
sjenning:fix-is-critical-checks

Conversation

@sjenning
Copy link
Copy Markdown
Contributor

@sjenning sjenning commented Jul 11, 2018

Currently IsCriticalPod() calls throughout the code are protected by utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation).

However, with Pod Priority, this gate could be disabled which skips the priority check inside IsCriticalPod().

This PR moves the feature gate checking inside IsCriticalPod() and handles both situations properly.

@aveshagarwal @ravisantoshgudimetla @derekwaynecarr
/sig node
/sig scheduling
/king bug

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 11, 2018

@sjenning: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 11, 2018
@k8s-ci-robot k8s-ci-robot requested review from Random-Liu and k82cn July 11, 2018 18:43
@sjenning
Copy link
Copy Markdown
Contributor Author

sjenning commented Jul 11, 2018

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 11, 2018
@sjenning sjenning force-pushed the fix-is-critical-checks branch 2 times, most recently from 59e0be5 to 3969d2f Compare July 11, 2018 20:24
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2018
aveshagarwal
aveshagarwal reviewed Jul 11, 2018
View reviewed changes
pkg/kubelet/preemption/preemption_test.go Outdated
Copy link
Copy Markdown
Contributor

@aveshagarwal aveshagarwal Jul 11, 2018

Choose a reason for hiding this comment

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

curious how was it working before?

Copy link
Copy Markdown
Contributor Author

@sjenning sjenning Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

took me a second to figure it out. the IsCriticalPod() in sortPodsByQOS() was unprotected by the feature gate.

kubernetes/pkg/kubelet/preemption/preemption.go

Line 233 in 817869e

if !kubetypes.IsCriticalPod(pod) {

@sjenning sjenning force-pushed the fix-is-critical-checks branch from 3969d2f to f2a7654 Compare July 11, 2018 21:10
@aveshagarwal
Copy link
Copy Markdown
Contributor

aveshagarwal commented Jul 11, 2018

/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 11, 2018
@sjenning
Copy link
Copy Markdown
Contributor Author

sjenning commented Jul 12, 2018

/retest

k82cn
k82cn reviewed Jul 13, 2018
View reviewed changes
pkg/kubelet/types/pod_update.go
// to make admission and scheduling decisions.
func IsCriticalPod(pod *v1.Pod) bool {
return IsCritical(pod.Namespace, pod.Annotations) || (pod.Spec.Priority != nil && IsCriticalPodBasedOnPriority(*pod.Spec.Priority))
if utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
Copy link
Copy Markdown
Contributor

@k82cn k82cn Jul 13, 2018

Choose a reason for hiding this comment

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

We did not check priority feature gate before :) @dashpole , any background to share?

Copy link
Copy Markdown
Contributor

@dashpole dashpole Jul 13, 2018

Choose a reason for hiding this comment

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

Looks like that was an oversight when we added IsCriticalPodBasedOnPriority. We should add it here.

Copy link
Copy Markdown
Contributor

@k82cn k82cn Jul 17, 2018

Choose a reason for hiding this comment

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

That's great !

LGTM then :)

@sjenning
Copy link
Copy Markdown
Contributor Author

sjenning commented Jul 13, 2018

/assign @derekwaynecarr @mikedanese

@sjenning
Copy link
Copy Markdown
Contributor Author

sjenning commented Aug 1, 2018

/test pull-kubernetes-integration

@derekwaynecarr
Copy link
Copy Markdown
Member

derekwaynecarr commented Aug 1, 2018

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, derekwaynecarr, sjenning

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

k8s-github-robot commented Aug 1, 2018

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

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Aug 1, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 0a284c1 into kubernetes:master Aug 1, 2018
This was referenced Sep 13, 2018
Closed
Closed
Merged
k8s-ci-robot added a commit that referenced this pull request Sep 14, 2018
@k8s-ci-robot
Merge pull request #68601 from k82cn/automated-cherry-pick-of-#66082-#…
ed30179 
…64954-#68494-upstream-release-1.11

Automated cherry pick of #66082: move feature gate checks inside IsCriticalPod #64954: Added unschedulable and network-unavailable toleration. #68494: Added default tolerations for new pods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

3 more reviewers

@dashpole dashpole dashpole left review comments

@aveshagarwal aveshagarwal aveshagarwal left review comments

@k82cn k82cn k82cn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mikedanese mikedanese

@aveshagarwal aveshagarwal

@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-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sjenning @k8s-ci-robot @aveshagarwal @derekwaynecarr @k8s-github-robot @dashpole @k82cn @mikedanese