Rework layer handling to return a ResourceCloser

Currently, the layers package relies on the caller of Mount*COWLayers to
subsequently call NewImageLayers, which constructs a special ImageLayers
object that can be used to later clean up the layer mounts. However,
this requires the caller to know too much about the internals of the
layer mounting process.

A cleaner approach, which I take here, is to instead return a standard
ResourceCloser from Mount*COWLayers which then knows how to clean up
whatever mounts were done. I have also changed the layers code to use
ResourceCloser in more places internally.

There is a new check in resources_*cow.go, such that the layers closer
is only stored if the container is not a hypervisor-isolated sandbox
container. This duplicates the logic that was previously in
(*ImageLayers).Release.

Signed-off-by: Kevin Parsons <kevpar@microsoft.com>

internal/hcsoci/resources_lcow.go

@@ -30,13 +30,16 @@ func allocateLinuxResources(ctx context.Context, coi *createOptionsInternal, r *
 	containerRootInUVM := r.ContainerRootInUVM()
 	if coi.Spec.Windows != nil && len(coi.Spec.Windows.LayerFolders) > 0 {
 		log.G(ctx).Debug("hcsshim::allocateLinuxResources mounting storage")
-		rootPath, scratchPath, err := layers.MountLCOWLayers(ctx, coi.actualID, coi.Spec.Windows.LayerFolders, containerRootInUVM, "", coi.HostingSystem)
+		rootPath, scratchPath, closer, err := layers.MountLCOWLayers(ctx, coi.actualID, coi.Spec.Windows.LayerFolders, containerRootInUVM, coi.HostingSystem)
 		if err != nil {
 			return errors.Wrap(err, "failed to mount container storage")
 		}
 		coi.Spec.Root.Path = rootPath
-		layers := layers.NewImageLayers(coi.HostingSystem, containerRootInUVM, coi.Spec.Windows.LayerFolders, "", isSandbox)
-		r.SetLayers(layers)
+		// If this is the pause container in a hypervisor-isolated pod, we can skip cleanup of
+		// layers, as that happens automatically when the UVM is terminated.
+		if !isSandbox || coi.HostingSystem == nil {
+			r.SetLayers(closer)
+		}
 		r.SetLcowScratchPath(scratchPath)
 	} else if coi.Spec.Root.Path != "" {
 		// This is the "Plan 9" root filesystem.

internal/hcsoci/resources_wcow.go

@@ -60,13 +60,16 @@ func allocateWindowsResources(ctx context.Context, coi *createOptionsInternal, r
 	if coi.Spec.Root.Path == "" && (coi.HostingSystem != nil || coi.Spec.Windows.HyperV == nil) {
 		log.G(ctx).Debug("hcsshim::allocateWindowsResources mounting storage")
 		containerRootInUVM := r.ContainerRootInUVM()
-		containerRootPath, err := layers.MountWCOWLayers(ctx, coi.actualID, coi.Spec.Windows.LayerFolders, containerRootInUVM, "", coi.HostingSystem)
+		containerRootPath, closer, err := layers.MountWCOWLayers(ctx, coi.actualID, coi.Spec.Windows.LayerFolders, containerRootInUVM, "", coi.HostingSystem)
 		if err != nil {
 			return errors.Wrap(err, "failed to mount container storage")
 		}
 		coi.Spec.Root.Path = containerRootPath
-		layers := layers.NewImageLayers(coi.HostingSystem, containerRootInUVM, coi.Spec.Windows.LayerFolders, "", isSandbox)
-		r.SetLayers(layers)
+		// If this is the pause container in a hypervisor-isolated pod, we can skip cleanup of
+		// layers, as that happens automatically when the UVM is terminated.
+		if !isSandbox || coi.HostingSystem == nil {
+			r.SetLayers(closer)
+		}
 	}
 
 	if err := setupMounts(ctx, coi, r); err != nil {

internal/jobcontainers/jobcontainer.go

@@ -21,7 +21,6 @@ import (
 	"github.com/Microsoft/hcsshim/internal/hcs/schema1"
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
 	"github.com/Microsoft/hcsshim/internal/jobobject"
-	"github.com/Microsoft/hcsshim/internal/layers"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/queue"
 	"github.com/Microsoft/hcsshim/internal/resources"
@@ -189,25 +188,16 @@ func Create(ctx context.Context, id string, s *specs.Spec) (_ cow.Container, _ *
 		}
 	})
 
+	var closer resources.ResourceCloser
 	if fileBindingSupport {
-		if err := container.bindSetup(ctx, s); err != nil {
-			return nil, nil, err
-		}
+		closer, err = container.bindSetup(ctx, s)
 	} else {
-		if err := container.fallbackSetup(ctx, s); err != nil {
-			return nil, nil, err
-		}
+		closer, err = container.fallbackSetup(ctx, s)
 	}
-
-	// We actually don't need to pass in anything for volumeMountPath below if we have file binding support,
-	// the filter allows us to pass in a raw volume path and it can use that to bind a volume to a friendly path
-	// instead so we can skip calling SetVolumeMountPoint.
-	var rootfsMountPoint string
-	if !fileBindingSupport {
-		rootfsMountPoint = container.rootfsLocation
+	if err != nil {
+		return nil, nil, err
 	}
-	layers := layers.NewImageLayers(nil, "", s.Windows.LayerFolders, rootfsMountPoint, false)
-	r.SetLayers(layers)
+	r.SetLayers(closer)
 
 	volumeGUIDRegex := `^\\\\\?\\(Volume)\{{0,1}[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}(\}){0,1}\}(|\\)$`
 	if matched, err := regexp.MatchString(volumeGUIDRegex, s.Root.Path); !matched || err != nil {
@@ -775,37 +765,56 @@ func (c *JobContainer) replaceWithMountPoint(str string) (string, bool) {
 	return newStr, str != newStr
 }
 
-func (c *JobContainer) bindSetup(ctx context.Context, s *specs.Spec) (err error) {
+func (c *JobContainer) bindSetup(ctx context.Context, s *specs.Spec) (_ resources.ResourceCloser, err error) {
 	// Must be upgraded to a silo so we can get per silo bindings for the container.
 	if err := c.job.PromoteToSilo(); err != nil {
-		return err
+		return nil, err
 	}
 	// Union the container layers.
-	if err := c.mountLayers(ctx, c.id, s, ""); err != nil {
-		return fmt.Errorf("failed to mount container layers: %w", err)
+	closer, err := c.mountLayers(ctx, c.id, s, "")
+	if err != nil {
+		return nil, fmt.Errorf("failed to mount container layers: %w", err)
 	}
+	defer func() {
+		if err != nil {
+			_ = closer.Release(ctx)
+		}
+	}()
+
 	rootfsLocation := defaultSiloRootfsLocation
 	if loc := customRootfsLocation(s.Annotations); loc != "" {
 		rootfsLocation = loc
 	}
 
 	if err := c.setupRootfsBinding(rootfsLocation, s.Root.Path); err != nil {
-		return err
+		return nil, err
 	}
 	c.rootfsLocation = rootfsLocation
-	return c.setupMounts(ctx, s)
+	if err := c.setupMounts(ctx, s); err != nil {
+		return nil, err
+	}
+	return closer, nil
 }
 
 // This handles the fallback case where bind mounting isn't available on the machine. This mounts the
 // container layers on the host and sets up any mounts present in the OCI runtime spec.
-func (c *JobContainer) fallbackSetup(ctx context.Context, s *specs.Spec) (err error) {
+func (c *JobContainer) fallbackSetup(ctx context.Context, s *specs.Spec) (_ resources.ResourceCloser, err error) {
 	rootfsLocation := fmt.Sprintf(fallbackRootfsFormat, c.id)
 	if loc := customRootfsLocation(s.Annotations); loc != "" {
 		rootfsLocation = filepath.Join(loc, c.id)
 	}
-	if err := c.mountLayers(ctx, c.id, s, rootfsLocation); err != nil {
-		return fmt.Errorf("failed to mount container layers: %w", err)
+	closer, err := c.mountLayers(ctx, c.id, s, rootfsLocation)
+	if err != nil {
+		return nil, fmt.Errorf("failed to mount container layers: %w", err)
 	}
+	defer func() {
+		if err != nil {
+			_ = closer.Release(ctx)
+		}
+	}()
 	c.rootfsLocation = rootfsLocation
-	return fallbackMountSetup(s, c.rootfsLocation)
+	if err := fallbackMountSetup(s, c.rootfsLocation); err != nil {
+		return nil, err
+	}
+	return closer, nil
 }

internal/jobcontainers/storage.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/Microsoft/hcsshim/internal/layers"
 	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/resources"
 	"github.com/Microsoft/hcsshim/internal/wclayer"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
@@ -25,40 +26,42 @@ const fallbackRootfsFormat = `C:\hpc\%s\`
 // C:\hpc\<containerID>
 const defaultSiloRootfsLocation = `C:\hpc\`
 
-func (c *JobContainer) mountLayers(ctx context.Context, containerID string, s *specs.Spec, volumeMountPath string) (err error) {
+func (c *JobContainer) mountLayers(ctx context.Context, containerID string, s *specs.Spec, volumeMountPath string) (_ resources.ResourceCloser, err error) {
 	if s == nil || s.Windows == nil || s.Windows.LayerFolders == nil {
-		return errors.New("field 'Spec.Windows.Layerfolders' is not populated")
+		return nil, errors.New("field 'Spec.Windows.Layerfolders' is not populated")
 	}
 
 	// Last layer always contains the sandbox.vhdx, or 'scratch' space for the container.
 	scratchFolder := s.Windows.LayerFolders[len(s.Windows.LayerFolders)-1]
 	if _, err := os.Stat(scratchFolder); os.IsNotExist(err) {
 		if err := os.MkdirAll(scratchFolder, 0777); err != nil {
-			return fmt.Errorf("failed to auto-create container scratch folder %s: %w", scratchFolder, err)
+			return nil, fmt.Errorf("failed to auto-create container scratch folder %s: %w", scratchFolder, err)
 		}
 	}
 
 	// Create sandbox.vhdx if it doesn't exist in the scratch folder.
 	if _, err := os.Stat(filepath.Join(scratchFolder, "sandbox.vhdx")); os.IsNotExist(err) {
 		if err := wclayer.CreateScratchLayer(ctx, scratchFolder, s.Windows.LayerFolders[:len(s.Windows.LayerFolders)-1]); err != nil {
-			return fmt.Errorf("failed to CreateSandboxLayer: %w", err)
+			return nil, fmt.Errorf("failed to CreateSandboxLayer: %w", err)
 		}
 	}
 
 	if s.Root == nil {
 		s.Root = &specs.Root{}
 	}
 
+	var closer resources.ResourceCloser
 	if s.Root.Path == "" {
 		log.G(ctx).Debug("mounting job container storage")
-		rootPath, err := layers.MountWCOWLayers(ctx, containerID, s.Windows.LayerFolders, "", volumeMountPath, nil)
+		var rootPath string
+		rootPath, closer, err = layers.MountWCOWLayers(ctx, containerID, s.Windows.LayerFolders, "", volumeMountPath, nil)
 		if err != nil {
-			return fmt.Errorf("failed to mount job container storage: %w", err)
+			return nil, fmt.Errorf("failed to mount job container storage: %w", err)
 		}
 		s.Root.Path = rootPath + "\\"
 	}
 
-	return nil
+	return closer, nil
 }
 
 // setupRootfsBinding binds the copy on write volume for the container to a static path