node label for policy and condition for CPUPressure #970
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
| This policy will be reported in the Node resource as a label with key | ||
| `alpha.kubernetes.io/cpu-policy`. This allows users with with a policy | ||
| preference to use a node selector to ensure the pod lands on a node with a | ||
| particular cpu manager policy enabled. |
There was a problem hiding this comment.
there are efforts in progress (#911) to remove or restrict the ability of nodes to label themselves, since that allows them to steer workloads to themselves.
There was a problem hiding this comment.
Wow ok. Didn't know about that.
I can either 1) remove this section completely or 2) s/will/can/ if trusted node label mechanism configures it independently rather then the kubelet knowing its own config and reporting it through a label.
What will happen to all the node labels in kubernetes.io that the node self-labels now?
There was a problem hiding this comment.
we'll have to whitelist them (which we may do for things like os/arch/instance-type) or silently drop them until we can remove them from the kubelet and get past the version skew window for kubelets that try to set them
There was a problem hiding this comment.
using labels for this could still work, I would just expect them to be applied to nodes, rather than having nodes self-report them
There was a problem hiding this comment.
I understand the spirit of the design, but it does mean that some fields the kubelet sets today based on its own introspection of the machine need to move to fields. Are we ok with the node controller then converting those fields to labels? Can cpu manager policy be included in that initial whitelist?
There was a problem hiding this comment.
@vishh - have you been tracking removing the ability for a node to self label? did you have other ideas for converting them to fields and then node controller assigning them as labels/taints?
There was a problem hiding this comment.
We rely on Node labels heavily to address scheduling constraints. Fields in node object cannot be used for scheduling purposes as of now.
Having users create labels that map kubelet configuration is not a great user experience. I can see the security aspect of having nodes introduce arbitrary labels.
Perhaps a whitelist of label keys is a way to get around the security issue? Ideally such a whitelist would be configurable (Configmaps maybe?) which would give cluster admins the ability to choose what labels they'd like to expose and also extend kubernetes easily.
There was a problem hiding this comment.
@derekwaynecarr #911 is news to me. I wonder why that proposal wasn't discussed in SIG-node yet.
k8s-github-robot
added
the
size/S
|
|
||
| NOTE: The decision to allow the shared pool to completely empty is | ||
| driven by a desire to keep the scheduler accounting simple. If a | ||
| --cpu-policy-static-min-shared-cpus flag were to exist, there is no simple way |
There was a problem hiding this comment.
nit: add backticks to flag name to avoid soft-wrapping.
| --cpu-policy-static-min-shared-cpus flag were to exist, there is no simple way | ||
| to convey that information to the scheduler. The scheduler would then need to | ||
| know that some portion of the CPU Allocatable is "special", for use only by | ||
| non-exlusive containers. |
There was a problem hiding this comment.
Possibly obvious, but consider also mentioning what would happen with a shared pool low-water-mark but without scheduler enhancements -- Kubelet would need to reject some G pods after scheduling, with high potential to confuse users.
|
|
||
| 1. _The shared pool becomes nonempty._ | ||
| 1. The CPU manager removes the node condition with effect NoSchedule, | ||
| NoExecute for BestEffort and Burstable QoS class pods. | ||
| 1. The CPU manager sets a CPUPressure node condition to false that allows |
There was a problem hiding this comment.
As I mentioned in the PR, this node condition isn't necessary if we reserve a core by default which I feel will be user's expectation.
|
I spoke with @ConnorDoyle and agree we can drop the CPU pressure condition. @sjenning -- can you remove the CPU pressure condition parts of this PR. |
|
@derekwaynecarr with the "no node self labeling" and dropping of the CPU Pressure condition, I'm not sure if any parts of this PR are needed anymore. Should I just close? |
|
This PR can probably be closed, but we need to open a separate issue for #970 (comment) |
* Update steering members Signed-off-by: Faseela K <faseela.k@est.tech> * add ctrath as member Signed-off-by: Faseela K <faseela.k@est.tech> --------- Signed-off-by: Faseela K <faseela.k@est.tech>
converted from ConnorDoyle#2
This is to address feedback from the RWMG discussion about a month ago.
Adds information on node label advertising cpu-policy and CPUPressure node condition.
@derekwaynecarr @ConnorDoyle