Partitionable with common attributes #30
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnbelamaric 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 |
k8s-ci-robot
added
approved
| @@ -68,6 +68,11 @@ type ResourcePoolSpec struct { | |||
| // +optional | |||
| SharedCapacity []SharedCapacity `json:"sharedCapacity,omitempty"` | |||
|
|
|||
| // Attributes contains common device attributes that are the same | |||
| // for all devices in the pool, unless a device specifically over | |||
There was a problem hiding this comment.
"the pool" or "the slice"?
The prototype is not up-to-date here 😢 We should update it to match the current KEP before doing further prototyping.
There was a problem hiding this comment.
Yeah. I didn't want to do that yet, I figured it could be done and I could rebase...
That helps reduce the size on average, but for the worst-case analysis which determines the limits of the slices we have to assume that the new |
Yes. True. |
|
I am making several options, see comment in #20 |
| - bool: true | ||
| name: mig-capable | ||
| - bool: false | ||
| name: mig-enabled |
There was a problem hiding this comment.
NOTE: we should establish core guidelines - are attributes expected to be mutable or immutable? Do we encourage drivers to embed status here or are attributes only intrinsic things about the device? This one feels like status.
There was a problem hiding this comment.
This should not be an attribute. MIG capable yes, enabled no. This is leftover from the structure I used to represent all of this in classic DRA. It is very compact, but also not generalizable.
| string: 1g.5gb+me | ||
| name: gpu-0-mig-1g.5gb-me-1 | ||
| sharedCapacityConsumed: | ||
| - capacity: "14" |
There was a problem hiding this comment.
"capacity" is a producer term, not a consumer term. This should be "quantity" or "amount" or "request" or something.
There was a problem hiding this comment.
It reuses the same struct as that used to declare the capacity. We would need a new struct then (which seems fine).
| name: gpu-0-ofa-engines | ||
| - capacity: 4864Mi | ||
| name: gpu-0-memory | ||
| - capacity: "1" |
There was a problem hiding this comment.
MAYBE (really not sure) we should say that if capacity is omitted here, it means "all"? Then it is sort of a fancy mutex for "partitions" and an quantity for fungibles.
Maybe not worthwhile
There was a problem hiding this comment.
I think it is clear that shared attributes is an appropriate optimization to include.
I think this further emphasizes that some form of nesting would (IMO) make other optimizations possible. There are attributes that apply to all "cards" (e.g. vendor). There are some attributes that apply to all GPUs (MIG or not) on a single card (e.g. the card UUID, for matching). There are some attributes that apply to each leaf device. Why are we shoehorning that into 2 levels?
|
Option 4 has some nesting. That's #31. It is much more efficient than this one. |
|
We could do more levels. Not clear the payoff is there. |
|
Option 3 makes common attributes. Option 4 makes common attributes AND common partition "map". |
|
I 100% agree we should have shared attributes. My first prototype had it, but at the time you said „let’s not prematurely optimize for size“ so we dropped it. I’m still not sold on nesting though. I also had it originally (in my „recursive“ device model), but you all (rightly) convinced me to drop it. And now after working with flat devices and updating both the example driver and the NVIDIA GPU driver to adhere to them, I’m really happy with the flexibility that a flat model brings us. I really don’t think it buys us much to have nesting and I have a strong feeling will come back to bite us fairly quickly. |
|
I was continuing on the KEP PR, but detoured to these option, so I will copy a comment: I just find it super weird to have gpu1's shared items consumable from gpu0. This is the thing that is setting me on edge. That implies to me a level of fungibility which doesn't exist. There is a grouping that is smaller than a ResourceSlice but bigger than a Device, and we are not modelling it. Call it a "card" for the moment. A slice doesn't have shared resources, a "card" does. Now you'll probably tell me "actually, one card can borrow resources on another card". In fact, I can already see the (hypothetical) use-case for a channelized I really can see both sides, and I don't mean to be dogmatic. It just smells funny. Let's keep the conversation overall moving forward, and if this is all that's left, we can hash it out.
Yeah, we dropped a LOT to get the baseline, and bringing partitions back makes it clear to me that this is one piece that really does make sense. |
| - capacity: 40Gi | ||
| name: gpu-0-memory | ||
| - attributes: | ||
| - name: mig-profile |
There was a problem hiding this comment.
We should include in this YAML the requirement that all MIG partitions on a "card" have a "parent UUID" attribute with the same value (for matching). I think that would highlight the "hidden" nesting level, even if we ultimately choose not to model it.
johnbelamaric
force-pushed
the
partitionable-3
branch
from
8da0e74 to
0c1d5fa
Compare
Based on kubernetes-sigs/wg-device-management#30. Instead of a discriminator (kubernetes-sigs/wg-device-management#36 (comment)), a versioning scheme gets added.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This is an evolution of the partitionable model defined in #27, which moves common attributes up to the pool level to reduce the size of the objects.