support pod and container updates #931
Conversation
|
@katiewasnothere #903 is in |
katiewasnothere
force-pushed
the
task_update_implementation
branch
5 times, most recently
from
badbac7 to
0331c47
Compare
| func normalizeContainerMemorySize(ctx context.Context, cid string, requestedSize uint64) uint64 { | ||
| actual := (requestedSize + 1) &^ 1 // align up to an even number | ||
| if requestedSize != actual { | ||
| log.G(ctx).WithFields(logrus.Fields{ | ||
| "containerID": cid, | ||
| "requested": requestedSize, | ||
| "assigned": actual, | ||
| }).Warn("Changing user requested MemorySizeInMB to align to 2MB") | ||
| } | ||
| return actual |
There was a problem hiding this comment.
We have a function that does the exact same thing in uvm/create.go can we just move that to a common file somewhere and use that at both locations?
There was a problem hiding this comment.
yeah I saw that. I'm not sure what a good common place would be though. Any suggestions?
There was a problem hiding this comment.
I think we can create a new file at internal/helpers.go and put this in there? Maybe a few other commons functions can go in that file too?
There was a problem hiding this comment.
Did we decide something about this?
internal/jobobject/utilities.go
Outdated
| @@ -0,0 +1,19 @@ | |||
| package jobobject | |||
There was a problem hiding this comment.
I personally think it's fine to leave them in this package if the methods are needed in other places (it used to be the case where only host process containers cared about this), but can we just call this file conversions.go maybe?
There was a problem hiding this comment.
Can't we just put these in hcsoci or something? It seems like they are just about converting from OCI concepts into job object concepts.
There was a problem hiding this comment.
I was looking at it as the notion of converting to a "processor count" isn't really tied to OCI, but it is what we were using the conversion for. I don't mind if it's in hcsoci
There was a problem hiding this comment.
I'm trying to push to keep jobobject completely unaware of what a container is. I would like it to be something we can put into go-winio in the future, so nothing container related should leak into it.
There was a problem hiding this comment.
Makes sense, I just didn't think a processor count was really container specific and might have other use cases
There was a problem hiding this comment.
moving these into hcsoci is a actually problematic since it creates an import cycle :/
There was a problem hiding this comment.
Looking at the code, it actually looks like nothing in the shim uses them aside from jobcontainers. If we're moving them just to use in test code, maybe it would be better to just duplicate the conversion instead?
There was a problem hiding this comment.
Good point. I think previously it was also being used for working with my test tool to set cpu limits for pre Vb builds, but since that's gone I can take it out. We'll move it if there's a reason to later.
|
|
||
| // share the test utility in so we can query the job object | ||
| guestPath := podJobObjectUtilPath | ||
| if err := shareInUVM(ctx, shimClient, testJobObjectUtilFilePath, guestPath, false); err != nil { |
There was a problem hiding this comment.
I'm confused a bit. If we've got a process isolated pod won't this fail? Shouldn't we be using the check below to skip this (in addition to setting where we expect the binary to be like you're doing)?
There was a problem hiding this comment.
Good catch, fixing
| return err | ||
| } | ||
|
|
||
| if ht.ownsHost && ht.host != nil { |
There was a problem hiding this comment.
Assuming ownsHost will only be true for LCOW if this is the pause container task, do we not support updating the pause container resources? It seems we won't ever call into updateTaskContainerResources in that case.
There was a problem hiding this comment.
Should we support updating the pause container resources? What would the scenario be?
| } | ||
| if resources.Memory != nil && resources.Memory.Limit != nil { | ||
| newMemorySizeInMB := *resources.Memory.Limit / bytesPerMB | ||
| memoryLimit := normalizeContainerMemorySize(ctx, ht.id, newMemorySizeInMB) |
There was a problem hiding this comment.
Is the 2MB alignment required for process-isolated containers as well? I thought it was only a requirement for UVMs.
There was a problem hiding this comment.
Talking to Duen-wen he said that it applies to the UVM and the container
| func (ht *hcsTask) requestUpdateContainer(ctx context.Context, resourcePath string, settings interface{}) error { | ||
| var modification interface{} | ||
| if ht.isWCOW { | ||
| modification = &hcsschema.ModifySettingRequest{ |
There was a problem hiding this comment.
Should we be using a guestrequest.GuestRequest for a hypervisor-isolated Windows container? Or does this request work for all Windows containers (process and hypervisor isolated)?
There was a problem hiding this comment.
hcsschema.ModifySettingRequest can be used to issue a modify request to a hypervisor isolated container. In particular, we need to provide the container resource path ResourcePath.
There was a problem hiding this comment.
Okay, so this works for both hypervisor and process isolated Windows container?
| var processorLimits *hcsschema.ProcessorLimits | ||
|
|
||
| switch resources := data.(type) { | ||
| case *specs.WindowsResources: |
There was a problem hiding this comment.
I don't like overloading the resource values on the OCI spec to also be used for UVM resources. We don't do this at UVM creation time, so I don't think we should do so here either. I think we should just look up the CPU/memory annotations that we use at UVM creation time, and use them here as well.
There was a problem hiding this comment.
That's a fair point. That'll be a pretty big change here though. Would you be okay with me removing the support for cpu update for a pod in this PR and adding it back using annotations in a later PR?
|
It would be great if we can tackle containerd/containerd#5187 once this is in. |
That's the plan :) |
katiewasnothere
force-pushed
the
task_update_implementation
branch
from
ff50f43 to
d98ba65
Compare
katiewasnothere
force-pushed
the
task_update_implementation
branch
2 times, most recently
from
93561ea to
8872793
Compare
Signed-off-by: Kathryn Baldauf <[email protected]>
katiewasnothere
force-pushed
the
task_update_implementation
branch
from
8872793 to
dffc7ef
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
…mment This comment was added in 09a0c94 when the Windows integration tests were enabled. The mentioned PR (microsoft/hcsshim#931) which was in hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8, so it looks like this comment is no longer current. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…mment This comment was added in 09a0c94 when the Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was merged, and part of hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8, so it looks like this comment is no longer current. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…mment This comment was added in 09a0c94 when the Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was merged, and part of hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8, so it looks like this comment is no longer current. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…mment This comment was added in 09a0c94 when the Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was merged, and part of hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8, so it looks like this comment is no longer current. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…mment This comment was added in 09a0c94 when the Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was merged, and part of hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8, so it looks like this comment is no longer current. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This PR adds support in hcsshim for container and pod task updating for cpu and memory. Additionally add until tests for the various shimTask interface implementations and e2e cri-containerd tests.
This PR depends on Change the update container bridge call to use the modify call instead
Signed-off-by: Kathryn Baldauf [email protected]