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

Fix priorityClassName typo, add numeric priority to static pods #89970

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Fix priorityClassName typo, add numeric priority to static pods #89970

merged 1 commit into from
Apr 9, 2020

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Apr 8, 2020

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes typo in static pod manifests, adds numeric priority to ensure relative preemption works properly.

Which issue(s) this PR fixes:
xref #89966

Special notes for your reviewer:
Fixes regression introduced in 1.16 when all static pods were made critical

Does this PR introduce a user-facing change?:

Restores priority of static control plane pods in the cluster/gce/manifests control-plane manifests

/sig cluster-lifecycle
/sig node

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 8, 2020
@liggitt liggitt changed the title Fix priorityClass typo, add numeric priority to static pods Fix priorityClassName typo, add numeric priority to static pods Apr 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject labels Apr 8, 2020
liggitt
liggitt commented Apr 8, 2020
View reviewed changes
cluster/gce/manifests/konnectivity-server.yaml
@@ -7,7 +7,8 @@ metadata:
seccomp.security.alpha.kubernetes.io/pod: 'docker/default'
component: konnectivity-server
spec:
priorityClassName: system-cluster-critical
priorityClassName: system-node-critical
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @caesarxuchao, since this is using UDS, it needs to run on this node (can't be rescheduled to another node)

This was referenced Apr 8, 2020
Merged
Merged
Merged
@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2020

/assign @dchen1107
based on #80203 (comment)

@dchen1107
Copy link
Member

We discussed at SIG Node last year, and I documented the conclusion at: #80203 (comment)

Per our discussion, the system should treat all static pods as the critical pods blindly. If I remembered it correctly, the fix was cherrypicked to 1.15. I am wondering if some part of the system didn't agree with that decision?

@dchen1107
Copy link
Member

/lgtm

cc/ @dashpole for another look too.

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

liggitt commented Apr 8, 2020

Per our discussion, the system should treat all static pods as the critical pods blindly. If I remembered it correctly, the fix was cherrypicked to 1.15.

I see the change made in #80491. I don't see it picked to 1.16.

I am wondering if some part of the system didn't agree with that decision?

Critical pods can have different priorities, and the kubelet will preempt ones with lower priority values. Because the static control plane pods did not specify a numeric priority, they were subject to preemption by a cluster-critical pod.

@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2020

/hold for @dashpole to take a look

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2020
@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2020

/priority important-soon

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

dashpole commented Apr 8, 2020

/lgtm

@liggitt
Copy link
Member Author

liggitt commented Apr 9, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2020
@k8s-ci-robot k8s-ci-robot merged commit 08e1fd3 into kubernetes:master Apr 9, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 9, 2020
k8s-ci-robot added a commit that referenced this pull request Apr 9, 2020
@k8s-ci-robot
Merge pull request #89973 from liggitt/automated-cherry-pick-of-#8997…
0ff2c2a 
…0-upstream-release-1.16

Automated cherry pick of #89970: Fix priorityClass typo, add numeric priority to static pods
k8s-ci-robot added a commit that referenced this pull request Apr 9, 2020
@k8s-ci-robot
Merge pull request #89971 from liggitt/automated-cherry-pick-of-#8997…
0c7f1c5 
…0-upstream-release-1.18

Automated cherry pick of #89970: Fix priorityClass typo, add numeric priority to static pods
k8s-ci-robot added a commit that referenced this pull request Apr 9, 2020
@k8s-ci-robot
Merge pull request #89972 from liggitt/automated-cherry-pick-of-#8997…
d7eaf96 
…0-upstream-release-1.17

Automated cherry pick of #89970: Fix priorityClass typo, add numeric priority to static pods
@liggitt liggitt deleted the static-priority branch April 10, 2020 15:12
This was referenced Dec 7, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrHohn MrHohn Awaiting requested review from MrHohn

@tallclair tallclair Awaiting requested review from tallclair

Assignees

@dashpole dashpole

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject 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/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
4 participants
@liggitt @k8s-ci-robot @dchen1107 @dashpole