-
Notifications
You must be signed in to change notification settings - Fork 81
Introduce device profiles #129
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh 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 |
|
I wanted to open this PR before I'm off on vacation next week, but I intend to take a look at #128 first and incorporate those changes here before merging this. FYI @guptaNswati /hold |
|
@sunya-ch If you could take a look to see if your consumable capacity changes are able to align with this that would be great! |
sunya-ch
left a comment
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.
@nojnhuh
Could we define a DeviceProfile interface/struct with abstract methods like GetDevices, ApplyConfig, and ValidateConfig, and then keep a mapping from profile name to its implementation?
If we take that approach, I could implement ConsumableNetDevProfile or ConsumableGPUProfile independently. What do you think?
| resources: {} | ||
|
|
||
| kubeletPlugin: | ||
| # numDevices describes how many GPUs to advertise on each node when the "gpu" |
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 think we may want to pass this numDevices also to other profiles. Is there any reason to limit to gpu profile?
| sb := gpu.ConfigSchemeBuilder | ||
| assert.NoError(t, sb.AddToScheme(configScheme)) | ||
|
|
||
| s := httptest.NewServer(newMux(newConfigDecoder(), gpu.ValidateConfig, driverName)) |
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.
Are we expecting each profile to have its own ValidateConfig function? If so, we could define it as an abstract method on the profile struct.
This PR lays the groundwork for the example driver to be able to demonstrate several different kinds of devices, each implemented as a separate "profile." The changes mostly involve extracting the GPU-specific details and exposing handles to plug in different logic.
To see how this works with multiple profiles, I have a POC based on these changes implementing a new
gpupartprofile which demonstrates partitionable devices: nojnhuh/dra-example-driver@profiles...gpupartFixes #92