Merge pull request #4643 from dcantah/feedback-lcow-snapshotter

Optimize Windows and LCOW snapshotters to only create scratch layer on the final snapshot

rootfs/apply.go

@@ -123,7 +123,7 @@ func applyLayers(ctx context.Context, layers []Layer, chain []digest.Digest, sn
 	)
 
 	for {
-		key = fmt.Sprintf("extract-%s %s", uniquePart(), chainID)
+		key = fmt.Sprintf(snapshots.UnpackKeyFormat, uniquePart(), chainID)
 
 		// Prepare snapshot with from parent, label as root
 		mounts, err = sn.Prepare(ctx, key, parent.String(), opts...)

snapshots/lcow/lcow.go

@@ -330,37 +330,44 @@ func (s *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k
 		for _, o := range opts {
 			o(&snapshotInfo)
 		}
-
+		// IO/disk space optimization
-		var sizeGB int
-		if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeLabel]; ok {
-			i32, err := strconv.ParseInt(sizeGBstr, 10, 32)
-			if err != nil {
-				return nil, errors.Wrapf(err, "failed to parse annotation %q=%q", rootfsSizeLabel, sizeGBstr)
-			}
-			sizeGB = int(i32)
-		}
-
-		scratchSource, err := s.openOrCreateScratch(ctx, sizeGB)
-		if err != nil {
-			return nil, err
-		}
-		defer scratchSource.Close()
-
-		// TODO: JTERRY75 - This has to be called sandbox.vhdx for the time
-		// being but it really is the scratch.vhdx. Using this naming convention
-		// for now but this is not the kubernetes sandbox.
 		//
-		// Create the sandbox.vhdx for this snapshot from the cache.
+		// We only need one sandbox.vhd for the container. Skip making one for this
-		destPath := filepath.Join(snDir, "sandbox.vhdx")
+		// snapshot if this isn't the snapshot that just houses the final sandbox.vhd
-		dest, err := os.OpenFile(destPath, os.O_RDWR|os.O_CREATE, 0700)
+		// that will be mounted as the containers scratch. Currently the key for a snapshot
-		if err != nil {
+		// where a layer.vhd will be extracted to it will have the string `extract-` in it.
-			return nil, errors.Wrap(err, "failed to create sandbox.vhdx in snapshot")
+		// If this is changed this will also need to be changed.
-		}
+		//
-		defer dest.Close()
+		// We save about 17MB per layer (if the default scratch vhd size of 20GB is used) and of
-		if _, err := io.Copy(dest, scratchSource); err != nil {
+		// course the time to copy the vhd per snapshot.
-			dest.Close()
+		if !strings.Contains(key, snapshots.UnpackKeyPrefix) {
-			os.Remove(destPath)
+			var sizeGB int
-			return nil, errors.Wrap(err, "failed to copy cached scratch.vhdx to sandbox.vhdx in snapshot")
+			if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeLabel]; ok {
+				i32, err := strconv.ParseInt(sizeGBstr, 10, 32)
+				if err != nil {
+					return nil, errors.Wrapf(err, "failed to parse label %q=%q", rootfsSizeLabel, sizeGBstr)
+				}
+				sizeGB = int(i32)
+			}
+
+			scratchSource, err := s.openOrCreateScratch(ctx, sizeGB)
+			if err != nil {
+				return nil, err
+			}
+			defer scratchSource.Close()
+
+			// Create the sandbox.vhdx for this snapshot from the cache.
+			destPath := filepath.Join(snDir, "sandbox.vhdx")
+			dest, err := os.OpenFile(destPath, os.O_RDWR|os.O_CREATE, 0700)
+			if err != nil {
+				return nil, errors.Wrap(err, "failed to create sandbox.vhdx in snapshot")
+			}
+			defer dest.Close()
+			if _, err := io.Copy(dest, scratchSource); err != nil {
+				dest.Close()
+				os.Remove(destPath)
+				return nil, errors.Wrap(err, "failed to copy cached scratch.vhdx to sandbox.vhdx in snapshot")
+			}
 		}
 	}
 

snapshots/snapshotter.go

