generated from kubernetes/kubernetes-template-project
-
Notifications
You must be signed in to change notification settings - Fork 7
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
DRA API evolution #14
Merged
Merged
Changes from all commits
Commits
Show all changes
71 commits
Select commit
Hold shift + click to select a range
a67f632
add "dra-evolution" proposal
pohly 1504622
dra_evolution: add quota
pohly 7c2d569
dra-evolution: claim status notes
pohly e3b570c
dra-evolution: move pod to api
pohly 0a77223
dra-evolution: add ResourceClaimTemplate
pohly bec6652
dra-evolution: add automatic testing of YAML files
pohly ab87abd
dra-evolution/testdata: split up YAML
pohly c2a062c
Migrate dra-evolution/testdata/classes.yaml to relevant types
klueska 84cc5f1
dra-evolution api: fix container resource claim list
pohly 56c74fd
dra-evolution: validate CEL expressions
pohly dea8ded
dra-evolution: fix some CEL expressions
pohly 83b43b9
dra-evolution: also validate ResourceClaim[Template]
pohly 73af0cf
dra-prototype README.md: compare CEL syntax
pohly ee69061
dra-evolution: consolidate filter and request types
pohly 8024e0c
dra-evolution: add device.driverName
pohly 6f8518e
dra-evolution: update classes.yaml
pohly d435a4a
dra-evolution README.md: explain how to do a YAML diff
pohly 8cbe299
dra-evolution: update pod_types.go with proper json/protobuf tags
klueska c34e043
dra-evolution: migrate pod-one-container-one-gpu.yaml to relevant types
klueska 7a622ff
dra-evolution: device.attributes and device.<type>Attributes
pohly 59de778
dra-evolution: clarify expection for 'one device' class
pohly f4d03d5
dra-evolution: update claim_types.go with proper json tags
klueska 22c7ae6
dra-evolution: migrate pod-one-container-two-gpus-*.yaml to relevant …
klueska 65d5c49
dra-evolution: add MatchAttributes into ResourceRequestDetail
klueska 4e9e3f3
dra-evolution: update examples with new "localized" MatchAttributes
klueska f4b4052
dra-evolution: migrate two-pods-one-gpu-*.yaml to relevant types
klueska bb2a6d8
dra-evolution: harmonize fields
pohly 846e397
dra-evolution: remove ResourceClaimDevice and expand ResourceClaimEntry
klueska 7b3aef2
dra-evolution: migrate pod-two-containers-*.yaml to relevant types
klueska bfa9359
dra-evolution: migrate pod-one-container-one-gpu-one-vf.yaml to relev…
klueska 88f447b
dra-evolution: consistently use pcie-root.dra.k8s.io in the examples
klueska d3fb9c4
dra-evolution: skip YAML test cases that haven't been converted
pohly 4cb8709
dra-evolution: unknown keys are not runtime errors
pohly 45d14ba
dra-evolution: require fully-qualified attribute names
pohly ac9871d
dra-evolution: use fully-qualified attribute name
pohly aa0d952
dra-evolution: introduce more general request requirements
pohly cd5ba42
dra-evolution: add support for expressing shared resources between a …
klueska bd8117a
dra-evolution: migrate pools-two-nodes-one-dgxa100.yaml to relevant t…
klueska 3e5c69f
dra-evolution: refine the notion of a ResourceRequirement based on sh…
klueska 3b98607
dra-evolution: migrate pod-one-container-shared-split-allocation-gpus…
klueska 324e2e4
dra-evolution: add missing IntRange
pohly e71290b
dra-evolution: introduce "claim requirements"
pohly 3ec5c7e
dra-evolution: hide empty claim and request options
pohly 7703dfa
dra-evolution: revise class inheritance
pohly b4c48f6
dra-evolution: typo fix
pohly 74df07f
dra-evolution: harmonize list of vendor configs with other fields
pohly adb0b5d
dra-evolution: add proper type for intrange
klueska c920b2b
dra-evolution: revise naming of allocation result fields and structs
pohly 210b376
dra-evolution: remove "forClass" claim source
pohly 6c887ba
dra-evolution: rename class references
pohly aecf75c
fixup! dra-evolution: remove forClass claim source
pohly 819ac75
dra-evolution: use "constraints" and "requirements"
pohly 883167e
dra-evolution: update stale content in README.md
pohly ed651b1
dra-evolution: fix mock api server
pohly 1dbb6d2
dra-evolution: remove multi-inheritance of classes
pohly 6eab574
dra-evolution: remove "source" nesting via inlining
pohly 9120701
dra-evolution: add support for network-attached devices
pohly b18ad3b
Revert "dra-evolution: add proper type for intrange"
pohly 3b324ee
Revert "dra-evolution: migrate pod-one-container-shared-split-allocat…
pohly 75d1d2e
Revert "dra-evolution: refine the notion of a ResourceRequirement bas…
pohly ed56e13
Revert "dra-evolution: migrate pools-two-nodes-one-dgxa100.yaml to re…
pohly 9e74635
Revert "dra-evolution: add support for expressing shared resources be…
pohly ebfa95f
dra-evolution: ResourceClass -> DeviceClass
pohly 7aeb3c4
dra-evolution: revise ResourcePool
pohly 82be70e
DRA: simplified proposal
pohly 308b670
dra-prototype: review feedback
pohly 13eace9
dra-prototype: use driver names which follow the recommended naming p…
pohly 3ed68fa
dra-evolution: simplify referencing the node in allocation result
pohly dfd9ec8
dra-evolution: review feedback
pohly 49e124c
dra-evolution: update README.md
pohly 72c1f7c
dra-evolution: typo fix
pohly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,45 +1,64 @@ | ||
module github.com/kubernetes-sigs/wg-device-management/k8srm-prototype | ||
module github.com/kubernetes-sigs/wg-device-management/dra-evolution | ||
|
||
go 1.22.1 | ||
|
||
replace github.com/kubernetes-sigs/wg-device-management/nv-partitionable-resources => ../nv-partitionable-resources | ||
|
||
require ( | ||
github.com/NVIDIA/go-nvml v0.12.0-5 | ||
github.com/google/cel-go v0.20.1 | ||
github.com/blang/semver/v4 v4.0.0 | ||
github.com/google/cel-go v0.17.8 | ||
github.com/kubernetes-sigs/wg-device-management/nv-partitionable-resources v0.0.0-00010101000000-000000000000 | ||
github.com/stretchr/testify v1.9.0 | ||
k8s.io/api v0.30.0 | ||
k8s.io/apimachinery v0.30.0 | ||
k8s.io/apiserver v0.30.0 | ||
k8s.io/klog/v2 v2.120.1 | ||
k8s.io/utils v0.0.0-20240423183400-0849a56e8f22 | ||
sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver v0.0.0-20240404191132-83bd9c05741b | ||
sigs.k8s.io/yaml v1.4.0 | ||
) | ||
|
||
require ( | ||
github.com/Masterminds/semver v1.5.0 // indirect | ||
github.com/NVIDIA/go-nvlib v0.3.0 // indirect | ||
github.com/antlr4-go/antlr/v4 v4.13.0 // indirect | ||
github.com/antlr/antlr4/runtime/Go/antlr/v4 v4.0.0-20230305170008-8188dc5388df // indirect | ||
github.com/davecgh/go-spew v1.1.1 // indirect | ||
github.com/emicklei/go-restful/v3 v3.11.0 // indirect | ||
github.com/go-logr/logr v1.4.1 // indirect | ||
github.com/go-openapi/jsonpointer v0.19.6 // indirect | ||
github.com/go-openapi/jsonreference v0.20.2 // indirect | ||
github.com/go-openapi/swag v0.22.3 // indirect | ||
github.com/gogo/protobuf v1.3.2 // indirect | ||
github.com/golang/protobuf v1.5.4 // indirect | ||
github.com/google/gnostic-models v0.6.8 // indirect | ||
github.com/google/gofuzz v1.2.0 // indirect | ||
github.com/google/uuid v1.6.0 // indirect | ||
github.com/josharian/intern v1.0.0 // indirect | ||
github.com/json-iterator/go v1.1.12 // indirect | ||
github.com/mailru/easyjson v0.7.7 // indirect | ||
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect | ||
github.com/modern-go/reflect2 v1.0.2 // indirect | ||
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect | ||
github.com/pmezard/go-difflib v1.0.0 // indirect | ||
github.com/stoewer/go-strcase v1.2.0 // indirect | ||
golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa // indirect | ||
golang.org/x/net v0.23.0 // indirect | ||
golang.org/x/oauth2 v0.10.0 // indirect | ||
golang.org/x/sync v0.6.0 // indirect | ||
golang.org/x/sys v0.18.0 // indirect | ||
golang.org/x/term v0.18.0 // indirect | ||
golang.org/x/text v0.14.0 // indirect | ||
google.golang.org/genproto/googleapis/api v0.0.0-20230803162519-f966b187b2e5 // indirect | ||
google.golang.org/genproto/googleapis/rpc v0.0.0-20230803162519-f966b187b2e5 // indirect | ||
golang.org/x/time v0.3.0 // indirect | ||
google.golang.org/appengine v1.6.7 // indirect | ||
google.golang.org/genproto/googleapis/api v0.0.0-20230726155614-23370e0ffb3e // indirect | ||
google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d // indirect | ||
google.golang.org/protobuf v1.33.0 // indirect | ||
gopkg.in/inf.v0 v0.9.1 // indirect | ||
gopkg.in/yaml.v2 v2.4.0 // indirect | ||
gopkg.in/yaml.v3 v3.0.1 // indirect | ||
k8s.io/klog/v2 v2.120.1 // indirect | ||
k8s.io/utils v0.0.0-20240423183400-0849a56e8f22 // indirect | ||
k8s.io/client-go v0.30.0 // indirect | ||
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect | ||
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect | ||
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect | ||
) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Talking to people, I think we should pin down what a class is actually for.
Is it for vendors to expose all of their devices, e.g. class "nvidia-gpu" sets the driver "nvidia.com/gpu" and that's it? That seems pointless.
Is it for vendors to expose each individual device, e.g. classes "nvidia-a100", "nvidia-h100", etc each set a specific constraint? That seems heavy handed.
Is it for kubernetes to try to equivocate, e.g. class "k8s-gpu" allows NVIDIA, Intel, or AMD devices)? That seems unlikely to work.
Is it for cluster-admins to define a qualitative name and indirection (e.g. class "good-gpu" is a100 on one cluster and h100 on another)? Is that realistic?
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.
In "classic" DRA classes were alot more meaningful. Since every claim had to point to a class, and every class had to point to a driver, classes provided an admin with a way to restrict what resources a claim could grab hold of in a given environment. Vendors could design their class parameters to define what restrictions were possible (e.g. disallow sharing), and their vendor-specific controller could enforce these restrictions at allocation time.
In the new world of "semantic" models, the purpose of a class is a little less clear since the allocation policy is not vendor specific anymore. In the latest iteration its purpose has become even less given that claims no longer have to point to a class, and classes no longe have to point to a driver,
In the latest iteration, classes have basically become a way of creating a set of predefined constraints / requirements as a convenience so that users don't have to write it all out themselves when defining their claims.
In principle, I'm not opposed to providing such conveniences -- but the abstraction it provides no longer feels like a
ResourceClass
in the sense that it did back when DRA was first formulated. Where things get most weird to me though, is when we start talking about having a proliferation ofResourceClasses
defined this way -- each of which encapsulates one small constraint or a single resource requirement -- and then providing a means for a claim to "inherit" from multiple such classes to build out its full set of requirements.If this is the true purpose of the
ResourceClass
now, then it almost feels like it would be better to do away with the concept of "resource classes" altogether and instead define a different top-level object which simply contains a list of named CEL expressions. One could then reference this object and one of its named expressions from appropriate places inside the claim.There would be a lot less objects, the intent would be clearer, and the messy notion of "inheritance" would not be built into the model anywhere.
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'm not sure how I feel about this, still.
Yeah, this doesn't excite me.
I'm soundly of a "less is more" mindset right now. Classes originated with
StorageClass
, whose goal was expressly to give cluster admins a way to interpose between requests for storage and the implementations of the volumes, so that workloads could plausibly port between clusters. This predicated on the idea that the API (mounted filesystems) was roughly identical between providers (true for block and FS volumes, a little less true for NAS volumes). I don't see that same property for GPUs any time soon, though it may be true for other kinds of devices (of which we have a shortage of real examples).The only real properties of ResourceClass left are:
Are these valuable enough?
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.
There is a third reason for classes:
I don't know whether that is a strong enough reason to have them. But if we add a
resourceClassName
field later, it would be ignored by an older scheduler, so adding it later is going to be harder.I'm not sure I understand what is meant with this. Is this a weaker form of a vendor-independent "give me a GPU" class?
My thinking was that this could be enforced with mutating webhooks, i.e. no need to have it in core Kubernetes. I might be wrong and it really has to be a mandatory field because a webhook would have to interpret the request to determine which device is meant, which might not be possible.
I'm not a fan of making classes mandatory. If we did this and then had "restricted" classes and "less restricted classes" in the cluster, we'd need access control for the classes. IMHO a better solution is to implement a quota mechanism that can be used to prevent the usage of certain devices per namespace, instead of doing it indirectly through classes.
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.
How do we govern that? Is it a different CRD type for class config vs claim config? I buy the use case, but I wonder how important it is REALLY? I know you have thought more about non-GPU devices than the rest of us...
Exactly. I, the cluster/platform admin declare that NVIDIA and AMD are "equivalent enough". Or more realistically, that two network or storage or something else device providers are equivalent enough.
I don't think I see how that can work in a reasonable way, but I have not gotten to your quota parts yet.
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.
Here's another, simpler alternative that is more in line with John's proposal:
Much easier to understand and explain. It's also more limited. There's no way to pre-define constraints between multiple devices. Users have to put those into their claims. I'm fine with this.
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.
Because classes are optional, we need a different mechanism to limit which devices are accessible within which namespace. See https://github.com/kubernetes-sigs/wg-device-management/pull/14/files#r1613095446 for a proposal.
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.
Even if classes were mandatory, we then still would need an access control mechanism. I prefer to decouple "device access control" from "privileged device configuration access control" (aka access to classes). They seem orthogonal to me.
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.
Recapping from a doc-thread:
If classes are how we do quota, then they can't be optional.
If classes are not how we do quota, then a) we need a way to do quota; and b) I'm not sure classes are really valuable
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.
Agreed. We also need to do it at allocation time, to accurately reflect what the user actually is getting.
If we were to base quota on classes, the classes would have to be accurate: if class is "a 10GiB GPU or larger", then users can game the system by using that class and hoping that they get something bigger. Therefore I believe that quota should be based on device attributes, not classes.
The current proposal (DeviceClass) seems useful to me, but I am also okay with dropping them as long as we design the API so that we can add them back safely through a separate KEP.
We could "reserve" fields for future use:
Same for a request instead of defining a
deviceClassName
. Note that although the struct is now calledClaimClassRef
, the actual field name is only going to be defined in the future and even the struct can be renamed. Nothing of this will appear in the current user API, it's just Go code.We should only do this judiciously. It puts a burden on clients which might never pay off. But it is a way for us to punt on implementing something now that we want (or might want) to add in the future.
The error message in current clients needs to know what the field is for. We could even avoid that with:
But that would make the future API weird: even if we set the FieldName in the API server as a default, it would be user-visible.