From 95687a9324821d33c28dfdd55274390cc2044598 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 2 Mar 2023 10:32:15 -0800 Subject: [PATCH] Fix go.mod, simplify boolean logic, add logging Signed-off-by: Gabriel Adrian Samfira --- integration/client/go.mod | 3 +- mount/mount_windows.go | 18 +++++--- snapshots/windows/windows.go | 82 ++++++++++++++++++------------------ 3 files changed, 56 insertions(+), 47 deletions(-) diff --git a/integration/client/go.mod b/integration/client/go.mod index b68261e67..d8b4c97f4 100644 --- a/integration/client/go.mod +++ b/integration/client/go.mod @@ -4,6 +4,7 @@ go 1.19 require ( github.com/AdaLogics/go-fuzz-headers v0.0.0-20230106234847-43070de90fa1 + github.com/Microsoft/go-winio v0.6.1-0.20230228163719-dd5de6900b62 github.com/Microsoft/hcsshim v0.10.0-rc.7 github.com/Microsoft/hcsshim/test v0.0.0-20210408205431-da33ecd607e1 github.com/containerd/cgroups/v3 v3.0.1 @@ -22,8 +23,6 @@ require ( golang.org/x/sys v0.6.0 ) -require github.com/Microsoft/go-winio v0.6.1-0.20230228163719-dd5de6900b62 - require ( github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20221215162035-5330a85ea652 // indirect github.com/cilium/ebpf v0.9.1 // indirect diff --git a/mount/mount_windows.go b/mount/mount_windows.go index fd4a2a60a..3f29d1b4e 100644 --- a/mount/mount_windows.go +++ b/mount/mount_windows.go @@ -17,16 +17,17 @@ package mount import ( + "context" "encoding/json" "errors" "fmt" "os" "path/filepath" "strings" - "syscall" "github.com/Microsoft/go-winio/pkg/bindfilter" "github.com/Microsoft/hcsshim" + "github.com/containerd/containerd/log" "golang.org/x/sys/windows" ) @@ -74,16 +75,21 @@ func (m *Mount) mount(target string) error { } defer func() { if err != nil { - hcsshim.DeactivateLayer(di, layerID) + if layerErr := hcsshim.DeactivateLayer(di, layerID); layerErr != nil { + log.G(context.TODO()).WithError(layerErr).Error("failed to deactivate layer during mount failure cleanup") + } } }() if err = hcsshim.PrepareLayer(di, layerID, parentLayerPaths); err != nil { return fmt.Errorf("failed to prepare layer %s: %w", m.Source, err) } + defer func() { if err != nil { - hcsshim.UnprepareLayer(di, layerID) + if layerErr := hcsshim.UnprepareLayer(di, layerID); layerErr != nil { + log.G(context.TODO()).WithError(layerErr).Error("failed to unprepare layer during mount failure cleanup") + } } }() @@ -97,7 +103,9 @@ func (m *Mount) mount(target string) error { } defer func() { if err != nil { - bindfilter.RemoveFileBinding(target) + if bindErr := bindfilter.RemoveFileBinding(target); bindErr != nil { + log.G(context.TODO()).WithError(bindErr).Error("failed to remove binding during mount failure cleanup") + } } }() @@ -144,7 +152,7 @@ func Unmount(mount string, flags int) error { } if err := bindfilter.RemoveFileBinding(mount); err != nil { - if errno, ok := errors.Unwrap(err).(syscall.Errno); ok && errno == windows.ERROR_INVALID_PARAMETER || errno == windows.ERROR_NOT_FOUND { + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) || errors.Is(err, windows.ERROR_NOT_FOUND) { // not a mount point return nil } diff --git a/snapshots/windows/windows.go b/snapshots/windows/windows.go index c20a766b4..25b87300a 100644 --- a/snapshots/windows/windows.go +++ b/snapshots/windows/windows.go @@ -362,7 +362,7 @@ func (s *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k // Create the new snapshot dir snDir := s.getSnapshotDir(newSnapshot.ID) if err = os.MkdirAll(snDir, 0700); err != nil { - return err + return fmt.Errorf("creating snapshot dir: %w", err) } if strings.Contains(key, snapshots.UnpackKeyPrefix) { @@ -372,57 +372,59 @@ func (s *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k // snapshot if this isn't the snapshot that just houses the final sandbox.vhd // that will be mounted as the containers scratch. Currently the key for a snapshot // where a layer will be extracted to will have the string `extract-` in it. - } else if len(newSnapshot.ParentIDs) == 0 { + return nil + } + + if len(newSnapshot.ParentIDs) == 0 { // A parentless snapshot is just a bind-mount to a directory named // "Files". When committed, there'll be some post-processing to fill in the rest // of the metadata. filesDir := filepath.Join(snDir, "Files") if err := os.MkdirAll(filesDir, 0700); err != nil { - return err + return fmt.Errorf("creating Files dir: %w", err) } - } else { - parentLayerPaths := s.parentIDsToParentPaths(newSnapshot.ParentIDs) + return nil + } - var snapshotInfo snapshots.Info - for _, o := range opts { - o(&snapshotInfo) + parentLayerPaths := s.parentIDsToParentPaths(newSnapshot.ParentIDs) + var snapshotInfo snapshots.Info + for _, o := range opts { + o(&snapshotInfo) + } + + var sizeInBytes uint64 + if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeInGBLabel]; ok { + log.G(ctx).Warnf("%q label is deprecated, please use %q instead.", rootfsSizeInGBLabel, rootfsSizeInBytesLabel) + + sizeInGB, err := strconv.ParseUint(sizeGBstr, 10, 32) + if err != nil { + return fmt.Errorf("failed to parse label %q=%q: %w", rootfsSizeInGBLabel, sizeGBstr, err) } + sizeInBytes = sizeInGB * 1024 * 1024 * 1024 + } - var sizeInBytes uint64 - if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeInGBLabel]; ok { - log.G(ctx).Warnf("%q label is deprecated, please use %q instead.", rootfsSizeInGBLabel, rootfsSizeInBytesLabel) - - sizeInGB, err := strconv.ParseUint(sizeGBstr, 10, 32) - if err != nil { - return fmt.Errorf("failed to parse label %q=%q: %w", rootfsSizeInGBLabel, sizeGBstr, err) - } - sizeInBytes = sizeInGB * 1024 * 1024 * 1024 - } - - // Prefer the newer label in bytes over the deprecated Windows specific GB variant. - if sizeBytesStr, ok := snapshotInfo.Labels[rootfsSizeInBytesLabel]; ok { - sizeInBytes, err = strconv.ParseUint(sizeBytesStr, 10, 64) - if err != nil { - return fmt.Errorf("failed to parse label %q=%q: %w", rootfsSizeInBytesLabel, sizeBytesStr, err) - } - } - - var makeUVMScratch bool - if _, ok := snapshotInfo.Labels[uvmScratchLabel]; ok { - makeUVMScratch = true - } - - // This has to be run first to avoid clashing with the containers sandbox.vhdx. - if makeUVMScratch { - if err = s.createUVMScratchLayer(ctx, snDir, parentLayerPaths); err != nil { - return fmt.Errorf("failed to make UVM's scratch layer: %w", err) - } - } - if err = s.createScratchLayer(ctx, snDir, parentLayerPaths, sizeInBytes); err != nil { - return fmt.Errorf("failed to create scratch layer: %w", err) + // Prefer the newer label in bytes over the deprecated Windows specific GB variant. + if sizeBytesStr, ok := snapshotInfo.Labels[rootfsSizeInBytesLabel]; ok { + sizeInBytes, err = strconv.ParseUint(sizeBytesStr, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse label %q=%q: %w", rootfsSizeInBytesLabel, sizeBytesStr, err) } } + var makeUVMScratch bool + if _, ok := snapshotInfo.Labels[uvmScratchLabel]; ok { + makeUVMScratch = true + } + + // This has to be run first to avoid clashing with the containers sandbox.vhdx. + if makeUVMScratch { + if err = s.createUVMScratchLayer(ctx, snDir, parentLayerPaths); err != nil { + return fmt.Errorf("failed to make UVM's scratch layer: %w", err) + } + } + if err = s.createScratchLayer(ctx, snDir, parentLayerPaths, sizeInBytes); err != nil { + return fmt.Errorf("failed to create scratch layer: %w", err) + } return nil }) if err != nil {