Portlayer images use standard Join pattern (vmware#7397)

This brings consistent organisation and naming of files to the storage portlayer and moves Image to use the standard Join/Bind semantic. Previously container images were injected into the cVM spec inline in the exec component of the portlayer. This was tech debt from the very early days of the project before the Join/Bind semantic was fully established. This also moves and renames files to make it easier to locate specific portions of code, both within a single implementation and across implementations (implementations are currently nfs and vsphere). This adds package documentation to provide developers with some introduction and guidance at a high level. It does not detail many of the lower level packages. There are some additional structure naming changes that could be made for consistency, and some leaking abstractions (e.g. exposing volume type to handlers) that should eventually be addressed. This is merge without those as it is a significant improvement over current state of affairs and may simplify some outstanding work.
hashicorp · Dec 11, 2018 · 16ce0ef · 16ce0ef
1 parent d7681fa
commit 16ce0ef
Show file tree

Hide file tree

Showing 63 changed files with 1,828 additions and 1,252 deletions.
diff --git a/infra/scripts/bash-helpers.sh b/infra/scripts/bash-helpers.sh
@@ -136,7 +136,11 @@ vic-inspect () {
 vic-upgrade () {
     vicProfileTranscode
 
-    "$(vic-path)/bin/${VIC_VERSION}/vic-machine-$OS" upgrade --target="$TARGET_URL" --compute-resource="$COMPUTE" --name="${VIC_NAME:-${USER}test}" --thumbprint="$THUMBPRINT" "$@"
+    # change to the bin directory as that's where our certs would have been generated by vic-create
+    (
+        cd "$(vic-path)"/bin || return
+        "$(vic-path)/bin/${VIC_VERSION}/vic-machine-$OS" upgrade --target="$TARGET_URL" --compute-resource="$COMPUTE" --name="${VIC_NAME:-${USER}test}" --thumbprint="$THUMBPRINT" "$@"
+    )
 }
 
 vic-ls () {

diff --git a/lib/apiservers/engine/backends/container.go b/lib/apiservers/engine/backends/container.go
@@ -752,6 +752,11 @@ func (c *ContainerBackend) containerCreate(op trace.Operation, vc *viccontainer.
 		return "", err
 	}
 
+	h, err = c.storageProxy.AddImageToContainer(op, h, id, vc.LayerID, vc.ImageID, config)
+	if err != nil {
+		return "", err
+	}
+
 	h, err = c.containerProxy.CreateContainerTask(op, h, id, config)
 	if err != nil {
 		return "", err

diff --git a/lib/apiservers/engine/backends/container_test.go b/lib/apiservers/engine/backends/container_test.go
@@ -1,4 +1,4 @@
-// Copyright 2016-2017 VMware, Inc. All Rights Reserved.
+// Copyright 2016-2018 VMware, Inc. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -59,6 +59,15 @@ type CreateHandleMockData struct {
 	createErrSubstr string
 }
 
+type AddImageMockData struct {
+	retHandle       string
+	retErr          error
+	deltaID         string
+	layerID         string
+	imageID         string
+	createErrSubstr string
+}
+
 type AddToScopeMockData struct {
 	createInputID   string
 	retHandle       string
@@ -99,6 +108,7 @@ type LogMockData struct {
 type MockContainerProxy struct {
 	mockRespIndices        []int
 	mockCreateHandleData   []CreateHandleMockData
+	mockAddImageData       []AddImageMockData
 	mockAddToScopeData     []AddToScopeMockData
 	mockAddVolumesData     []AddVolumesMockData
 	mockAddInteractionData []AddInteractionMockData
@@ -135,8 +145,9 @@ var dummyContainers = []string{dummyContainerID, dummyContainerIDTTY}
 
 func NewMockContainerProxy() *MockContainerProxy {
 	return &MockContainerProxy{
-		mockRespIndices:        make([]int, 6),
+		mockRespIndices:        make([]int, 7),
 		mockCreateHandleData:   MockCreateHandleData(),
+		mockAddImageData:       MockAddImageData(),
 		mockAddToScopeData:     MockAddToScopeData(),
 		mockAddVolumesData:     MockAddVolumesData(),
 		mockAddInteractionData: MockAddInteractionData(),
@@ -166,6 +177,10 @@ func MockCreateHandleData() []CreateHandleMockData {
 	return mockCreateHandleData
 }
 
+func MockAddImageData() []AddImageMockData {
+	return nil
+}
+
 func MockAddToScopeData() []AddToScopeMockData {
 	addToScopeNotFound := plscopes.AddContainerNotFound{
 		Payload: &plmodels.Error{
@@ -461,6 +476,10 @@ func (s *MockStorageProxy) Remove(ctx context.Context, name string) error {
 	return nil
 }
 
+func (s *MockStorageProxy) AddImageToContainer(ctx context.Context, handle, id, layerID, imageID string, config types.ContainerCreateConfig) (string, error) {
+	return "", nil
+}
+
 func (s *MockStorageProxy) AddVolumesToContainer(ctx context.Context, handle string, config types.ContainerCreateConfig) (string, error) {
 	return "", nil
 }
@@ -504,10 +523,12 @@ func (sp *MockStreamProxy) StreamContainerStats(ctx context.Context, config *con
 // cache
 func TestContainerCreateEmptyImageCache(t *testing.T) {
 	mockContainerProxy := NewMockContainerProxy()
+	mockStorageProxy := NewMockStorageProxy()
 
 	// Create our personality Container backend
 	cb := &ContainerBackend{
 		containerProxy: mockContainerProxy,
+		storageProxy:   mockStorageProxy,
 	}
 
 	// mock a container create config
@@ -528,10 +549,12 @@ func TestContainerCreateEmptyImageCache(t *testing.T) {
 // then vicbackends.ContainerCreate() should return errors from that.
 func TestCreateHandle(t *testing.T) {
 	mockContainerProxy := NewMockContainerProxy()
+	mockStorageProxy := NewMockStorageProxy()
 
 	// Create our personality Container backend
 	cb := &ContainerBackend{
 		containerProxy: mockContainerProxy,
+		storageProxy:   mockStorageProxy,
 	}
 
 	AddMockImageToCache()
@@ -573,10 +596,12 @@ func TestCreateHandle(t *testing.T) {
 // possible input/outputs for adding container to scope and calls vicbackends.ContainerCreate()
 func TestContainerAddToScope(t *testing.T) {
 	mockContainerProxy := NewMockContainerProxy()
+	mockStorageProxy := NewMockStorageProxy()
 
 	// Create our personality Container backend
 	cb := &ContainerBackend{
 		containerProxy: mockContainerProxy,
+		storageProxy:   mockStorageProxy,
 	}
 
 	AddMockImageToCache()

diff --git a/lib/apiservers/engine/backends/eventmonitor.go b/lib/apiservers/engine/backends/eventmonitor.go
@@ -129,7 +129,7 @@ func (m *PortlayerEventMonitor) Start() error {
 			select {
 			case <-m.stop:
 				log.Infof("Portlayer Event Monitor stopped normally")
-				break
+				return
 			default:
 				if err = m.monitor(); err != nil {
 					log.Errorf("Restarting Portlayer event monitor due to error: %s", err)

diff --git a/lib/apiservers/engine/proxy/container_proxy.go b/lib/apiservers/engine/proxy/container_proxy.go
@@ -169,14 +169,6 @@ func (c *ContainerProxy) CreateContainerHandle(ctx context.Context, vc *vicconta
 		return "", "", errors.NillPortlayerClientError("ContainerProxy")
 	}
 
-	if vc.ImageID == "" {
-		return "", "", errors.NotFoundError("No image specified")
-	}
-
-	if vc.LayerID == "" {
-		return "", "", errors.NotFoundError("No layer specified")
-	}
-
 	// Call the Exec port layer to create the container
 	host, err := sys.UUID()
 	if err != nil {
@@ -186,12 +178,6 @@ func (c *ContainerProxy) CreateContainerHandle(ctx context.Context, vc *vicconta
 	plCreateParams := dockerContainerCreateParamsToPortlayer(ctx, config, vc, host).WithOpID(&opID)
 	createResults, err := c.client.Containers.Create(plCreateParams)
 	if err != nil {
-		if _, ok := err.(*containers.CreateNotFound); ok {
-			cerr := fmt.Errorf("No such image: %s", vc.ImageID)
-			log.Errorf("%s (%s)", cerr, err)
-			return "", "", errors.NotFoundError(cerr.Error())
-		}
-
 		// If we get here, most likely something went wrong with the port layer API server
 		return "", "", errors.InternalServerError(err.Error())
 	}
@@ -1083,21 +1069,9 @@ func dockerContainerCreateParamsToPortlayer(ctx context.Context, cc types.Contai
 	config.NumCpus = cc.HostConfig.CPUCount
 	config.MemoryMB = cc.HostConfig.Memory
 
-	// Layer/vmdk to use
-	config.Layer = vc.LayerID
-
-	// Image ID
-	config.Image = vc.ImageID
-
-	// Repo Requested
-	config.RepoName = cc.Config.Image
-
 	//copy friendly name
 	config.Name = cc.Name
 
-	// image store
-	config.ImageStore = &models.ImageStore{Name: imageStore}
-
 	// network
 	config.NetworkDisabled = cc.Config.NetworkDisabled
 

diff --git a/lib/apiservers/engine/proxy/storage_proxy.go b/lib/apiservers/engine/proxy/storage_proxy.go
@@ -37,6 +37,7 @@ import (
 	"github.com/vmware/vic/lib/apiservers/portlayer/models"
 	"github.com/vmware/vic/lib/constants"
 	"github.com/vmware/vic/pkg/trace"
+	"github.com/vmware/vic/pkg/vsphere/sys"
 )
 
 type VicStorageProxy interface {
@@ -45,6 +46,7 @@ type VicStorageProxy interface {
 	VolumeInfo(ctx context.Context, name string) (*models.VolumeResponse, error)
 	Remove(ctx context.Context, name string) error
 
+	AddImageToContainer(ctx context.Context, handle, deltaID, layerID, imageID string, config types.ContainerCreateConfig) (string, error)
 	AddVolumesToContainer(ctx context.Context, handle string, config types.ContainerCreateConfig) (string, error)
 }
 
@@ -239,6 +241,60 @@ func (s *StorageProxy) Remove(ctx context.Context, name string) error {
 	return nil
 }
 
+// AddImageToContainer adds the specified image to a container, referenced by handle.
+// If an error is returned, the returned handle should not be used.
+// - deltaID is the ID to use for the read/write layer - it's expected that this does not exist
+// - layerID is the parent layer for the delta layer and is expected to exist
+// - 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)
+	defer trace.End(trace.Begin(deltaID, op))
+	opID := op.ID()
+
+	if s.client == nil {
+		return "", errors.InternalServerError("ContainerProxy.AddImageToContainer failed to get the portlayer client")
+	}
+
+	if imageID == "" {
+		return "", errors.NotFoundError("No image specified")
+	}
+
+	if layerID == "" {
+		return "", errors.NotFoundError("No layer specified")
+	}
+
+	host, err := sys.UUID()
+	if err != nil {
+		return "", errors.InternalServerError("ContainerProxy.AddImageToContainer got unexpected error getting VCH UUID")
+	}
+
+	response, err := s.client.Storage.ImageJoin(storage.NewImageJoinParamsWithContext(op).WithOpID(&opID).WithStoreName(host).WithID(layerID).
+		WithConfig(&models.ImageJoinConfig{
+			Handle:   handle,
+			DeltaID:  deltaID,
+			ImageID:  imageID,
+			RepoName: config.Config.Image,
+		}))
+	if err != nil {
+		if _, ok := err.(*storage.ImageJoinNotFound); ok {
+			cerr := fmt.Errorf("No such image: %s", imageID)
+			op.Errorf("%s: %s", cerr, err)
+			return "", errors.NotFoundError(cerr.Error())
+		}
+
+		return "", errors.InternalServerError(err.Error())
+	}
+	handle, ok := response.Payload.Handle.(string)
+	if !ok {
+		return "", errors.InternalServerError(fmt.Sprintf("Type assertion failed for %#+v", handle))
+	}
+
+	return handle, nil
+}
+
 // AddVolumesToContainer adds volumes to a container, referenced by handle.
 // If an error is returned, the returned handle should not be used.
 //

diff --git a/lib/apiservers/portlayer/restapi/handlers/containers_handlers.go b/lib/apiservers/portlayer/restapi/handlers/containers_handlers.go
@@ -80,8 +80,6 @@ func (handler *ContainersHandlersImpl) CreateHandler(params containers.CreatePar
 	op := trace.NewOperationFromID(context.Background(), params.OpID, "containers.CreateHandler(%s)", params.CreateConfig.Name)
 	defer trace.End(trace.Begin("CreateHandler", op))
 
-	var err error
-
 	session := handler.handlerCtx.Session
 	id := uid.New().String()
 
@@ -106,9 +104,6 @@ func (handler *ContainersHandlersImpl) CreateHandler(params containers.CreatePar
 		CreateTime: time.Now().UTC().UnixNano(),
 		Version:    version.GetBuild(),
 		Key:        pem.EncodeToMemory(&privateKeyBlock),
-		LayerID:    params.CreateConfig.Layer,
-		ImageID:    params.CreateConfig.Image,
-		RepoName:   params.CreateConfig.RepoName,
 		Hostname:   params.CreateConfig.Hostname,
 		Domainname: params.CreateConfig.Domainname,
 	}
@@ -122,9 +117,7 @@ func (handler *ContainersHandlersImpl) CreateHandler(params containers.CreatePar
 
 	// Create the executor.ExecutorCreateConfig
 	c := &exec.ContainerCreateConfig{
-		Metadata:       m,
-		ParentImageID:  params.CreateConfig.Layer,
-		ImageStoreName: params.CreateConfig.ImageStore.Name,
+		Metadata: m,
 		Resources: exec.Resources{
 			NumCPUs:  params.CreateConfig.NumCpus,
 			MemoryMB: params.CreateConfig.MemoryMB,
@@ -133,7 +126,7 @@ func (handler *ContainersHandlersImpl) CreateHandler(params containers.CreatePar
 
 	h, err := exec.Create(op, session, c)
 	if err != nil {
-		log.Errorf("ContainerCreate error: %s", err.Error())
+		op.Errorf("ContainerCreate error: %s", err.Error())
 		return containers.NewCreateNotFound().WithPayload(&models.Error{Message: err.Error()})
 	}