[RuntimeClass scheduling] native scheduler support, ready to implement #909

tallclair · 2019-03-20T22:48:00Z

Updated the RuntimeClass scheduling KEP to use native scheduler support rather than a mixin approach.

This approach uses a cleaner API (doesn't need to mixin a bunch of rules into the pod's node affinity), it let's us use the more expressive NodeSelector API, and provides a nice user experience in the error case.

I am opening this PR to continue the discussion @bsalamat started here

/sig node scheduling
/assign @bsalamat @derekwaynecarr @dchen1107
/cc @yastij
/milestone v1.15
/priority important-longterm

yastij · 2019-03-20T23:02:27Z

keps/sig-node/runtime-class-scheduling.md

-scheduling is to merge the topology constraints from the RuntimeClass into the
-PodSpec. Eventually, the controller's responsibilities may grow, such as to
-merge in [pod overhead][] or validate feature compatibility.
+A new scheduler predicate will manage the RuntimeClass topology. It will lookup


Maybe we should discuss predicate composition vs adding support of runtimeClass into other existing predicate

If we add it to existing predicates, we don't get a useful error message when the pod can't be scheduled, right?

@tallclair - if we merge tolerations into podSpec how can we inform user correctly (UX) when not finding a node that fits ?

I don't think that changes anything, as the tolerations would be resolved by the same PodToleratesNodeTaints predicate? But I might be misunderstanding how the debug messages are created.

Do you mean that we’d still check the runtimeClass when computing podToleratesNodeTaints ?

I think we can move forward, and discuss this while implementing

yastij · 2019-03-20T23:07:00Z

keps/sig-node/runtime-class-scheduling.md

-#### Mix-in
-
-The RuntimeClass topology is merged with the pod's NodeSelector & Tolerations.
+The `PodToleratesNodeTaints` predicate will also be extended to be


Can't we just call from the RuntimeClassPredicate the PodToleratesNodeTaints with the right parameters ? composing seems easier than modifying each predicate to include runtimeClass awareness

I assume predicates are AND'd together, in which case the PodToleratesNodeTaints predicate would also be called without the runtimeclass tolerations, and in that instance fail? I'm not very familiar with the scheduler code though, so I could be totally wrong.

This raises a broader Issue at predicates level, when a predicate is basically a composition of other:

do we add domain awareness to the existing predicates ?

do we compose these predicates into domain predicate ?

This might over complicate our predicates definition, and create performance overhead in some cases.

tallclair · 2019-03-26T18:44:30Z

Summarizing feedback from 2019-03-26 sig-node:

Add a section about node label restrictions, and how those should play into the RuntimeClass topology (security considerations)
Additional tradeoff on native scheduling vs. mixin: with native scheduling, it's harder to apply scheduling policy. This seems like more of a concern with tolerations than with the node selector, so I think we should actually take a hybrid approach, where tolerations are mixed in but the node selector is applied at scheduling time.

/assign @egernst

yastij · 2019-03-26T20:29:39Z

@tallclair - I think this has 2 main benefits

This tradeoff aligns with the scheduling policy, especially that enforcement of affinities will be done at scheduling time.
This will avoid the issue of predicate composition (If we still want to go down this path, I think we need a KEP for predicate composition)

my only concern is on the UX side as @bsalamat stated

bsalamat · 2019-03-27T00:04:23Z

keps/sig-node/runtime-class-scheduling.md

    // +optional
-    NodeSelectorTerm []corev1.NodeSelectorRequirement
+    NodeSelector corev1.NodeSelector

    // tolerations adds tolerations to pods running with this RuntimeClass.


I am a bit unsure about having tolerations as a part of "topology" in RuntimeClass. This is useful if one doesn't want to provide RuntimeClass on all their pods. If they provide RuntimeClass for all their pods they can just use NodeSelector. If they insist on tainting some nodes, they can use regular Toleration (which is not a part of RuntimeClass). So, I guess my question is "what is the major benefit of having tolerations" here?

I think this is covered under the Tolerations discussion on line 170:

Tolerations

While NodeSelectors and labels are used for steering pods towards nodes,
[taints and tolerations][taint-and-toleration] are used for steering pods away
from nodes. If every pod had a RuntimeClass and every RuntimeClass had a strict
NodeSelector, then RuntimeClasses could use non-overlapping selectors in place
of taints & tolerations. However the same could be said of regular pod
selectors, yet taints & tolerations are still a useful addition. Examples of
use cases for including tolerations in RuntimeClass topology
inculde:

Tainting Windows nodes with windows.microsoft.com to keep default linux pods
away from the nodes. Windows RuntimeClasses would then need a corresponding
toleration.

Tainting "sandbox" nodes with sandboxed.kubernetes.io to keep services
providing privileged node features away from sandboxed workloads. Sandboxed
RuntimeClasses would need a toleration to enable them to be run on those
nodes.

I agree that it's not strictly necessary, but I think it will be useful in practice. I guess both of the use cases listed could be accomplished with a default RuntimeClass that looked like this:

