Add support for creating and mounting merged CIMs. #2123
Conversation
ambarve
force-pushed
the
cims_merged
branch
2 times, most recently
from
8d74e32
to
73c01f0
Compare
pkg/cimfs/cim_writer_windows.go
Outdated
| defer winapi.CimCloseImage(cim.handle) | ||
|
|
||
| for _, lpath := range cimPaths { | ||
| if err := winapi.CimAddFsToMergedImage(cim.handle, lpath); err != nil { |
There was a problem hiding this comment.
Basically I don't undestand what this step of adding merges to a cim file is for. I thought merging was purely a mount-time operation.
There was a problem hiding this comment.
There are two types of merged CIMs. On disk merge and in memory merge. What you are referring to is an in memory merge. However, with that we can't reuse an already merged CIM with multiple containers (every mount operation will create the merge). With an on disk merge, we create a merged CIM once (which basically just a CIM that contains metadata about underlying layer CIMs) and mount that merged CIM every time we start a container. CimAddFsToMergedImage call adds a CIM into such a merged CIM. Does that help?
pkg/cimfs/cim_writer_windows.go
Outdated
| @@ -62,6 +62,33 @@ func Create(imagePath string, oldFSName string, newFSName string) (_ *CimFsWrite | |||
| return &CimFsWriter{handle: handle, name: filepath.Join(imagePath, fsName)}, nil | |||
| } | |||
|
|
|||
| // CreateMergedCim creates a new merged CIM from the CIMs provided in the `cimPaths` array. CIM at index 0 is | |||
| // considered to be the topmost CIM and the CIM at index `cimPath.length-1` is considered the base CIM | |||
There was a problem hiding this comment.
I think it would be more untuitive to make cimPaths[0] the base CIM. Is there a reason we don't?
There was a problem hiding this comment.
It's just how CimFS expects the layers to be added. The CimAddFstoMergedImage call expects that the topmost layer is added first and bottom most layer is added last. But I have updated the function to take cim paths in base to topmost order.
| if err := winapi.CimMergeMountImage(uint32(len(cimsToMerge)), &cimsToMerge[0], mountFlags, &volumeGUID); err != nil { | ||
| return "", &MountError{Cim: mergedCimPath, Op: "MountMerged", Err: err} | ||
| } | ||
| return fmt.Sprintf("\\\\?\\Volume{%s}\\", volumeGUID.String()), nil |
There was a problem hiding this comment.
If you use backticks for your string literal, you don't need to escape backslashes.
There was a problem hiding this comment.
I personally just prefer double quotes since backticks are easy to confuse with single quotes. Since rest of the file is already using double quotes, let us keep them here too?
| // MountMergedCims mounts the given merged CIM (usually created with `CreateMergedCim`) at the a volume with | ||
| // given GUID. The `cimPaths` MUST be identical to the `cimPaths` passed to `CreateMergedCim` when creating | ||
| // this merged CIM. | ||
| func MountMergedCims(cimPaths []string, mergedCimPath string, mountFlags uint32, volumeGUID guid.GUID) (string, error) { |
There was a problem hiding this comment.
My previous impression of merged cims was that each cim could be mounted at a specific subdir in the filesystem. Is that not the case?
There was a problem hiding this comment.
We won't need to explicitly mount individual CIMs, we just have to mount the final merged CIM.
| // MountMergedCims mounts the given merged CIM (usually created with `CreateMergedCim`) at the a volume with | ||
| // given GUID. The `cimPaths` MUST be identical to the `cimPaths` passed to `CreateMergedCim` when creating | ||
| // this merged CIM. | ||
| func MountMergedCims(cimPaths []string, mergedCimPath string, mountFlags uint32, volumeGUID guid.GUID) (string, error) { |
There was a problem hiding this comment.
Are any flags currently supported?
There was a problem hiding this comment.
All of the standard mount flags that are already defined here are supported. I will add two more flags to this for blocked CIMs when I open that PR.
| //sys CimMountImage(imagePath string, fsName string, flags uint32, volumeID *g) (hr error) = cimfs.CimMountImage? | ||
| //sys CimDismountImage(volumeID *g) (hr error) = cimfs.CimDismountImage? | ||
|
|
||
| //sys CimCreateImage(imagePath string, oldFSName *uint16, newFSName *uint16, cimFSHandle *FsHandle) (hr error) = cimfs.CimCreateImage? | ||
| //sys CimCloseImage(cimFSHandle FsHandle) = cimfs.CimCloseImage? | ||
| //sys CimCloseImage(cimFSHandle FsHandle) = cimfs.CimCloseImage |
There was a problem hiding this comment.
May be good to add ? to the end here, in case it's attempted on a build without the DLL, it will error instead of panicking.
There was a problem hiding this comment.
Yeah, I think there is a bug in the mkwinsyscall code where if the function has void return and you put a ?, it will still generate a return value for the syscall. I couldn't find a fix for that after a cursory look, so I changed it here. We can make a fix to mkwinsyscall later on and then update this.
CimFS supports merging CIMs and then mounting those merged CIMs as an alternative to creating forked CIMs. Advantage of merging CIMs that each CIM can be stored in its own directory. Unlike forked ICMs there is no requirement that all CIMs need to be present in the same directory. This commit adds the Go wrappers and some tests for using the new CIM merging APIs. Also, fixes a bug related to the return value of CimCloseImage API. This API has void return so we shouldn't be returning anything in our wrapper. Signed-off-by: Amit Barve <ambarve@microsoft.com>
|
Closing this as CimFS APIs for mounting & merging block CIMs have changed. I will open a new PR with the updated APIs. |
CimFS supports merging CIMs and then mounting those merged CIMs as an alternative to creating forked CIMs. Advantage of merging CIMs that each CIM can be stored in its own directory. Unlike forked ICMs there is no requirement that all CIMs need to be present in the same directory. This commit adds the Go wrappers and some tests for using the new CIM merging APIs.