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

feat: cleanup pod critical pod annotations feature #80342

Conversation

@draveness
Copy link
Member

commented Jul 19, 2019

/kind cleanup
/priority important-soon
/sig scheduling
/sig node

What this PR does / why we need it:

Reapply #79554

Critical Pod Annotation is an experimental feature that lets Kubernetes control plane know about a pod being a critical pod by adding a specific annotation to the pod. This was added in 1.5 when K8s didn't have pod priority. Pod priority allows us to mark pods as critical with our first-class API. As a result, this feature was deprecated in 1.13. We can now remove the feature from our code base.

Which issue(s) this PR fixes:

Fixes #79548

Does this PR introduce a user-facing change?:

scheduler.alpha.kubernetes.io/critical-pod annotation is removed. Pod priority (spec.priorityClassName) should be used instead to mark pods as critical. Action required!
@draveness

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

/hold

blocked by regression #80203

@draveness

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

/retest

2 similar comments
@draveness

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

/retest

@draveness

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

/retest

Copy link
Contributor

left a comment

Just to confirm... this is the same diff as before, but we are holding it on fixing the issue you linked above (which was the cause of the regression)?

@draveness

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Just to confirm... this is the same diff as before, but we are holding it on fixing the issue you linked above (which was the cause of the regression)?

Yes, this would be merged after we solve #80203

@draveness draveness force-pushed the draveness:feature/remove-critical-pod-annotation branch from 204f833 to f121ffa Aug 9, 2019
@draveness draveness force-pushed the draveness:feature/remove-critical-pod-annotation branch from f121ffa to 7c5aedb Aug 9, 2019
@draveness draveness force-pushed the draveness:feature/remove-critical-pod-annotation branch from 7c5aedb to 495faa2 Aug 9, 2019
@dixudx

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I think it is safe to merge this now, since statis pod issue has been fixed in #80491.

/lgtm
/approve

@dixudx
dixudx approved these changes Aug 9, 2019
@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Kindly ping @deads2k @mikedanese @vishh @liggitt for approval since the previous PR #80203 has already been merged.

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, draveness, 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

@draveness

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 273e926 into kubernetes:master Aug 15, 2019
23 checks passed
23 checks passed
cla/linuxfoundation draveness authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 15, 2019
@draveness draveness deleted the draveness:feature/remove-critical-pod-annotation branch Aug 15, 2019
rifelpet added a commit to rifelpet/kops that referenced this pull request Aug 18, 2019
The E2E tests are currently failing [0] due to this kubelet error [1]:

`F0818 22:43:57.642896    6424 server.go:179] unrecognized feature gate: ExperimentalCriticalPodAnnotation`

This feature gate was removed in Kubernetes 1.16 [2]

[0] https://testgrid.k8s.io/sig-cluster-lifecycle-kops#kops-aws-1.14
[1] https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-e2e-kops-aws-beta/1163216201782923264/artifacts/52.77.251.45/kubelet.log
[2] kubernetes/kubernetes#80342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.