Skip to content

Commit

Permalink
Use QueryVirtualDiskInfo in storage [specific ci=Group1-Docker-Comman…
Browse files Browse the repository at this point in the history 
…ds] (vmware#4376)

* Use QueryVirtualDiskInfo in place of parentMap

Parent relationships are now retrieved from vsphere.  The parentmap is
no longer required.

* Move storage API to using DatastorePath

* Review comments and test update

The tests now compile and even pass!
  • Loading branch information
@fdawg4l
fdawg4l authored Mar 24, 2017
1 parent 717fa2b commit 1c494f3
Show file tree
Hide file tree
Showing 12 changed files with 202 additions and 371 deletions.
4 changes: 4 additions & 0 deletions lib/portlayer/storage/image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/vmware/vic/lib/portlayer/util"
"github.com/vmware/vic/pkg/index"
"github.com/vmware/vic/pkg/trace"
"github.com/vmware/vic/pkg/vsphere/disk"
)

// ImageStorer is an interface to store images in the Image Store
Expand Down Expand Up @@ -95,6 +96,9 @@ type Image struct {

// Metadata associated with the image.
Metadata map[string][]byte

// Disk is the underlying disk implementation
Disk *disk.VirtualDisk
}

func (i *Image) Copy() index.Element {
Expand Down
113 changes: 50 additions & 63 deletions lib/portlayer/storage/vsphere/image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ import (
"net/url"
"os"
"path"
"strings"

"github.com/docker/docker/pkg/archive"

"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/vim25/types"
"github.com/vmware/vic/lib/portlayer/exec"
portlayer "github.com/vmware/vic/lib/portlayer/storage"
Expand All @@ -38,6 +40,9 @@ import (
// All paths on the datastore for images are relative to <datastore>/VIC/
var StorageParentDir = "VIC"

// Set to false for unit tests
var DetachAll = true

const (
StorageImageDir = "images"
defaultDiskLabel = "containerfs"
Expand All @@ -53,18 +58,10 @@ type ImageStore struct {
s *session.Session

ds *datastore.Helper

// Parent relationships
// This will go away when First Class Disk support is added to vsphere.
// Currently, we can't get a disk spec for a disk outside of creating the
// disk (and the spec). This spec has the parent relationship for the
// disk. So, for now, persist this data in the datastore and look it up
// when we need it.
parents *parentM
}

func NewImageStore(op trace.Operation, s *session.Session, u *url.URL) (*ImageStore, error) {
dm, err := disk.NewDiskManager(op, s)
dm, err := disk.NewDiskManager(op, s, DetachAll)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -110,8 +107,11 @@ func (v *ImageStore) imageDiskPath(storeName, imageName string) string {
}

// Returns the path to the vmdk itself in datastore url format
func (v *ImageStore) imageDiskDSPath(storeName, imageName string) string {
return path.Join(v.ds.RootURL.String(), v.imageDiskPath(storeName, imageName))
func (v *ImageStore) imageDiskDSPath(storeName, imageName string) *object.DatastorePath {
return &object.DatastorePath{
Datastore: v.ds.RootURL.Datastore,
Path: path.Join(v.ds.RootURL.Path, v.imageDiskPath(storeName, imageName)),
}
}

// Returns the path to the metadata directory for an image
Expand All @@ -135,13 +135,6 @@ func (v *ImageStore) CreateImageStore(op trace.Operation, storeName string) (*ur
return nil, err
}

if v.parents == nil {
pm, err := restoreParentMap(op, v.ds, storeName)
if err != nil {
return nil, err
}
v.parents = pm
}
return u, nil
}

Expand Down Expand Up @@ -170,18 +163,10 @@ func (v *ImageStore) GetImageStore(op trace.Operation, storeName string) (*url.U
return nil, fmt.Errorf("Stat error: path doesn't exist (%s)", p)
}

if v.parents == nil {
// This is startup. Look for image directories without manifest files and
// nuke them.
if err := v.cleanup(op, u); err != nil {
return nil, err
}

pm, err := restoreParentMap(op, v.ds, storeName)
if err != nil {
return nil, err
}
v.parents = pm
// This is startup. Look for image directories without manifest files and
// nuke them.
if err := v.cleanup(op, u); err != nil {
return nil, err
}

return u, nil
Expand Down Expand Up @@ -230,6 +215,7 @@ func (v *ImageStore) WriteImage(op trace.Operation, parent *portlayer.Image, ID
return nil, err
}

var dsk *disk.VirtualDisk
// If this is scratch, then it's the root of the image store. All images
// will be descended from this created and prepared fs.
if ID == portlayer.Scratch.ID {
Expand All @@ -243,17 +229,10 @@ func (v *ImageStore) WriteImage(op trace.Operation, parent *portlayer.Image, ID
return nil, fmt.Errorf("parent ID is empty")
}

// persist the relationship
v.parents.Add(ID, parent.ID)

if err := v.parents.Save(op); err != nil {
return nil, err
}

if err := v.writeImage(op, storeName, parent.ID, ID, meta, sum, r); err != nil {
dsk, err = v.writeImage(op, storeName, parent.ID, ID, meta, sum, r)
if err != nil {
return nil, err
}

}

newImage := &portlayer.Image{
Expand All @@ -262,6 +241,7 @@ func (v *ImageStore) WriteImage(op trace.Operation, parent *portlayer.Image, ID
ParentLink: parent.SelfLink,
Store: parent.Store,
Metadata: meta,
Disk: dsk,
}

return newImage, nil
Expand Down Expand Up @@ -292,20 +272,20 @@ func (v *ImageStore) cleanupDisk(op trace.Operation, ID, storeName string, vmdis
// If everything matches, move the tmp vmdk to ID.vmdk. The unwind path is a
// bit convoluted here; we need to clean up on the way out in the error case
func (v *ImageStore) writeImage(op trace.Operation, storeName, parentID, ID string, meta map[string][]byte,
sum string, r io.Reader) error {
sum string, r io.Reader) (*disk.VirtualDisk, error) {

// Create a temp image directory in the store.
imageDir := v.imageDirPath(storeName, ID)
_, err := v.ds.Mkdir(op, true, imageDir)
if err != nil {
return err
return nil, err
}

// Write the metadata to the datastore
metaDataDir := v.imageMetadataDirPath(storeName, ID)
err = writeMetadata(op, v.ds, metaDataDir, meta)
if err != nil {
return err
return nil, err
}

// datastore path to the parent
Expand All @@ -327,17 +307,17 @@ func (v *ImageStore) writeImage(op trace.Operation, storeName, parentID, ID stri
// Create the disk
vmdisk, err = v.dm.CreateAndAttach(op, diskDsURI, parentDiskDsURI, 0, os.O_RDWR)
if err != nil {
return err
return nil, err
}
// tmp dir to mount the disk
dir, err := ioutil.TempDir("", "mnt-"+ID)
if err != nil {
return err
return nil, err
}
defer os.RemoveAll(dir)

if err := vmdisk.Mount(dir, nil); err != nil {
return err
return nil, err
}

h := sha256.New()
Expand All @@ -346,23 +326,23 @@ func (v *ImageStore) writeImage(op trace.Operation, storeName, parentID, ID stri
// Untar the archive
var n int64
if n, err = archive.ApplyLayer(dir, t); err != nil {
return err
return nil, err
}

op.Debugf("%s wrote %d bytes", ID, n)

actualSum := fmt.Sprintf("sha256:%x", h.Sum(nil))
if actualSum != sum {
err = fmt.Errorf("Failed to validate image checksum. Expected %s, got %s", sum, actualSum)
return err
return nil, err
}

if err = vmdisk.Unmount(); err != nil {
return err
return nil, err
}

if err = v.dm.Detach(op, vmdisk); err != nil {
return err
return nil, err
}

// Write our own bookkeeping manifest file to the image's directory. We
Expand All @@ -376,10 +356,10 @@ func (v *ImageStore) writeImage(op trace.Operation, storeName, parentID, ID stri
// with atomic operations. Touching an empty file seems to work well
// enough.
if err = v.writeManifest(op, storeName, ID, nil); err != nil {
return err
return nil, err
}

return nil
return vmdisk, nil
}

func (v *ImageStore) scratch(op trace.Operation, storeName string) error {
Expand Down Expand Up @@ -417,7 +397,7 @@ func (v *ImageStore) scratch(op trace.Operation, storeName string) error {
}()

// Create the disk
vmdisk, err = v.dm.CreateAndAttach(op, imageDiskDsURI, "", size, os.O_RDWR)
vmdisk, err = v.dm.CreateAndAttach(op, imageDiskDsURI, nil, size, os.O_RDWR)
if err != nil {
op.Errorf("CreateAndAttach(%s) error: %s", imageDiskDsURI, err)
return err
Expand All @@ -441,7 +421,6 @@ func (v *ImageStore) scratch(op trace.Operation, storeName string) error {
}

func (v *ImageStore) GetImage(op trace.Operation, store *url.URL, ID string) (*portlayer.Image, error) {

defer trace.End(trace.Begin(store.String()))
storeName, err := util.ImageStoreName(store)
if err != nil {
Expand All @@ -464,26 +443,34 @@ func (v *ImageStore) GetImage(op trace.Operation, store *url.URL, ID string) (*p
return nil, err
}

diskDsURI := v.imageDiskDSPath(storeName, ID)

var s = *store
var parentURL *url.URL

parentID := v.parents.Get(ID)
if parentID != "" {
parentURL, _ = util.ImageURL(storeName, parentID)
dsk, err := v.dm.Get(op, diskDsURI)
if err != nil {
return nil, err
}

var parentURL *url.URL
if dsk.ParentDatastoreURI != nil {
vmdk := path.Base(dsk.ParentDatastoreURI.Path)
parentURL, err = util.ImageURL(storeName, strings.TrimSuffix(vmdk, path.Ext(vmdk)))
if err != nil {
return nil, err
}
}

newImage := &portlayer.Image{
ID: ID,
SelfLink: imageURL,
// We're relying on the parent map for this since we don't currently have a
// way to get the disk's spec. See VIC #482 for details. Parent:
// parent.SelfLink,
ID: ID,
SelfLink: imageURL,
Store: &s,
ParentLink: parentURL,
Metadata: meta,
Disk: dsk,
}

op.Debugf("Returning image from location %s with parent url %s", newImage.SelfLink, newImage.Parent())
op.Debugf("GetImage(%s) has parent %s", newImage.SelfLink, newImage.Parent())
return newImage, nil
}

Expand Down
11 changes: 9 additions & 2 deletions lib/portlayer/storage/vsphere/image_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ import (

func setup(t *testing.T) (*portlayer.NameLookupCache, *session.Session, string, error) {
logrus.SetLevel(logrus.DebugLevel)
trace.Logger.Level = logrus.DebugLevel
DetachAll = false

client := datastore.Session(context.TODO(), t)
if client == nil {
Expand Down Expand Up @@ -274,7 +276,10 @@ func TestCreateImageLayers(t *testing.T) {
return
}
for _, img := range listedImages {
if !assert.Equal(t, expectedImages[img.ID], img) {
if !assert.Equal(t, expectedImages[img.ID].Store.String(), img.Store.String()) {
return
}
if !assert.Equal(t, expectedImages[img.ID].SelfLink.String(), img.SelfLink.String()) {
return
}
}
Expand Down Expand Up @@ -568,7 +573,9 @@ func tarFiles(files []tarFile) (*bytes.Buffer, error) {
}

func mountLayerRO(v *ImageStore, parent *portlayer.Image) (*disk.VirtualDisk, error) {
roName := v.imageDiskDSPath("testStore", parent.ID) + "-ro.vmdk"
roName := v.imageDiskDSPath("testStore", parent.ID)
roName.Path = roName.Path + "-ro.vmdk"

parentDsURI := v.imageDiskDSPath("testStore", parent.ID)

op := trace.NewOperation(context.TODO(), "ro")
Expand Down
Loading

0 comments on commit 1c494f3

Please sign in to comment.