kind: RuntimeClass metadata: name: default handler: runc nodeSelector: nodeSelectorTerms: matchExpressions: - key: "windows.microsoft.com" operator: DoesNotExist - key: "sandboxed.kubernetes.io" operator: DoesNotExist

Does this assume that the toleration key is also set as label ? I think that this isn't always the case.

This implies using labels rather than taints.

it is more staightforward for users to implement dedicated nodes using taints and tolerations rather than nodeSelectors.

@bsalamat - The benefit of having tolerations on the runtimeClass is freeing users that want to consume a specific runtimeClass available on a dedicated node to think about which toleration they should set or not.

Your points are valid. Thanks for clarifying.

tallclair · 2019-03-27T00:19:09Z

Integrated feedback from sig-node. In particular, the RuntimeController was added back, and tolerations are back to being a mixin.

bsalamat · 2019-03-27T17:39:01Z

keps/sig-node/runtime-class-scheduling.md

@@ -203,6 +202,36 @@ will be a clear explanation of why. For example:
 0/10 Nodes are available: 5 nodes do not have enough memory, 5 nodes don't match RuntimeClass
 ```

+### RuntimeController


nit: I would call this Runtime Admission Controller to clarify that this is not a controller with a control loop that runs all the time and monitors things in the cluster.

Looks like the convention of the admission plugins is to just omit the controller piece altogether - so this would just be "Runtime". I'll clarify the name in the KEP though.

or rename to RuntimeTopology ?

The plan is to reuse the controller for other RuntimeClass-related admission controls. In particular PodOverhead. So I don't think it makes sense to include topology in the name.

agree with a single admission plugin for a common feature.

@tallclair - have a suggested controller name in mind? ContainerRuntime? Runtime is pretty overloaded...

I went with RuntimeClass for the controller name. Pretty self-explanatory.

keps/sig-node/runtime-class-scheduling.md

yastij · 2019-04-01T15:48:45Z

/lgtm
/hold

just a rename or clarification on the name of the admission controller is missing.

@bsalamat - any thoughts on merging toleration into podSpec ? This would help enforcing schedulingPolicy

bsalamat · 2019-04-01T18:33:22Z

@bsalamat - any thoughts on merging toleration into podSpec ? This would help enforcing schedulingPolicy

Are you saying that instead of having "toleration" in the RuntimeClass, we use the same toleration of Podspec? If so, we discussed it in this KEP and decided that the existence of toleration in the runtime class is useful.

tallclair · 2019-04-01T18:35:22Z

@bsalamat I think @yastij was asking about your thoughts on merging the tolerations FROM the RuntimeClass into the PodSpec during admission (what this KEP currently proposes), as opposed to just merging the tolerations at scheduling time (what it originally proposed).

bsalamat · 2019-04-01T18:52:30Z

@tallclair I'd prefer it to be separate so that we can provide better "unschedulable" message to users.

yastij · 2019-04-01T19:31:53Z

@bsalamat - I agree

@tallclair - if we merge tolerations into podSpec how can we inform user correctly (UX) when not finding a node that fits ?

I think we can keep them separate for now. If we see that we need to add tolerations to the podSpec we can do it afterwards.

derekwaynecarr · 2019-04-01T21:06:47Z

@tallclair reached out and asked that i review. i am fine with what is presented.

tallclair · 2019-04-01T22:48:38Z

I spoke with @bsalamat offline, and we agreed that it makes sense to merge the tolerations into the pod spec, but the RuntimeClass.Topology.NodeSelector will still be handled in a new scheduler predicate. There isn't a UX benefit to handling the RuntimeClass tolerations in the scheduler, because they still need to be handled in the same PodHandlesNodeTaints predicate, meaning the error message is still the same.

If the user experience is the same, merging the tolerations is much simpler (for the reasons outlined about, around handling RC deletion, etc.), and also makes it easier to apply toleration restrictions through policies (e.g. SchedulingPolicy, gatekeeper).

tallclair · 2019-04-01T22:49:18Z

@yastij WDYT? Does that alleviate your concerns?

yastij · 2019-04-01T23:14:18Z

@tallclair - sgtm

yastij · 2019-04-01T23:14:33Z

/hold cancel

egernst

lgtm

bsalamat · 2019-04-02T00:20:33Z

@tallclair
@egernst has a good point. What will happen if a RuntimeClass is deleted after pod creation? I would like to clarify the behavior of the scheduler for pods that are created but not scheduled yet. I am not concerned about already running pods.
When the scheduler sees a pod with a non-existing RuntimeClass, what should it do? I think it is reasonable to assume that no RuntimeClass is specified and always pass the predicate. Another obvious option is to always fail the RuntimeClass predicate and never schedule the pod. Anyway, I think this should be specified in the KEP.

tallclair · 2019-04-02T00:28:43Z

Hmm, good point. I wasn't thinking about the case that the RuntimeClass was missing before the pod is scheduled...

Another obvious option is to always fail the RuntimeClass predicate and never schedule the pod.

I think this makes the most sense, otherwise we're just delaying the failure until it lands on a node. If the predicate fails, the scheduler will retry, right?

yastij · 2019-04-02T12:23:47Z

I think this makes the most sense, otherwise we're just delaying the failure until it lands on a node. If the predicate fails, the scheduler will retry, right?

I agree here we'll just delay the failure, the predicate should fail and provide meaningful logs/reason.

bsalamat · 2019-04-02T20:26:56Z

I think this makes the most sense, otherwise we're just delaying the failure until it lands on a node. If the predicate fails, the scheduler will retry, right?

That's right. The scheduler will keep retrying (with some backoff time).

bsalamat

/lgtm
/approve

Thanks, @tallclair!

tallclair · 2019-04-04T18:23:41Z

Added failure modes for the scheduler & admission controller
Renamed RuntimeController to RuntimeClass (admission controller)
Marked KEP as implementable - I am happy with it's current state, and think we should commence coding.

tallclair · 2019-04-04T18:26:39Z

Squashed commits.

keps/sig-node/runtime-class-scheduling.md

derekwaynecarr · 2019-04-04T18:38:24Z

/approve

we can clarify the admission requirement in a follow-on or during documentation.

yastij · 2019-04-04T18:44:32Z

/lgtm

k8s-ci-robot · 2019-04-04T18:44:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, derekwaynecarr, tallclair, yastij

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:

~~keps/sig-node/OWNERS~~ [derekwaynecarr]

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 assigned bsalamat, dchen1107 and derekwaynecarr Mar 20, 2019

k8s-ci-robot added this to the v1.15 milestone Mar 20, 2019

k8s-ci-robot requested a review from yastij March 20, 2019 22:48

tallclair mentioned this pull request Mar 20, 2019

RuntimeClass scheduling KEP #896

Merged

yastij reviewed Mar 20, 2019

View reviewed changes

k8s-ci-robot assigned egernst Mar 26, 2019

bsalamat reviewed Mar 27, 2019

View reviewed changes

yastij mentioned this pull request Mar 27, 2019

Introduce topology into the runtimeClass API kubernetes/kubernetes#75744

Merged

bsalamat reviewed Mar 27, 2019

View reviewed changes

Huang-Wei reviewed Mar 28, 2019

View reviewed changes

keps/sig-node/runtime-class-scheduling.md Show resolved Hide resolved

k8s-ci-robot assigned yastij Apr 1, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 1, 2019

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 1, 2019

egernst approved these changes Apr 1, 2019

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2019

bsalamat approved these changes Apr 4, 2019

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 4, 2019

tallclair changed the title ~~RFC: [RuntimeClass scheduling] native scheduler support~~ [RuntimeClass scheduling] native scheduler support, ready to implement Apr 4, 2019

egernst approved these changes Apr 4, 2019

View reviewed changes

egernst mentioned this pull request Apr 4, 2019

keps: sig-node: initial pod overhead proposal #887

Merged

tallclair force-pushed the runtimeclass-scheduling branch from 7d9e6d2 to e99bc99 Compare April 4, 2019 18:26

derekwaynecarr reviewed Apr 4, 2019

View reviewed changes

keps/sig-node/runtime-class-scheduling.md Outdated Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2019

tallclair added 2 commits April 4, 2019 11:39

RuntimeClass scheduling KEP: native scheduler support

94c25e9

Mark the RuntimeClass Scheduling KEP as implementable

152d8a8

tallclair force-pushed the runtimeclass-scheduling branch from e99bc99 to 152d8a8 Compare April 4, 2019 18:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2019

k8s-ci-robot merged commit 246c106 into kubernetes:master Apr 4, 2019

[RuntimeClass scheduling] native scheduler support, ready to implement #909

[RuntimeClass scheduling] native scheduler support, ready to implement #909

Conversation

tallclair commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Mar 26, 2019

yastij commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij commented Apr 1, 2019

bsalamat commented Apr 1, 2019

tallclair commented Apr 1, 2019

bsalamat commented Apr 1, 2019

yastij commented Apr 1, 2019

derekwaynecarr commented Apr 1, 2019

tallclair commented Apr 1, 2019

tallclair commented Apr 1, 2019

yastij commented Apr 1, 2019

yastij commented Apr 1, 2019

egernst left a comment

Choose a reason for hiding this comment

bsalamat commented Apr 2, 2019

tallclair commented Apr 2, 2019

yastij commented Apr 2, 2019

bsalamat commented Apr 2, 2019

bsalamat left a comment

Choose a reason for hiding this comment

tallclair commented Apr 4, 2019

tallclair commented Apr 4, 2019

derekwaynecarr commented Apr 4, 2019

yastij commented Apr 4, 2019

k8s-ci-robot commented Apr 4, 2019

yastij Mar 27, 2019 •

edited

Loading