-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
feat: cleanup pod critical pod annotations feature #79554
feat: cleanup pod critical pod annotations feature #79554
Conversation
262419e
to
8e06039
Compare
/assign @derekwaynecarr @spiffxp |
/retest |
d70b7b9
to
202290d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two high level questions on the yaml from me - this code touches some parts I'm not super familiar with so going to leave the full review to someone else :)
cluster/addons/calico-policy-controller/calico-node-daemonset.yaml
Outdated
Show resolved
Hide resolved
cluster/addons/calico-policy-controller/calico-node-vertical-autoscaler-deployment.yaml
Show resolved
Hide resolved
/retest |
14d2e3c
to
196f8ab
Compare
196f8ab
to
8a5b10a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks, @draveness!
@@ -1820,7 +1820,6 @@ func TestInsufficientCapacityNodeDaemonLaunchesCriticalPod(t *testing.T) { | |||
ds := newDaemonSet("critical") | |||
ds.Spec.UpdateStrategy = *strategy | |||
ds.Spec.Template.Spec = podSpec | |||
setDaemonSetCritical(ds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought it was not necessary to check that non-critical daemonsets are not scheduled, but let's leave the check in.
Kindly ping @derekwaynecarr for approval. |
/milestone v1.16 |
/approve update the release note with the specific annotation that is no longer honored (so people can easily search their manifests) and the specific field ( |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, draveness, liggitt, spiffxp 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 |
1312db8
to
b6d41ee
Compare
/lgtm |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
Commit kubernetes/kubernetes#79554 in kubernetes/kubernetes removed the ExperimentalCriticalPodAnnotation feature in kubernetes 1.16. This PR aligns aks-engine with those changes, removing the feature-gate from kube-proxy daemonset and the pod-critical annotations from any manifest that uses it. Fixes: Azure#1617
We have to revert this PR, as it caused a regression for static pods: #80203. After the static pod issue is addressed, we can resubmit this PR. |
…Name this feature was deprecated in 1.13. Now it`s not work.[1] "scheduler.alpha.kubernetes.io/critical-pod annotation is removed. Pod priority (spec.priorityClassName) should be used instead to mark pods as critical. Action required!"[2] [1]kubernetes/kubernetes#79548 [2]kubernetes/kubernetes#79554 Change-Id: I5913030634fe3f53b11ddb9bbe40d665f45a1254
* Update openstack-helm-infra from branch 'master' to 0053fc5737304e598dc2372413544a739c65e8b3 - replace scheduler.alpha.kubernetes.io/critical-pod with priorityClassName this feature was deprecated in 1.13. Now it`s not work.[1] "scheduler.alpha.kubernetes.io/critical-pod annotation is removed. Pod priority (spec.priorityClassName) should be used instead to mark pods as critical. Action required!"[2] [1]kubernetes/kubernetes#79548 [2]kubernetes/kubernetes#79554 Change-Id: I5913030634fe3f53b11ddb9bbe40d665f45a1254
/kind cleanup
/priority important-soon
/assign @bsalamat
/sig scheduling
/sig node
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #79548
Does this PR introduce a user-facing change?: