KEP-4381: DRA update for 1.31

[pohly]

• One-line PR description: Update the DRA API and design for Kubernetes 1.31

• Issue links:

  • DRA: structured parameters #4381
  • DRA: control plane controller ("classic DRA") #3063

• Other comments:

This is based on #4667 which got already merged.

If you review this anew, start with 4381, then circle back to 3063.

This update is the result of the prototyping in
https://github.com/kubernetes-sigs/wg-device-management/tree/main/dra-evolution.

The key changes compared to 1.30 are:

• Removal of CRD support. Vendor configuration parameters
  are still supported as raw extensions of the in-tree types.
• Removal of immediate allocation.
• Removal of "claim.status.sharable": all claims can be shared when multiple pods reference them.
• Removal of the notion of a "device model" and a selection mechanism
  that only works for one such model. This simplifies the API.
• Instead of one claim referencing a ResourceClass, individual
  requests inside a claim now reference a DeviceClass.
• Support for "match attributes".
• Support for "admin access".
• Support for "partititional devices".
• Support for more than one device per request ("all" and fixed amount).
• Support for selecting devices from a request in a claim for a
  container instead of exposing all devices of a claim.
• Support for network-attached devices with structured parameters (not a priority, came out of generalizing the pool concept)

Editorial changes:

• Reduced usage of the word "resource" in favor of "devices" and
  "DRA drivers".
• Removal of some out-dated text.

