-
Notifications
You must be signed in to change notification settings - Fork 14k
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
add more clarification regarding priorityClassName use with DaemonSet #40851
add more clarification regarding priorityClassName use with DaemonSet #40851
Conversation
66a0070
to
00ed931
Compare
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Pull request preview available for checking
To edit notification comments on pull requests, go to your Netlify site configuration. |
00ed931
to
d41a3a0
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.
I wouldn't use system-node-critical
as the example priority class. Either make one up (priorityClassName: example
), or use system-cluster-critical
.
Why? Because it means that a reader who isn't paying attention is less likely to nobble their cluster.
@@ -35,6 +35,10 @@ spec: | |||
volumeMounts: | |||
- name: varlog | |||
mountPath: /var/log | |||
# system-node-critical is the highest priority class and can be used to ensure that | |||
# a critical daemonset's pod will evict an existing pod if that is required to allow |
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.
# a critical daemonset's pod will evict an existing pod if that is required to allow | |
# a node-critical pod will preempt an existing Pod if that is required to allow |
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.
I reworked the language a bit, I didn't like the comment being about system-node-critical
, with the commented out example being system-cluster-critical
. I made it a bit more generic and just used system-cluster-critical
in the example. WDYT?
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.
On reflection, even that's higher that I'm really happy with.
d41a3a0
to
ba424af
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 for docs
I don't think this needs a tech review.
/lgtm
LGTM label has been added. Git tree hash: c1227b2aa4899271ee21a030c81fd42d3ad5c77d
|
ba424af
to
0ee42c9
Compare
LGTM, despite #40851 (comment) |
0ee42c9
to
b900d2c
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.
Thanks
/lgtm
LGTM label has been added. Git tree hash: 7de311a4032482764f2105a62e416cd958b675e9
|
/lgtm |
LGTM label has been added. Git tree hash: eea3a0c67b335f57e432724688b6588059949289
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tengqm 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 |
Not using a priority class name can result in DaemonsetPods remaining pending. This can occur with cluster-autoscaler where the new DS pod is created, but can't evict existing
scheduled pods due to not having a high priority class. The DS pod then remains pending
indefinitely unless existing pods on the node happen to terminate to make room.