Minor CimFS bug fixes (#1980)

* Minor fixes for cimfs writer

Adds minor fixes like updating the Windows build which supports CimFS, using safefile for creating directories in CimFS writer etc.


* Always expand volume when expanding sandbox VHD

Currently, ExpandScratchSize or ExpandSandboxSize functions expand the VHD itself but don't expand the volume
on that VHD (unless we are on 19H1 & build < 19020). This works because for legacy layers the PrepareLayer
call made just before starting the container will automatically expand the volume to match the size of the
VHD. However, in case of CimFS layers we don't call PrepareLayer at all, so in that case we need to expand the
volume at the time of expanding the VHD.

This also means in case of legacy layers, we might have a small perf hit because the VHD is mounted twice for
expansion (once here and once during the PrepareLayer call). But as long as the perf hit is minimal, we should
be okay.

Signed-off-by: Amit Barve <ambarve@microsoft.com>

internal/wclayer/cim/file_writer.go

@@ -56,7 +56,7 @@ func (sfw *stdFileWriter) Add(name string) error {
 
 	// The directory of this file might be created inside the cim.
 	// make sure we have the same parent directory chain here
-	if err := os.MkdirAll(filepath.Join(sfw.path, filepath.Dir(name)), 0755); err != nil {
+	if err := safefile.MkdirAllRelative(filepath.Dir(name), sfw.root); err != nil {
 		return fmt.Errorf("failed to create file %s: %w", name, err)
 	}
 

internal/wclayer/expandscratchsize.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/Microsoft/hcsshim/internal/hcserror"
 	"github.com/Microsoft/hcsshim/internal/oc"
-	"github.com/Microsoft/hcsshim/osversion"
 	"go.opencensus.io/trace"
 )
 
@@ -30,14 +29,17 @@ func ExpandScratchSize(ctx context.Context, path string, size uint64) (err error
 		return hcserror.New(err, title, "")
 	}
 
-	// Manually expand the volume now in order to work around bugs in 19H1 and
-	// prerelease versions of Vb. Remove once this is fixed in Windows.
-	if build := osversion.Build(); build >= osversion.V19H1 && build < 19020 {
-		err = expandSandboxVolume(ctx, path)
-		if err != nil {
-			return err
-		}
+	// Always expand the volume too. In case of legacy layers not expanding the volume here works because
+	// the PrepareLayer call internally handles the expansion. However, in other cases (like CimFS) we
+	// don't call PrepareLayer and so the volume will never be expanded.  This also means in case of
+	// legacy layers, we might have a small perf hit because the VHD is mounted twice for expansion (once
+	// here and once during the PrepareLayer call). But as long as the perf hit is minimal, we should be
+	// okay.
+	err = expandSandboxVolume(ctx, path)
+	if err != nil {
+		return err
 	}
+
 	return nil
 }
 

internal/winapi/cimfs.go

@@ -36,7 +36,7 @@ type CimFsFileMetadata struct {
 //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) (hr error) = cimfs.CimCloseImage?
+//sys CimCloseImage(cimFSHandle FsHandle) = cimfs.CimCloseImage?
 //sys CimCommitImage(cimFSHandle FsHandle) (hr error) = cimfs.CimCommitImage?
 
 //sys CimCreateFile(cimFSHandle FsHandle, path string, file *CimFsFileMetadata, cimStreamHandle *StreamHandle) (hr error) = cimfs.CimCreateFile?

pkg/cimfs/cim_writer_windows.go

@@ -8,11 +8,13 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"unsafe"
 
 	"github.com/Microsoft/go-winio"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/winapi"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/windows"
 )
 
@@ -128,6 +130,8 @@ func (c *CimFsWriter) AddFile(path string, info *winio.FileBasicInfo, fileSize i
 		fileMetadata.ExtendedAttributes = unsafe.Pointer(&extendedAttributes[0])
 		fileMetadata.EACount = uint32(len(extendedAttributes))
 	}
+	// remove the trailing `\` if present, otherwise it trips off the cim writer
+	path = strings.TrimSuffix(path, "\\")
 	err = winapi.CimCreateFile(c.handle, path, fileMetadata, &c.activeStream)
 	if err != nil {
 		return &PathError{Cim: c.name, Op: "addFile", Path: path, Err: err}
@@ -231,6 +235,12 @@ func DestroyCim(ctx context.Context, cimPath string) (retErr error) {
 		}
 	}
 
+	log.G(ctx).WithFields(logrus.Fields{
+		"cimPath":     cimPath,
+		"regionFiles": regionFilePaths,
+		"objectFiles": objectFilePaths,
+	}).Debug("destroy cim")
+
 	for _, regFilePath := range regionFilePaths {
 		if err := os.Remove(regFilePath); err != nil {
 			log.G(ctx).WithError(err).Warnf("remove file %s", regFilePath)

pkg/cimfs/cimfs.go

@@ -13,5 +13,5 @@ func IsCimFSSupported() bool {
 	if err != nil {
 		logrus.WithError(err).Warn("get build revision")
 	}
-	return osversion.Build() == 20348 && rv >= 1700
+	return osversion.Build() == 20348 && rv >= 2031
 }

pkg/ociwclayer/cim/import.go

@@ -25,6 +25,10 @@ import (
 // ImportCimLayerFromTar reads a layer from an OCI layer tar stream and extracts it into
 // the CIM format at the specified path. The caller must specify the parent layers, if
 // any, ordered from lowest to highest layer.
+// This function expects that the layer paths (both the layer that is being imported & the parent layers) are
+// formatted like `.../snapshots/<id>` and the corresponding layer CIMs are located/will be created at
+// `.../snapshots/cim-layers/<id>.cim`. Each CIM file also has corresponding region & objectID files and those
+// files will also be stored inside the `cim-layers` directory.
 //
 // This function returns the total size of the layer's files, in bytes.
 func ImportCimLayerFromTar(ctx context.Context, r io.Reader, layerPath string, parentLayerPaths []string) (int64, error) {