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

Changed IsCriticalPod to return true in case of static pods #80491

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

hpandeycodeit
Copy link

@hpandeycodeit hpandeycodeit commented Jul 23, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

Currently, If the static pods is critical, the kubelet will not realize it, since it's priority value determines whether the pod is critical or not. So SIG Node decided to consider all static pods as critical regardless of their PodSpec.PriorityClassName.

Which issue(s) this PR fixes:

Fixes #80489

Does this PR introduce a user-facing change?:

Kubelet considers all static pods as critical. Static pods pass kubelet admission even if a node does not have enough resources. Users must ensure that they account for resources when creating static pods.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 23, 2019
@hpandeycodeit
Copy link
Author

/sig scheduling
/sig node

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/node Categorizes an issue or PR as relevant to SIG Node. area/kubelet and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 23, 2019
@hpandeycodeit
Copy link
Author

/retest

Copy link
Contributor

@draveness draveness left a comment

Choose a reason for hiding this comment

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

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 24, 2019
Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

Thanks for this pr :)

I'm curious to see the results of the test suite after running the two hack scripts below. You may have an import cycle, in which case we may need to rethink the approach slightly. We'll know for sure after re-running the test suite after running the hack scripts.

@@ -23,6 +23,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilfeature "k8s.io/apiserver/pkg/util/feature"
kubeapi "k8s.io/kubernetes/pkg/apis/core"
kubepod "k8s.io/kubernetes/pkg/kubelet/pod"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run ./hack/update-bazel.sh to add this new dependency to the proper BUILD.bazel file.

Copy link
Author

@hpandeycodeit hpandeycodeit Jul 24, 2019

Choose a reason for hiding this comment

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

I ran the above hack scripts. Doesn't look like it's helping with the import cycle though :(

@@ -146,6 +147,9 @@ func (sp SyncPodType) String() string {
// or equal to SystemCriticalPriority. Both the default scheduler and the kubelet use this function
// to make admission and scheduling decisions.
func IsCriticalPod(pod *v1.Pod) bool {
if kubepod.IsStaticPod(pod){
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run ./hack/update-gofmt.sh :)

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.

Looks good. You may have to move IsCriticalPod to pkg/kubelet/pod, or another similar move to remove the circular dependency.
Also, please make sure to add adequate tests to cover the new logic.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 24, 2019
@hpandeycodeit
Copy link
Author

circular dependency

Thanks @bsalamat! I made the needed change to remove the import cycle.

Name: "pod5",
Namespace: "kube-system",
Annotations: map[string]string{
"scheduler.alpha.kubernetes.io/critical-pod": "file",
Copy link
Member

Choose a reason for hiding this comment

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

If you meant to add a static pod, the key should have been ConfigSourceAnnotationKey. What you have used is the deprecated critical annotation key.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 25, 2019
@bsalamat bsalamat added this to the v1.16 milestone Jul 25, 2019
@hpandeycodeit
Copy link
Author

/test pull-kubernetes-e2e-gce-100-performance

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.

/lgtm

Please go ahead and squash commits.
Thanks, @hpandeycodeit!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019
@hpandeycodeit
Copy link
Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

/lgtm

/assign @dchen1107
for reviewing and approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2019
@derekwaynecarr
Copy link
Member

I am not necessarily adverse to this. I know I was out of the SIG node meeting where this was discussed as a result of PTO. I am wondering why the static pod author is not able to set priorityClassName on their manifest to system-cluster-critical ? we have been doing this, and i had not seen an issue with that approach.

@bsalamat
Copy link
Member

bsalamat commented Aug 6, 2019

I am not necessarily adverse to this. I know I was out of the SIG node meeting where this was discussed as a result of PTO. I am wondering why the static pod author is not able to set priorityClassName on their manifest to system-cluster-critical ? we have been doing this, and i had not seen an issue with that approach.

@derekwaynecarr They can set priorityClassName in their podspec, but the integer value of the priority will not be resolved by time a static pod goes through admission logic of Kubelet. Please see #80489 for more info.

@k8s-ci-robot k8s-ci-robot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Aug 7, 2019
@dchen1107
Copy link
Member

@derekwaynecarr We discussed this at SIG Node, and I documented what we discussed at the original issue: #80203 (comment) I believe I also recorded the meeting. Please let me know if you had a second thought on it. Otherwise, I plan to approve the pr. Thanks!

@dchen1107
Copy link
Member

/lgtm

Will wait for @derekwaynecarr before giving the final approval.

@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, 2019
@derekwaynecarr
Copy link
Member

@bsalamat thanks for the missing detail.

/approve

@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 8, 2019
@draveness
Copy link
Contributor

/retest

@dchen1107
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, derekwaynecarr, hpandeycodeit

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

The pull request process is described here

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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
Development

Successfully merging this pull request may close these issues.

Treat all static pods as critical regardless of their priority
7 participants