@@ -26,6 +26,10 @@ import (
 )
 
 const (
+	// UnpackKeyPrefix is the beginning of the key format used for snapshots that will have
+	// image content unpacked into them.
+	UnpackKeyPrefix       = "extract"
+	UnpackKeyFormat       = UnpackKeyPrefix + "-%s %s"
 	inheritedLabelsPrefix = "containerd.io/snapshot/"
 	labelSnapshotRef      = "containerd.io/snapshot.ref"
 )

snapshots/windows/windows.go

@@ -326,35 +326,50 @@ func (s *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k
 	}
 
 	if kind == snapshots.KindActive {
-		parentLayerPaths := s.parentIDsToParentPaths(newSnapshot.ParentIDs)
+		log.G(ctx).Debug("createSnapshot active")
-
+		// Create the new snapshot dir
-		var parentPath string
+		snDir := s.getSnapshotDir(newSnapshot.ID)
-		if len(parentLayerPaths) != 0 {
+		if err := os.MkdirAll(snDir, 0700); err != nil {
-			parentPath = parentLayerPaths[0]
+			return nil, err
 		}
 
-		if err := hcsshim.CreateSandboxLayer(s.info, newSnapshot.ID, parentPath, parentLayerPaths); err != nil {
+		// IO/disk space optimization
-			return nil, errors.Wrap(err, "failed to create sandbox layer")
+		//
-		}
+		// We only need one sandbox.vhdx for the container. Skip making one for this
+		// 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.
+		if !strings.Contains(key, snapshots.UnpackKeyPrefix) {
+			parentLayerPaths := s.parentIDsToParentPaths(newSnapshot.ParentIDs)
 
-		var snapshotInfo snapshots.Info
+			var parentPath string
-		for _, o := range opts {
+			if len(parentLayerPaths) != 0 {
-			o(&snapshotInfo)
+				parentPath = parentLayerPaths[0]
-		}
-
-		var sizeGB int
-		if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeLabel]; ok {
-			i32, err := strconv.ParseInt(sizeGBstr, 10, 32)
-			if err != nil {
-				return nil, errors.Wrapf(err, "failed to parse annotation %q=%q", rootfsSizeLabel, sizeGBstr)
 			}
-			sizeGB = int(i32)
-		}
 
-		if sizeGB > 0 {
+			if err := hcsshim.CreateSandboxLayer(s.info, newSnapshot.ID, parentPath, parentLayerPaths); err != nil {
-			const gbToByte = 1024 * 1024 * 1024
+				return nil, errors.Wrap(err, "failed to create sandbox layer")
-			if err := hcsshim.ExpandSandboxSize(s.info, newSnapshot.ID, uint64(gbToByte*sizeGB)); err != nil {
+			}
-				return nil, errors.Wrapf(err, "failed to expand scratch size to %d GB", sizeGB)
+
+			var snapshotInfo snapshots.Info
+			for _, o := range opts {
+				o(&snapshotInfo)
+			}
+
+			var sizeGB int
+			if sizeGBstr, ok := snapshotInfo.Labels[rootfsSizeLabel]; ok {
+				i32, err := strconv.ParseInt(sizeGBstr, 10, 32)
+				if err != nil {
+					return nil, errors.Wrapf(err, "failed to parse label %q=%q", rootfsSizeLabel, sizeGBstr)
+				}
+				sizeGB = int(i32)
+			}
+
+			if sizeGB > 0 {
+				const gbToByte = 1024 * 1024 * 1024
+				if err := hcsshim.ExpandSandboxSize(s.info, newSnapshot.ID, uint64(gbToByte*sizeGB)); err != nil {
+					return nil, errors.Wrapf(err, "failed to expand scratch size to %d GB", sizeGB)
+				}
 			}
 		}
 	}

unpacker.go

@@ -138,7 +138,7 @@ EachLayer:
 
 		for try := 1; try <= 3; try++ {
 			// Prepare snapshot with from parent, label as root
-			key = fmt.Sprintf("extract-%s %s", uniquePart(), chainID)
+			key = fmt.Sprintf(snapshots.UnpackKeyFormat, uniquePart(), chainID)
 			mounts, err = sn.Prepare(ctx, key, parent.String(), opts...)
 			if err != nil {
 				if errdefs.IsAlreadyExists(err) {