-
Notifications
You must be signed in to change notification settings - Fork 970
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
Proposal of dynamic GPU slice plugin #3820
base: master
Are you sure you want to change the base?
Conversation
Welcome @sailorvii! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi, please squash to one commit and sign off. |
Signed-off-by: sailorvii <[email protected]> Signed-off-by: chenw66 <[email protected]>
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 have reviewd it, please take a look~
- plugins: | ||
- name: cugpushare | ||
arguments: | ||
cugpushare.schedulePolicy: spread |
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.
What does cu
mean in cugpushare
?
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.
Maybe we can rename the plugin? There is already a plugin called deviceshare
, we need to highlight that the plugin is used to dynamically split 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.
Sure, let's rename it.
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.
Where is the logic of this AddPod
, in the mig-agent of nos? I'm wondering whether our dynamic GPU slice plugin is strongly dependent on the nos project. You can see that the annotation has the watermark of nos, and nos project is not updated frequently.
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.
- AddPod is in volcano/pkg/scheduler/api/node_info.go addResource.
- 3 functions can be reused from nos project: mig agent, mps agent and mps device plugin. They are not the most important part. If needed, we could rewrite them.
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.
And I feel that the design of nos is strange. All the MIG profiles whether they are free or used are annotated as annotation entries of the node. And the MIG profiles requested by Pod are also annotated as annotation entries. That's not how annotation is meant to be used this way. Can we define a CRD to manage these specs and status, and node can refer to this CRD? Or we can aggregate these specs and status into one JSON struct and annotated as only one annotation entry.
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.
Sure, it's good to bind those annotations.
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 is the capacity in binpack or spread calculated here? Because we are dynamically dividing the GPU as MIG profiles, we don't know how many MIG profiles the GPU can be divided into.
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.
Oh, here we only use the memory as the resource capacity calculation. Each profile has a dedicated memory size.
cugpushare.weight.cpu: 1 | ||
cugpushare.weight.memory: 2 | ||
cugpushare.weight.gpu: 5 | ||
cugpushare.DevicePluginCMName: mps-configmap |
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.
Why do we need to specify the name and ns of the configmap here, isn't this fixed?
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.
This is a shared config between the planner and the device plugin. The name could be configured. The planner(in the scheduler) and device plugin should define the same name configmap.
NVIDIA official GPU sharing includes time-slice, MPS and MIG. Currently the MPS and MIG dynamic is not supported, we want to add this into volcano scheduler plugin