This revision of the KEP roughly corresponds to
https://github.com/kubernetes-sigs/wg-device-management/tree/258d109f54d3baaa48719519dec37a450a1dd5a1/dra-evolution
(= kubernetes-sigs/wg-device-management#14 with some minor edits).

Differences:

• Renaming of ResourceSlice ->
  ResourcePool (https://github.com/kubernetes-sigs/wg-device-management/pull/28/files#r1630964004) not included. Instead, the concept of a pool is introduced with stronger semantics around consistency and completeness. This includes non-local devices because we should be certain that we can cover those using the same terminology.
• Added "Amount" of devices per request (dra-evolution: counter per request, select device per index for container kubernetes-sigs/wg-device-management#22).
• Per-request constraints and configs
• Updated "classic DRA" (dra-evolution: restore minimal support for "classic DRA" kubernetes-sigs/wg-device-management#26)
• Partitionable devices via DRA: add text for supporting partitionable device pohly/enhancements#18

What was added from kubernetes-sigs/wg-device-management#26:

• ResourceClaim.Status.Allocation.NodeName -> NodeSelector
• flattening of the allocation result

Undecided:

• dra-evolution: "constraints" vs. "requirements" kubernetes-sigs/wg-device-management#16: now using "constraints" and "selectors"
• dra-evolution: "vendor" vs. "driver developer" kubernetes-sigs/wg-device-management#18: not a blocker
• dra-evolution: simplify CEL syntax kubernetes-sigs/wg-device-management#23: not a blocker
• dra-evolution: quota mechanism kubernetes-sigs/wg-device-management#24 and whether DeviceClassName should be required: proposal and discussion in KEP-4381: DRA update for 1.31 #4709 (comment), not a blocker because both alternatives seem feasible as a future extension (?)

[klueska]

As mentioned elsewhere, does this then imply that we will never be able to store a status on these objects?

[pohly]

We can add a status. Once we do, we need to become more careful with deletion and add more rules around it.

[johnbelamaric]

So we are not doing status in 1.31? I thought we were? It's on the cusp, I guess. The more important status is the one in #4681 which lets users see why their pods are failing.

[pohly]

Right, it's on the cusp. Someone still needs to define what that status looks like and who populates it (kubernetes-sigs/wg-device-management#19).

I prefer to merge this without a status, just to be sure that we have something that is agreed upon for 1.31. Perhaps we can get an extension to add the status.

[pohly]

I looked into this today (see issue comments), but it's not really obvious what the exact use cases are and how to model them in the API. Let's not try for 1.31.

[klueska]

Is there just a single selector per request? At one point we had a design for impliciltly "anding" multiple deviceSelector strings. Is that now not part of the design?

[pohly]

A request has multiple requirements, and each requirement is a CEL selector. So you can AND them together.

I need to change "a device selector" into "one or more device selectors".

[klueska]

This is related to:
https://github.com/kubernetes/enhancements/pull/4709/files#r1633087176

[johnbelamaric]

Maybe selectors is a better name for requirements? So, we would have selectors and constraints.

[pohly]

I like that. Changed, with "expression" instead of "deviceSelector".

[pohly]

The text still talks about "device selector" because that is what that expression is.

[klueska]

What other fields to we expect to live along side expressionas a one-of in the future?

[pohly]

Variable definitions for the CEL expressions was mentioned in one thread somewhere - found it: https://docs.google.com/document/d/1i0xVx884vsWzVaVb72LtEqjsVILmNkeZimAyz4Bj-m0/edit?disco=AAABLY07Bjc

[klueska]

Do we really need two names? Can't PoolName just be the NodeName in cases where all devices are node local?

[klueska]

In general, it's really confusing to see things like:

• only field1 OR field2 must be set
• only if field2 is set, then you can set field3

[pohly]

We can group "PoolName" and "NodeSelector" under "Pool", a struct containing "Name" and "NodeSelector".

Then "NodeName" and "Pool" are properly mutually exclusive.

[klueska]

I guess the confusing thing to me is that whether we are node-local or not, we still talk about having a "pool" of devices. Which (putting on my hat as a new-comer to this) would imply to me that I should have a PoolName in both cases.

I don't have a suggestion off the top of my head to avoid this (or an alternate name for this), but it's worth noting how confusing this is.

[klueska]

Could we require both PoolName and NodeSelector and in the single node case, set NodeSelector to the one and only node where the devices are available (as well as recommend that PoolName be set to the node name to ensure its uniqueness)?

[pohly]

Does PoolName really have to be required? It might not be needed when device names are globally unique.

[pohly]

I'm fine with requiring it, driver authors can always provide "global" if they cannot leave it empty - I just want to be sure.

[klueska]

I would say PoolName == NodeName is a suggestion (but it doesn’t have to be so long as it is globally unique) and yes it should be required so we don’t have to over explain the canonical naming convention.

[johnbelamaric]

+1 to this plan and to requiring PoolName, I think this is a lot clearer

[pohly]

Okay, changed.

I kept the part which says that a pool name may contains slashes, so we could end up with `dra.example.com/foo/bar/gpu" if a driver wants to separate different parts of its pool name.

[klueska]

source of the piece?

[johnbelamaric]

If this means "came from the class" maybe we should just name it that way ClassConfig or something?

[pohly]

"configuration piece", or just "configuration".

[klueska]

I guess I don't know what a "configuration piece" is...

[pohly]

Let's drop the "piece".

[pohly]

I'm using "configuration parameters" when I need a plural form. "Configurations" looked odd.

[pohly]

I've not renamed it to ClassConfig because I prefer to describe the semantic instead of the mechanism.

[klueska]

Why the extra level of indirection?

[pohly]

We agreed that "configuration" is a one-of, with one option right now the opaque vendor configuration. Standardized alternatives could get added later.

[klueska]

I guess I'm still confused as to what was decided for when we embed a struct vs. when we just put the one-of parameters inline. My understanding was that we decided to always put the one-of parameters inline...

[pohly]

The ConfigurationParameters is a one-of struct because it gets embedded in a slice where we need a struct. I'm using DriverConfigurationParameters as a struct because it mirrors that other struct.

Inlining a single field also has another challenge. It has to be required, but at the same time has to be a pointer because it might become optional:

type DeviceConfiguration struct {
   Admin bool
   
    // Currently required, might become optional when alternatives are added. Check for ni!
    Opaque *runtime.RawExtension
}

I find the struct easier to understand.

[pohly]

Can metadata.generation be used instead of creating a new field?

I tried with the current ResourceSlice implementation in 1.30 and could set metadata.generation on create, but not bump it during patching (tried kubectl apply and kubectl apply --server-side). Perhaps it would work if the type had a different support for the field.

[pohly]

No, metadata.generation on updates is not possible:
https://github.com/kubernetes/kubernetes/blob/6ba9fa89fb5889550649bfde847c742a55d3d29c/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go#L122-L127

That seems mandatory for all resources, so we have to use a separate field for this cross-resources generation.

[thockin]

Sending comments after a full day of reading. I stopped at ClaimStatus L1632. This is what you missed by me being sick, but the bill always comes due :) Really, though, I am sorry to have so many thoughts and keep pulling threads you thought were resolved.

FTR: I am not treating this a s a FULL review of API. I'll save that for the PR, but I will flag anything that seems like it will have major impact. I encourage you to send an API review WELL BEFORE the full impl is done, in case we make any structural changes (I know how annoying that is).

[thockin]

Same trap as above - you can't have nothing mean something. If we ever wanted to add a range option, but the client was back-rev, it would see all and count as unset and assume 1, which is wrong.

You can't even get away with saying that the apiserver will default count to 1 if no other field is set. If you have a modern client and a back-rev apiserver, and the client passes range (which the apiserver doesn't know about), the default will kick in, corrupting the client's intention.

AFAICT your choices are:

1. never add to this one-of and rely on "nothing set" = default

2. require exactly 1 field, which means trivial use-cases have to say count: 1

3. add a discriminator, if the discriminator is not set it gets defaulted to "Exact". Any other mode REQUIRES the discriminator be set.

We don't often do the discriminator thing in k8s. We have made the mistake several times and found ourselves in case 1 (e.g. NetworkPolicy). We usually can make case 2 work. In this case I think you might want 3.

Simple case: say nothing, get default CountMode: Exact and Count: 1

If I want 2: set CountMode: Exact and Count: 2

If I want all: set CountMode: All

If I want a range: set CountMode: Range and Range: { min: 1, max: 4 }

[johnbelamaric]

Ahh. That makes sense. I think the discriminator is probably what we want here.

[pohly]

The Amount struct is referenced via a pointer in RequestDetail. Therefore if we add a range, the back-rev client will see a pointer to an empty struct, which is safe.

The "if all and count are unset" is how the users sees it and therefore how the documentation is written. What the apiserver and client see is a nil *Amount pointer.

[johnbelamaric]

So, you are saying, "nil" means 1, but "empty" means "there must be something I don't now"; that is, we can differentiate as it is written, no need to change?

[thockin]

I don't think so. It's too clever and depends on the content-type and language/library details.

E.g. we support JSON as a transport. A forward-rev sender who sends range will unmarshall as a nil Amount in the receiver: https://go.dev/play/p/eQGyV5_UsIV

Also what does it mean if I pass all: false and nothing else?

[pohly]

This count "type" would have to be stored as a string to avoid issues with round-tripping.

But as we probably want to allow count: 1, an IntOrString is a better choice.

[thockin]

IntOrString is generally seen as a bad idea. JSON supports it, but protobuf (and any other reasonable transport) does not. We should not repeat that, I think.

if we allow countMode: exact and countMode: all in 1.31 and validation rejects everything else, how do we add countMode: range later?

API changes require an alpha. Assuming that we are successful in getting DRA to beat in 31:

1.31: we have only the main DRA gate
DRA gate (alpha) off: NA, not usable <-- default
DRA gate (alpha) on: would see "countMode: range" and reject it

1.32: we add a new gate for DRA count ranges
DRA gate (beta) off: NA, not usable
DRA gate (beta) on + range gate (alpha) off: would see "countMode: range" and reject it <-- default
DRA gate (beta) on + range gate (alpha) on: would see "countMode: range" and allow it

1.33: we have 2 gates
DRA gate (beta) off: NA, not usable
DRA gate (beta) on + range gate (beta) off: would see "countMode: range" and reject it
DRA gate (beta) on + range gate (beta) on: would see "countMode: range" and allow it <-- default

Any back-rev scheduler or autoscaler would say "I don't know what to do with that" and reject it.

The important piece is that API gate-checks are always "gate_enabled || already_in_use". In 1.32 you would have to manually enable the range gate (alpha), which waives rollback guarantees. In 1.33 the range gate (beta) would be on by default. If you used a range in 1.33, then rolled back to 1.32, it meets the "already_in_use" criterion. and would not fail validation.

Embedding the data into a string could ALSO satisfy this pattern, but we rarely, if ever, do formatted strings this way. The few places we have done it tend to be awkward (e.g. container image tags forcing consumers to parse for :latest). This would either abuse IntOrString (bad) or carry numbers as strings (bad, and yes I hate Quantity for this) and require a grammar and parsing (bad).

Or you can just require that clients pass exactly one option, making count: 1 explicit (no default).

[pohly]

So the already_in_use part is what makes validation not break when downgrading or disabling feature gates. That applies to adding new enum values as well as strings which need be parsed. But amount: "1" (now) and amount: "1-2" (future extension) requires parsing, so that's out.

I think that leaves @thockin's original proposal in this thread as the most viable approach (discriminator with additional fields). I'll keep the current simplistic one-of struct in my implementation PR for now, but will switch to the discriminator approach soon.

[thockin]

Thanks for working thru it. I am trying to collect information to write a clearer API convention on this specifically.

[pohly]

Included in kubernetes/kubernetes#125488.

[thockin]

If this moved up to type ConfigurationParameters (and was optional) then I think it would be simpler:

type ConfigurationParameters struct {
    DriverName string // optional

    // one-of
    Opaque *runtime.RawExtension
}

[pohly]

The driver name for a runtime.RawExtension config cannot be optional, otherwise it is unclear without peeking into the content which driver they are for.

[thockin]

If you don't specify a drivername, pass it to all of them?

[pohly]

And then they have to decode the configuration of some other driver to determine whether it is "for them". That seems fragile and error prone: decoding errors would have to be treated as "must be for someone else" and ignored when it really was meant for the driver and just has a typo somewhere.

[thockin]

Or it causes a decode error which causes the pod to fail, and the user should fix the YAML? I guess that's pretty ugly, but needs to be possible anyway. OK.

[thockin]

Why is ths useful? Again, don't make nothing mean something. How would you add that hypothetical non-device resource if this field being empty means "null" ?

[pohly]

The intention behind allowing an empty claim.requests was that its akin to malloc(0): it's not really wrong and may avoid having to code special cases in code which generates ResourceClaims for a pod.

Not particularly important, so if we feel that it helps avoid user mistakes ("forgot to ask for something by mistake") we can also make empty request an error.

How would you add that hypothetical non-device resource if this field being empty means "null" ?

I wouldn't - those would still be in requests IMHO.

[thockin]

Now that we agree on requests[*].device, this COULD be empty, but I still don't see why we want to allow it.

[pohly]

Okay.

[thockin]

Sending replies to comments

[thockin]

In that proposal I actually suggested that we remove all Quantity fields from DeviceAttributes

If we move allocatable things out of attributes, then this seems reasonable to me, though we would need to bring back IntValue, probably. I doubt that there are non-capacity attributes that benefit from quantityt notation, but if that is wrong, we could bring it back.

Users could then request these capacities in a Claim

I think that is ultimately coming, without much doubt. I also thing it makes sense to drop it from this release. That doesn't mean that we shouldn't start segregating "attribs" from "capacity". In this release people can treat capacity[] as a 2nd set of attributes and constrain against it. Later they make requests against it.

I guess we could still create the API for partial allocation now, even if actual allocation is whole device. So if the user makes a claim with requests for "10Gi", we will still grant them the whole GPU.

We don't need to put the requests API in yet - just let people reference dev.capacity["memory"] in CEL. "I want a GPU with at least 20Gi of memory, but I get the whole device". When we add requests the "at least" will be implied, but you will get only 20Gi.

[thockin]

Mostly resolved comments, just a few left

[thockin]

I thought we had landed on NOT qualifying them, which implied the drivername prefix?

[thockin]

If you don't specify a drivername, pass it to all of them?

[thockin]

Now that we agree on requests[*].device, this COULD be empty, but I still don't see why we want to allow it.