Add support for assigning cpu group on creation #1006
Conversation
dcantah
force-pushed
the
cpugroup-onstart
branch
from
da54335 to
7637ba3
Compare
dcantah
force-pushed
the
cpugroup-onstart
branch
from
7637ba3 to
dc7e80c
Compare
internal/uvm/cpugroups.go
Outdated
| } | ||
|
|
||
| // SetCPUGroup setups up the cpugroup for the VM with the requested id | ||
| func (uvm *UtilityVM) SetCPUGroup(ctx context.Context, id string) error { |
There was a problem hiding this comment.
So we can't set it at runtime anymore?
There was a problem hiding this comment.
From what I can tell this was just a workaround as we didn't support assigning it at creation time. I'd asked @katiewasnothere if we even needed to support runtime add anymore and there was no concerns but I can certainly leave this in.
There was a problem hiding this comment.
Yeah this was just a workaround. The expectation is that the orchestrator or host owner will create and manage cpugroups, so if they want to move a VM to a different cpugroup during runtime, they would have to handle it themselves, we don't provide knobs for that.
I don't think we need to keep this workaround, but make sure to still have a build block for assigning during create.
There was a problem hiding this comment.
I think we can expect (at least for the time being) for cpugroup creation/config/deletion to be handled out-of-band, but it seems like assigning a VM to a cpugroup is something we should own. It seems reasonable to me that we keep this exposed.
There was a problem hiding this comment.
By assigning do you mean just the initial assignment or additional reassignments during runtime?
There was a problem hiding this comment.
The point of us supporting the initial assignment is just so we can ensure that the VM starts immediately in the target cpugroup and that the VM's memory and cpu resources come from the same numa node(s) on start.
I don't think that exposing reassignment through us adds much and since the orchestrator is already in charge of managing the rest of the aspects of a cpugroup, I think it might be weird to have them call into us specifically for reassignment (this would need to be done through an update pod request).
Another thing to note: CO+ builds [will] support directly moving VMs between cpugroups (iow no need to move to the null group and then to the target new build).
Thoughts?
There was a problem hiding this comment.
Thanks for the background Kathryn :). It sounds like we don't need these really.. and afaik no ones even using this at the moment either. The runtime add/removes aren't actually hooked up to anything right? The functionality is here but there's nothing that someone could call externally to end up invoking any of this besides the previous way the initial assignment worked (add to group in uvm.Start and teardown in uvm.Close). I'm not tied to leaving or removing them, I don't see any harm in leaving them but if the plan was for reassignments to happen outside of the shim then I don't see a reason to keep them.
There was a problem hiding this comment.
If we don't think there is any need to support changing what cpugroup a VM is assigned to, I'm fine with taking this out. But if that's a requirement, I think we will need this capability, as there is no other path for the orchestrator to take.
There was a problem hiding this comment.
I removed it last night as from what Kathryn was saying it sounds like reassignments should be handled by whoever created the thing also. It would be trivial to re-add this as it wasn't actually hooked up to anything in the first place (but it would also be trivial to just leave it here and fix the linter warnings :P).
There was a problem hiding this comment.
aaaaand added it back per discussion :)
dcantah
force-pushed
the
cpugroup-onstart
branch
2 times, most recently
from
d8f7d72 to
2487edf
Compare
dcantah
force-pushed
the
cpugroup-onstart
branch
2 times, most recently
from
9a94df0 to
d0f5644
Compare
dcantah
force-pushed
the
cpugroup-onstart
branch
from
d0f5644 to
7201853
Compare
| const cpuGroupCreateBuild = 20124 | ||
|
|
||
| var errCPUGroupCreateNotSupported = fmt.Errorf("cpu group assignment on create requires a build of %d or higher", cpuGroupCreateBuild) | ||
|
|
||
| // ReleaseCPUGroup unsets the cpugroup from the VM | ||
| func (uvm *UtilityVM) ReleaseCPUGroup(ctx context.Context) error { |
There was a problem hiding this comment.
Are we keeping this for the future modify path?
In recent builds of Windows there was support added to the HCS to allow assigning a cpugroup at creation time of the VM instead of afterwards. The current approach in this repo of adding a vm after start was only a workaround as this wasn't supported at the time. The current approach isn;t ideal due to some wonky behavior on machines with multiple NUMA nodes as we can suffer performance penalties because of remote memory access on machines with > 1 node when adding a VM after start. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
dcantah
force-pushed
the
cpugroup-onstart
branch
from
7201853 to
377e39a
Compare
Related work items: microsoft#388, microsoft#389, microsoft#393, microsoft#394, microsoft#395, microsoft#396, microsoft#397, microsoft#398, microsoft#399, microsoft#400, microsoft#401, microsoft#402, microsoft#403, microsoft#404, microsoft#405, microsoft#931, microsoft#973, microsoft#1001, microsoft#1003, microsoft#1004, microsoft#1005, microsoft#1006, microsoft#1007, microsoft#1009, microsoft#1010, microsoft#1012, microsoft#1013, microsoft#1014, microsoft#1015, microsoft#1016, microsoft#1017, microsoft#1019, microsoft#1021, microsoft#1022, microsoft#1024, microsoft#1025, microsoft#1027, microsoft#1028, microsoft#1029, microsoft#1030, microsoft#1031, microsoft#1033
Add support for assigning cpu group on creation
In recent builds of Windows there was support added to the HCS to allow assigning
a cpugroup at creation time of the VM instead of afterwards. The current approach in this
repo of adding a vm after start was only a workaround as this wasn't supported at the time.
The current approach isn;t ideal due to some wonky behavior on machines with
multiple NUMA nodes as we can suffer performance penalties because of remote memory access on
machines with > 1 node when adding a VM after start.
Signed-off-by: Daniel Canter dcanter@microsoft.com