Set default scratch size for containers/UVMs and reuse LCOW scratch #886
Conversation
|
Can you rebase this PR? It shows Kevin's recent changes for scaling CPU as part of this PR. |
| int32 default_container_scratch_size_in_gb = 12; | ||
|
|
||
| // default_vm_scratch_size_in_gb is the default scratch size (sandbox.vhd) | ||
| // to be used for the UVM. This only applies to WCOW as LCOW doesn't mount a scratch | ||
| // specifically for the UVM. | ||
| int32 default_vm_scratch_size_in_gb = 13; | ||
|
|
||
| // share_scratch specifies if we'd like to reuse scratch space between multiple containers. | ||
| // This currently only affects LCOW. The sandbox containers scratch space is re-used for all | ||
| // subsequent containers launched in the pod. | ||
| bool share_scratch = 14; |
There was a problem hiding this comment.
Why are we adding in these options here but then never using them in this PR? I'd like to see this moved to another PR if possible.
There was a problem hiding this comment.
They're config options for containerd that will be used for the snapshotter. Theres nothing we can do with the values once we receive them in the shim, they need to be applied/used at the cri layer, they're useless here. Theres no other PR these could go in really
I don't see this? |
| // scale_cpu_limits_to_sandbox indicates that container CPU limits should | ||
| // be adjusted to account for the difference in number of cores between the | ||
| // host and UVM. | ||
| bool scale_cpu_limits_to_sandbox = 11; |
There was a problem hiding this comment.
@dcantah here is the dup code
There was a problem hiding this comment.
It's right above in the diff
There was a problem hiding this comment.
Ah looks like it was just reformatted.
|
Have you already created a PR for corresponding changes in containerd? Can you include a link for that PR in here? |
jterry75/cri#79, This is the LCOW work but you can see that they're utilized here. I'm just directly editing containerd in the vendor folder for ease of review so you'll have to click on Might as well lump this in here as well: microsoft/opengcs#383 |
dcantah
force-pushed
the
lcow-scratch-work
branch
from
7fe214b to
4f44c9f
Compare
dcantah
force-pushed
the
lcow-scratch-work
branch
from
4f44c9f to
99786b5
Compare
internal/layers/layers.go
Outdated
| @@ -285,6 +297,12 @@ func UnmountContainerLayers(ctx context.Context, layerFolders []string, containe | |||
| // Unload the SCSI scratch path | |||
| if (op & UnmountOperationSCSI) == UnmountOperationSCSI { | |||
| hostScratchFile := filepath.Join(layerFolders[len(layerFolders)-1], "sandbox.vhdx") | |||
| // Need to check if the sandbox is a symlink to another sandbox to handle the case | |||
There was a problem hiding this comment.
Do we also need to update the DestroyLayer function call to not delete the VHD if it is a symlink? I am not sure what the vmcompute.DestroyLayer API will do if the vhd is a symlink.
There was a problem hiding this comment.
DestroyLayer won't be called for LCOW layers which is the only time this would be a symlink so we're good here. We don't have any ideas for how we'd go about sharing scratch for WCOW yet.
There was a problem hiding this comment.
If we somehow arrived at a similar solution for WCOW, I don't believe we have to do anything special. DestroyLayer just calls DeleteFile on the vhd which seems like it handles symlinks just fine. https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-deletefile
Symbolic link behavior—
If the path points to a symbolic link, the symbolic link is deleted, not the target. To delete a target, you must call CreateFile and specify FILE_FLAG_DELETE_ON_CLOSE.
There was a problem hiding this comment.
Ah I am an idiot. I didn't realize that this was only for LCOW. Anyway we now also know that DestroyLayer will still work if we do this for WCOW.
dcantah
force-pushed
the
lcow-scratch-work
branch
from
99786b5 to
a08d4c8
Compare
… LCOW container scratch * Add containerd.toml options to be able to set the default scratch space size for containers and the UVMs scratch size for WCOW. * Add containerd.toml option to be able to specify that we'd like to share the sandbox containers scratch space for workload containers for LCOW. * Evaluate symlinks for the sandbox.vhd in the scratch layer as that's what is expected to be set up by the LCOW snapshotter instead of an absolute path to a previous containers sandbox.vhd. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
lcow-scratch-work
branch
from
a08d4c8 to
aff39ed
Compare
|
LGTM! |
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
Set default scratch size for containers/UVMs and reuse LCOW scratch
Add containerd.toml options to be able to set the default scratch space size for containers and the UVMs scratch size for WCOW.
Add containerd.toml option to be able to specify that we'd like to share the sandbox containers scratch space for workload containers for LCOW.
Evaluate symlinks for the sandbox.vhd in the scratch layer as that's what is expected to be set up by the LCOW snapshotter instead of an absolute path to a previous containers sandbox.vhd.
Signed-off-by: Daniel Canter [email protected]