WIP: Batch disk operations

Makefile

@@ -212,7 +212,7 @@ whitespace:
 	@infra/scripts/whitespace-check.sh
 
 # exit 1 if golint complains about anything other than comments
-golintf = $(GOLINT) $(1) | sh -c "! grep -v 'lib/apiservers/portlayer/restapi/operations'" | sh -c "! grep -v 'lib/config/dynamic/admiral/client'" | sh -c "! grep -v 'should have comment'" | sh -c "! grep -v 'comment on exported'" | sh -c "! grep -v 'by other packages, and that stutters'" | sh -c "! grep -v 'error strings should not be capitalized'"
+golintf = $(GOLINT) $(1) | sh -c "! grep -v 'lib/apiservers/portlayer/restapi/operations'" | sh -c "! grep -v 'lib/apiservers/service/restapi/operations/not_yet_implemented'" | sh -c "! grep -v 'lib/config/dynamic/admiral/client'" | sh -c "! grep -v 'should have comment'" | sh -c "! grep -v 'comment on exported'" | sh -c "! grep -v 'by other packages, and that stutters'" | sh -c "! grep -v 'error strings should not be capitalized'"
 
 golint: $(GOLINT)
 	@echo checking go lint...

cmd/port-layer-server/main.go

@@ -23,6 +23,7 @@ import (
 	"github.com/go-openapi/loads"
 	"github.com/jessevdk/go-flags"
 
+	"github.com/vmware/govmomi/vim25/debug"
 	"github.com/vmware/vic/lib/apiservers/portlayer/restapi"
 	"github.com/vmware/vic/lib/apiservers/portlayer/restapi/operations"
 	"github.com/vmware/vic/lib/config"
@@ -98,6 +99,22 @@ func main() {
 		extraconfig.SetLogLevel(log.DebugLevel)
 	}
 
+	if vchConfig.Diagnostics.DebugLevel > 4 {
+		// TODO: pull this from an env var or similar
+		debugDir := "/var/log/vic/govmomi"
+
+		err := os.MkdirAll(debugDir, 0700)
+		if err != nil {
+			log.Fatalf("Unable to create govmomi debug directory: %s", err)
+		}
+
+		p := debug.FileProvider{
+			Path: debugDir,
+		}
+
+		debug.SetProvider(&p)
+	}
+
 	if vchConfig.Diagnostics.SysLogConfig != nil {
 		logcfg.Syslog = &viclog.SyslogConfig{
 			Network:  vchConfig.Diagnostics.SysLogConfig.Network,

infra/scripts/bash-helpers.sh

@@ -30,7 +30,7 @@ fi
 # 1: release numnber, e.g. 1.4.1 or 1.4.1-rc2, or build id, e.g. 19653
 vic-set-version () {
     version="$1"
-    
+
     if [ "$1" == "" ]; then
         unset VIC_VERSION
         return
@@ -63,7 +63,7 @@ init-profile () {
 
 unset-vic () {
     unset TARGET_URL MAPPED_NETWORKS NETWORKS IMAGE_STORE DATASTORE COMPUTE VOLUME_STORES IPADDR TLS THUMBPRINT OPS_CREDS VIC_NAME PRESERVE_VOLUMES
-    unset GOVC_URL GOVC_INSECURE GOVC_DATACENTER GOVC_USERNAME GOVC_PASSWORD GOVC_DATASTORE GOVC_CERTIFICATE 
+    unset GOVC_URL GOVC_INSECURE GOVC_DATACENTER GOVC_USERNAME GOVC_PASSWORD GOVC_DATASTORE GOVC_CERTIFICATE
 
     unset vsphere datacenter user password opsuser opspass opsgrant timeout compute datastore dns publicNet publicIP publicGW bridgeNet bridgeRange
     unset clientNet clientIP clientGW managementNet managementIP managementGW tls volumestores preserveVolumestores containernet
@@ -79,7 +79,7 @@ vic-create () {
     base=$(pwd)
     (
         cd "$(vic-path)"/bin || return
-        "$(vic-path)/bin/${VIC_VERSION}/vic-machine-$OS" create --target="$TARGET_URL" "${OPS_CREDS[@]}" --image-store="$IMAGE_STORE" --compute-resource="$COMPUTE" "${TLS[@]}" ${TLS_OPTS} --name="${VIC_NAME:-${USER}test}" "${MAPPED_NETWORKS[@]}" "${VOLUME_STORES[@]}" "${NETWORKS[@]}" ${IPADDR} ${TIMEOUT} --thumbprint="$THUMBPRINT" --ai="${VIC_VERSION}/appliance.iso" --bi="${VIC_VERSION}/bootstrap.iso" "$@"
+        "$(vic-path)/bin/${VIC_VERSION}/vic-machine-$OS" create --target="$TARGET_URL" "${OPS_CREDS[@]}" --image-store="$IMAGE_STORE" --compute-resource="$COMPUTE" "${TLS[@]}" ${TLS_OPTS} --name="${VIC_NAME:-${USER}test}" "${MAPPED_NETWORKS[@]}" "${VOLUME_STORES[@]}" "${NETWORKS[@]}" ${IPADDR} --timeout=${TIMEOUT} --thumbprint="$THUMBPRINT" --ai="${VIC_VERSION}/appliance.iso" --bi="${VIC_VERSION}/bootstrap.iso" "$@"
     )
 
     vic-select

lib/apiservers/engine/backends/container.go

@@ -709,7 +709,7 @@ func (c *ContainerBackend) ContainerCreate(config types.ContainerCreateConfig) (
 
 	// Reserve the container name to prevent duplicates during a parallel operation.
 	if config.Name != "" {
-		err := cache.ContainerCache().ReserveName(container, config.Name)
+		err = cache.ContainerCache().ReserveName(container, config.Name)
 		if err != nil {
 			return containertypes.ContainerCreateCreatedBody{}, derr.NewRequestConflictError(err)
 		}
@@ -1856,7 +1856,7 @@ func clientFriendlyContainerName(name string) string {
 func createInternalVicContainer(image *metadata.ImageConfig) (*viccontainer.VicContainer, error) {
 	// provide basic container config via the image
 	container := viccontainer.NewVicContainer()
-	container.LayerID = image.V1Image.ID // store childmost layer ID to map to the proper vmdk
+	container.LayerID = image.VMDK // store childmost layer ID to map to the proper vmdk
 	container.ImageID = image.ImageID
 	container.Config = image.Config //Set defaults.  Overrides will get copied below.
 

lib/apiservers/engine/backends/image.go

@@ -129,7 +129,7 @@ func (i *ImageBackend) ImageDelete(imageRef string, force, prune bool) ([]types.
 			keepNodes[idx] = imgURL.String()
 		}
 
-		params := storage.NewDeleteImageParamsWithContext(op).WithStoreName(storeName).WithID(img.ID).WithKeepNodes(keepNodes)
+		params := storage.NewDeleteImageParamsWithContext(op).WithStoreName(storeName).WithID(img.VMDK).WithKeepNodes(keepNodes)
 		// TODO: This will fail if any containerVMs are referencing the vmdk - vanilla docker
 		// allows the removal of an image (via force flag) even if a container is referencing it
 		// should vic?

lib/apiservers/engine/proxy/storage_proxy.go

@@ -21,6 +21,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 
 	log "github.com/Sirupsen/logrus"
 	"github.com/google/uuid"
@@ -95,7 +96,7 @@ var SupportedVolDrivers = map[string]struct{}{
 	"local":   {},
 }
 
-//Validation pattern for Volume Names
+// Validation pattern for Volume Names
 var volumeNameRegex = regexp.MustCompile("^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
 
 func NewStorageProxy(client *client.PortLayer) VicStorageProxy {
@@ -248,6 +249,7 @@ func (s *StorageProxy) Remove(ctx context.Context, name string) error {
 // - imageID is the current label by which we address this image and is recorded as metadata
 // - repoName is the repository for the image and is recorded as metadata
 // returns:
+//
 //	modified handle
 func (s *StorageProxy) AddImageToContainer(ctx context.Context, handle, deltaID, layerID, imageID string, config types.ContainerCreateConfig) (string, error) {
 	op := trace.FromContext(ctx, "AddImageToContainer: %s", deltaID)
@@ -299,6 +301,7 @@ func (s *StorageProxy) AddImageToContainer(ctx context.Context, handle, deltaID,
 // If an error is returned, the returned handle should not be used.
 //
 // returns:
+//
 //	modified handle
 func (s *StorageProxy) AddVolumesToContainer(ctx context.Context, handle string, config types.ContainerCreateConfig) (string, error) {
 	op := trace.FromContext(ctx, "AddVolumesToContainer: %s", handle)
@@ -340,33 +343,52 @@ func (s *StorageProxy) AddVolumesToContainer(ctx context.Context, handle string,
 		}
 	}
 
-	// Create and join volumes.
+	// Create volumes in parallel
+	results := make(chan error, len(volList))
+	wg := sync.WaitGroup{}
+	wg.Add(len(volList))
 	for _, fields := range volList {
-		// We only set these here for volumes made on a docker create
-		volumeData := make(map[string]string)
-		volumeData[DriverArgFlagKey] = fields.Flags
-		volumeData[DriverArgContainerKey] = config.Name
-		volumeData[DriverArgImageKey] = config.Config.Image
-
-		// NOTE: calling volumeCreate regardless of whether the volume is already
-		// present can be avoided by adding an extra optional param to VolumeJoin,
-		// which would then call volumeCreate if the volume does not exist.
-		_, err := s.volumeCreate(op, fields.ID, "vsphere", volumeData, nil)
-		if err != nil {
+		go func(id, flags string) {
+			defer wg.Done()
+
+			// We only set these here for volumes made on a docker create
+			volumeData := make(map[string]string)
+			volumeData[DriverArgFlagKey] = flags
+			volumeData[DriverArgContainerKey] = config.Name
+			volumeData[DriverArgImageKey] = config.Config.Image
+
+			// NOTE: calling volumeCreate regardless of whether the volume is already
+			// present can be avoided by adding an extra optional param to VolumeJoin,
+			// which would then call volumeCreate if the volume does not exist.
+			_, err := s.volumeCreate(op, id, "vsphere", volumeData, nil)
+			if err == nil {
+				log.Infof("volumeCreate succeeded. Volume mount section ID: %s", id)
+				return
+			}
+
 			switch err := err.(type) {
 			case *storage.CreateVolumeConflict:
 				// Implicitly ignore the error where a volume with the same name
 				// already exists. We can just join the said volume to the container.
-				log.Infof("a volume with the name %s already exists", fields.ID)
+				log.Infof("a volume with the name %s already exists", id)
 			case *storage.CreateVolumeNotFound:
-				return handle, errors.VolumeCreateNotFoundError(volumeStore(volumeData))
+				results <- errors.VolumeCreateNotFoundError(volumeStore(volumeData))
 			default:
-				return handle, errors.InternalServerError(err.Error())
+				results <- errors.InternalServerError(err.Error())
 			}
-		} else {
-			log.Infof("volumeCreate succeeded. Volume mount section ID: %s", fields.ID)
-		}
+		}(fields.ID, fields.Flags)
+	}
 
+	wg.Wait()
+	if len(results) > 0 {
+		// TODO: should we attempt to do anything with errors other than the first?
+		// given we're not logging errors at this point prior to the parallelization I don't
+		// think it's needed.
+		return handle, <-results
+	}
+
+	// Attach volumes.
+	for _, fields := range volList {
 		flags := make(map[string]string)
 		//NOTE: for now we are passing the flags directly through. This is NOT SAFE and only a stop gap.
 		flags[constants.Mode] = fields.Flags

lib/imagec/docker.go

@@ -44,8 +44,14 @@ import (
 
 const (
 	// DigestSHA256EmptyTar is the canonical sha256 digest of empty tar file -
-	// (1024 NULL bytes)
+	// (1024 NULL bytes) - aka, the "empty" layer diffID.
 	DigestSHA256EmptyTar = string(dlayer.DigestSHA256EmptyTar)
+
+	// DigestSHA256EmptyBlobSum is the canonical sha256 digest of a gzipped empty tar file -
+	// (1024 NULL bytes, gzipped) - aka, the "empty" blobsum. This is used to identify empty
+	// layers using the schema v1 manifest without having to download, unzip and sha256 the
+	// 1024-byte empty layer tar.
+	DigestSHA256EmptyBlobSum = "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4"
 )
 
 // FSLayer is a container struct for BlobSums defined in an image manifest

lib/imagec/download.go

@@ -33,6 +33,11 @@ const (
 	maxConcurrentDownloads = 3
 )
 
+var (
+	vmdkMap   map[string]string
+	leafLayer *ImageWithMeta
+)
+
 type downloadTransfer struct {
 	xfer.Transfer
 
@@ -128,12 +133,19 @@ func (ldm *LayerDownloader) DownloadLayers(ctx context.Context, ic *ImageC) erro
 	// Grab the imageLayers
 	layers := ic.ImageLayers
 
+	vmdkMap = getVMDKMap(layers)
+
 	// iterate backwards through layers to download
 	for i := len(layers) - 1; i >= 0; i-- {
 
 		layer := layers[i]
-		id := layer.ID
 
+		if layer.EmptyLayer {
+			// skip attempting to download empty layers
+			continue
+		}
+
+		id := layer.ID
 		layerConfig, err := LayerCache().Get(id)
 		if err != nil {
 
@@ -155,6 +167,10 @@ func (ldm *LayerDownloader) DownloadLayers(ctx context.Context, ic *ImageC) erro
 				layer.Downloading = true
 				LayerCache().Add(layer)
 
+				// the child-most non-empty layer should be used as the leaf layer, and will
+				// be the parent of the container R/W layer VMDK
+				leafLayer = layer
+
 				continue
 			default:
 				return err
@@ -288,20 +304,27 @@ func (ldm *LayerDownloader) makeDownloadFunc(op trace.Operation, layer *ImageWit
 				}
 			}
 
+			// set this layer's parent to the appropriate (non-empty) VMDK
+			layer.DiskParent = vmdkMap[layer.ID]
+
 			// is this the leaf layer?
-			imageLayer := layer.ID == layers[0].ID
+			isLeaf := layer.ID == leafLayer.ID
 
 			if !ic.Standalone {
 				// if this is the leaf layer, we are done and can now create the image config
-				if imageLayer {
+				if isLeaf {
 					imageConfig, err := ic.CreateImageConfig(derivedOp, layers)
 					if err != nil {
 						d.err = err
 						return
 					}
+
+					// set the image VMDK so container creation uses the correct disk as the R/W's parent
+					imageConfig.VMDK = layer.ID
+
 					// cache and persist the image
 					cache.ImageCache().Add(&imageConfig)
-					if err := cache.ImageCache().Save(); err != nil {
+					if err = cache.ImageCache().Save(); err != nil {
 						d.err = fmt.Errorf("error saving image cache: %s", err)
 						return
 					}
@@ -321,7 +344,7 @@ func (ldm *LayerDownloader) makeDownloadFunc(op trace.Operation, layer *ImageWit
 			defer ldm.m.Unlock()
 
 			// Write blob to the storage layer
-			if err := ic.WriteImageBlob(derivedOp, layer, progressOutput, imageLayer); err != nil {
+			if err := ic.WriteImageBlob(derivedOp, layer, progressOutput, isLeaf); err != nil {
 				d.err = err
 				return
 			}
@@ -392,3 +415,22 @@ func (ldm *LayerDownloader) makeDownloadFuncFromDownload(op trace.Operation, lay
 	}
 
 }
+
+func getVMDKMap(layers []*ImageWithMeta) map[string]string {
+	result := make(map[string]string)
+	diskParent := "scratch"
+
+	for i := len(layers) - 1; i > 0; i-- {
+		layer := layers[i]
+		if layer.EmptyLayer {
+			// skip empty layers
+			continue
+		}
+		// set layer's parent to last known non-empty layer in the chain
+		result[layer.ID] = diskParent
+		// this layer isn't empty, so it will be the next parent layer in the chain
+		diskParent = layer.ID
+	}
+
+	return result
+}

lib/imagec/imagec.go

@@ -126,6 +126,13 @@ type ImageWithMeta struct {
 	Meta   string
 	Size   int64
 
+	// DiskParent is the ID of the most recent non-empty parent layer that will serve
+	// as this layer's parent in the disk chain
+	DiskParent string
+
+	// EmptyLayer is true if this layer contains no filesystem data, false otherwise
+	EmptyLayer bool
+
 	Downloading bool
 }
 
@@ -246,6 +253,12 @@ func (ic *ImageC) LayersToDownload(ctx context.Context) ([]*ImageWithMeta, error
 			return nil, fmt.Errorf("Failed to unmarshall image history: %s", err)
 		}
 
+		log.Infof(">>>>> Layer %s empty? %t - blobsum: %s", v1.ID, layer.BlobSum == DigestSHA256EmptyBlobSum, layer.BlobSum)
+		emptyLayer := false
+		if layer.BlobSum == DigestSHA256EmptyBlobSum {
+			emptyLayer = true
+		}
+
 		// if parent is empty set it to scratch
 		parent := constants.ScratchLayerID
 		if v1.Parent != "" {
@@ -259,8 +272,9 @@ func (ic *ImageC) LayersToDownload(ctx context.Context) ([]*ImageWithMeta, error
 				Parent: parent,
 				Store:  ic.Storename,
 			},
-			Meta:  history.V1Compatibility,
-			Layer: layer,
+			Meta:       history.V1Compatibility,
+			Layer:      layer,
+			EmptyLayer: emptyLayer,
 		}
 
 		// populate manifest layer with existing cached data
@@ -409,7 +423,7 @@ func (ic *ImageC) CreateImageConfig(ctx context.Context, images []*ImageWithMeta
 		}
 
 		// is this an empty layer?
-		if layer.DiffID == dockerLayer.DigestSHA256EmptyTar.String() {
+		if layer.Layer.BlobSum == DigestSHA256EmptyBlobSum {
 			h.EmptyLayer = true
 		} else {
 			// if not empty, add diffID to rootFS

lib/imagec/storage.go

@@ -97,7 +97,7 @@ func WriteImage(op trace.Operation, host string, image *ImageWithMeta, data io.R
 		storage.NewWriteImageParamsWithContext(op).
 			WithOpID(&opID).
 			WithImageID(image.ID).
-			WithParentID(image.Parent).
+			WithParentID(image.DiskParent).
 			WithStoreName(image.Store).
 			WithMetadatakey(key).
 			WithMetadataval(blob).
@@ -111,5 +111,4 @@ func WriteImage(op trace.Operation, host string, image *ImageWithMeta, data io.R
 	log.Printf("Created an image %#v", r.Payload)
 
 	return nil
-
 }