New compute methods for vSphere, storage operations by instance ID, misc vSphere fixes

- Fix errors messages for vSphere when disk is not attached
- Add InspectInstance implementation for vSphere
- Add new methods, ListInstances, CreateInstance, AttachByInstanceID
- DeviceMappings can now be done by instance ID
- Remove timeout from DeleteInstance. Callers should use exponentional backoff instead
- Add attach options to Attach method

Signed-off-by: Harsh Desai <harsh@portworx.com>

Author: Harsh Desai

Gopkg.lock

@@ -63,7 +63,7 @@
 
 [[constraint]]
   name = "github.com/vmware/govmomi"
-  version = "v0.15.0"
+  version = "v0.18.0"
 
 [[constraint]]
   branch = "master"

Gopkg.toml

@@ -35,6 +35,7 @@ const (
 )
 
 type awsOps struct {
+	cloudops.Storage
 	cloudops.Compute
 	instanceType string
 	instance     string
@@ -337,12 +338,21 @@ func (s *awsOps) matchTag(tag *ec2.Tag, match string) bool {
 		*tag.Key == match
 }
 
-func (s *awsOps) DeviceMappings() (map[string]string, error) {
-	instance, err := s.describe()
+func (s *awsOps) DeviceMappings(instanceID string) (map[string]string, error) {
+	var (
+		instance *ec2.Instance
+		err      error
+	)
+
+	if len(instanceID) == 0 {
+		instance, err = s.describe()
+	} else {
+		instance, err = DescribeInstanceByID(s.ec2, instanceID)
+	}
+
 	if err != nil {
 		return nil, err
 	}
-
 	m := make(map[string]string)
 	for _, d := range instance.BlockDeviceMappings {
 		if d.DeviceName != nil && d.Ebs != nil && d.Ebs.VolumeId != nil {
@@ -769,7 +779,7 @@ func (s *awsOps) Delete(id string) error {
 	return err
 }
 
-func (s *awsOps) Attach(volumeID string) (string, error) {
+func (s *awsOps) Attach(volumeID string, opts map[string]string) (string, error) {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 
@@ -817,6 +827,12 @@ func (s *awsOps) Attach(volumeID string) (string, error) {
 
 	return "", fmt.Errorf("failed to attach any of the free devices. Attempted: %v", devices)
 }
+func (s *awsOps) AttachByInstanceID(
+	instanceID, volumeID string, options map[string]string) (string, error) {
+	return "", &cloudops.ErrNotSupported{
+		Operation: "AttachByInstanceID",
+	}
+}
 
 func (s *awsOps) Detach(volumeID string) error {
 	return s.detachInternal(volumeID, s.instance)

aws/aws.go

@@ -40,7 +40,7 @@ func TestAll(t *testing.T) {
 		t.Skipf("skipping AWS tests as environment is not set...\n")
 	}
 
-	test.RunTest(drivers, diskTemplates, t)
+	test.RunTest(drivers, diskTemplates, nil, t)
 }
 
 func TestAwsGetPrefixFromRootDeviceName(t *testing.T) {

aws/aws_test.go

@@ -192,7 +192,7 @@ func (a *azureOps) GetDeviceID(disk interface{}) (string, error) {
 	)
 }
 
-func (a *azureOps) Attach(diskName string) (string, error) {
+func (a *azureOps) Attach(diskName string, opts map[string]string) (string, error) {
 	disk, err := a.checkDiskAttachmentStatus(diskName)
 	if err == nil {
 		// Disk is already attached locally, return device path
@@ -232,6 +232,13 @@ func (a *azureOps) Attach(diskName string) (string, error) {
 	return a.waitForAttach(diskName)
 }
 
+func (s *azureOps) AttachByInstanceID(
+	instanceID, volumeID string, options map[string]string) (string, error) {
+	return "", &cloudops.ErrNotSupported{
+		Operation: "AttachByInstanceID",
+	}
+}
+
 func (a *azureOps) handleAttachError(err error) error {
 	if de, ok := err.(autorest.DetailedError); ok {
 		if re, ok := de.Original.(azure.RequestError); ok &&
@@ -370,7 +377,13 @@ func (a *azureOps) Inspect(diskNames []*string) ([]interface{}, error) {
 	return disks, nil
 }
 
-func (a *azureOps) DeviceMappings() (map[string]string, error) {
+func (a *azureOps) DeviceMappings(instanceID string) (map[string]string, error) {
+	if len(instanceID) > 0 {
+		return nil, &cloudops.ErrNotSupported{
+			Operation: "DeviceMappings",
+			Reason:    "API currently does not support providing instanceID",
+		}
+	}
 	dataDisks, err := a.vmsClient.getDataDisks(a.instance)
 	if err != nil {
 		return nil, err

azure/azure.go

@@ -59,5 +59,5 @@ func TestAll(t *testing.T) {
 	d, disks := initAzure(t)
 	drivers[d.Name()] = d
 	diskTemplates[d.Name()] = disks
-	test.RunTest(drivers, diskTemplates, t)
+	test.RunTest(drivers, diskTemplates, nil, t)
 }

azure/azure_test.go

@@ -72,6 +72,56 @@ func (e *exponentialBackoff) InspectInstance(instanceID string) (*cloudops.Insta
 
 }
 
+func (e *exponentialBackoff) CreateInstance(template interface{}) (*cloudops.InstanceInfo, error) {
+	var (
+		instanceInfo *cloudops.InstanceInfo
+		origErr      error
+	)
+	conditionFn := func() (bool, error) {
+		instanceInfo, origErr = e.cloudOps.CreateInstance(template)
+		msg := fmt.Sprintf("Failed to create instance: %v.", template)
+		return e.handleError(origErr, msg)
+	}
+	expErr := wait.ExponentialBackoff(e.backoff, conditionFn)
+	if expErr == wait.ErrWaitTimeout {
+		return nil, cloudops.NewStorageError(cloudops.ErrExponentialTimeout, origErr.Error(), "")
+	}
+	return instanceInfo, origErr
+}
+
+func (e *exponentialBackoff) ListInstances(opts *cloudops.ListInstancesOpts) (
+	[]*cloudops.InstanceInfo, error) {
+
+	var (
+		instanceInfos []*cloudops.InstanceInfo
+		origErr       error
+	)
+	conditionFn := func() (bool, error) {
+		instanceInfos, origErr = e.cloudOps.ListInstances(opts)
+		msg := fmt.Sprintf("Failed to list instances with options: %v.", opts)
+		return e.handleError(origErr, msg)
+	}
+	expErr := wait.ExponentialBackoff(e.backoff, conditionFn)
+	if expErr == wait.ErrWaitTimeout {
+		return nil, cloudops.NewStorageError(cloudops.ErrExponentialTimeout, origErr.Error(), "")
+	}
+	return instanceInfos, origErr
+}
+
+func (e *exponentialBackoff) DeleteInstance(instanceID string, zone string) error {
+	var origErr error
+	conditionFn := func() (bool, error) {
+		origErr = e.cloudOps.DeleteInstance(instanceID, zone)
+		msg := fmt.Sprintf("Failed to inspect instance: %v.", instanceID)
+		return e.handleError(origErr, msg)
+	}
+	expErr := wait.ExponentialBackoff(e.backoff, conditionFn)
+	if expErr == wait.ErrWaitTimeout {
+		return cloudops.NewStorageError(cloudops.ErrExponentialTimeout, origErr.Error(), "")
+	}
+	return origErr
+}
+
 func (e *exponentialBackoff) InspectInstanceGroupForInstance(instanceID string) (*cloudops.InstanceGroupInfo, error) {
 	var (
 		instanceGroupInfo *cloudops.InstanceGroupInfo
@@ -140,23 +190,6 @@ func (e *exponentialBackoff) GetClusterSizeForInstance(instanceID string) (int64
 
 }
 
-func (e *exponentialBackoff) DeleteInstance(instanceID string, zone string, timeout time.Duration) error {
-	var (
-		origErr error
-	)
-	conditionFn := func() (bool, error) {
-		origErr = e.cloudOps.DeleteInstance(instanceID, zone, timeout)
-		msg := fmt.Sprintf("Failed to delete instance: %v.", instanceID)
-		return e.handleError(origErr, msg)
-	}
-	expErr := wait.ExponentialBackoff(e.backoff, conditionFn)
-	if expErr == wait.ErrWaitTimeout {
-		return cloudops.NewStorageError(cloudops.ErrExponentialTimeout, origErr.Error(), "")
-	}
-	return origErr
-
-}
-
 // Create volume based on input template volume and also apply given labels.
 func (e *exponentialBackoff) Create(template interface{}, labels map[string]string) (interface{}, error) {
 	var (
@@ -183,13 +216,13 @@ func (e *exponentialBackoff) GetDeviceID(template interface{}) (string, error) {
 
 // Attach volumeID.
 // Return attach path.
-func (e *exponentialBackoff) Attach(volumeID string) (string, error) {
+func (e *exponentialBackoff) Attach(volumeID string, options map[string]string) (string, error) {
 	var (
 		devPath string
 		origErr error
 	)
 	conditionFn := func() (bool, error) {
-		devPath, origErr = e.cloudOps.Attach(volumeID)
+		devPath, origErr = e.cloudOps.Attach(volumeID, options)
 		msg := fmt.Sprintf("Failed to attach drive (%v).", volumeID)
 		return e.handleError(origErr, msg)
 	}
@@ -200,11 +233,28 @@ func (e *exponentialBackoff) Attach(volumeID string) (string, error) {
 	return devPath, origErr
 }
 
-// Detach volumeID.
-func (e *exponentialBackoff) Detach(volumeID string) error {
+func (e *exponentialBackoff) AttachByInstanceID(
+	instanceID, volumeID string,
+	options map[string]string) (string, error) {
 	var (
+		devPath string
 		origErr error
 	)
+	conditionFn := func() (bool, error) {
+		devPath, origErr = e.cloudOps.AttachByInstanceID(instanceID, volumeID, options)
+		msg := fmt.Sprintf("Failed to attach drive (%v) on %s", volumeID, instanceID)
+		return e.handleError(origErr, msg)
+	}
+	expErr := wait.ExponentialBackoff(e.backoff, conditionFn)
+	if expErr == wait.ErrWaitTimeout {
+		return "", cloudops.NewStorageError(cloudops.ErrExponentialTimeout, origErr.Error(), "")
+	}
+	return devPath, origErr
+}
+
+// Detach volumeID.
+func (e *exponentialBackoff) Detach(volumeID string) error {
+	var origErr error
 	conditionFn := func() (bool, error) {
 		origErr = e.cloudOps.Detach(volumeID)
 		msg := fmt.Sprintf("Failed to detach drive (%v).", volumeID)
@@ -312,13 +362,13 @@ func (e *exponentialBackoff) Inspect(volumeIds []*string) ([]interface{}, error)
 }
 
 // DeviceMappings returns map[local_attached_volume_path]->volume ID/NAME
-func (e *exponentialBackoff) DeviceMappings() (map[string]string, error) {
+func (e *exponentialBackoff) DeviceMappings(instanceID string) (map[string]string, error) {
 	var (
 		mappings map[string]string
 		origErr  error
 	)
 	conditionFn := func() (bool, error) {
-		mappings, origErr = e.cloudOps.DeviceMappings()
+		mappings, origErr = e.cloudOps.DeviceMappings(instanceID)
 		msg := fmt.Sprintf("Failed to get device mappings.")
 		return e.handleError(origErr, msg)
 	}

backoff/exponential.go

@@ -61,10 +61,15 @@ type InstanceInfo struct {
 
 // Compute interface to manage compute instances.
 type Compute interface {
-	// DeleteInstance deletes the instance
-	DeleteInstance(instanceID string, zone string, timeout time.Duration) error
 	// InstanceID of instance where command is executed.
 	InstanceID() string
+	// CreateInstance creates a cloud instance with the given template
+	CreateInstance(template interface{}) (*InstanceInfo, error)
+	// DeleteInstance deletes a cloud instance with given ID/name and waits
+	// until given timeout to check if the instance got deleted
+	DeleteInstance(instanceID string, zone string) error
+	// ListInstances lists instances in the cloud provider with given options
+	ListInstances(opts *ListInstancesOpts) ([]*InstanceInfo, error)
 	// InspectInstance inspects the node with the given instance ID
 	// TODO: Add support for taking zone as input to inspect instance in any zone
 	InspectInstance(instanceID string) (*InstanceInfo, error)
@@ -90,8 +95,13 @@ type Storage interface {
 	// GetDeviceID returns ID/Name of the given device/disk or snapshot
 	GetDeviceID(template interface{}) (string, error)
 	// Attach volumeID.
+	// options are passthough options given to the cloud provider
 	// Return attach path.
-	Attach(volumeID string) (string, error)
+	Attach(volumeID string, options map[string]string) (string, error)
+	// AttachByInstanceID attaches diskPath to instance with given ID.
+	// options are passthough options given to the cloud provider
+	// Return attach path.
+	AttachByInstanceID(instanceID, volumeID string, options map[string]string) (string, error)
 	// Detach volumeID.
 	Detach(volumeID string) error
 	// DetachFrom detaches the disk/volume with given ID from the given instance ID
@@ -109,7 +119,10 @@ type Storage interface {
 	// Inspect volumes specified by volumeID
 	Inspect(volumeIds []*string) ([]interface{}, error)
 	// DeviceMappings returns map[local_attached_volume_path]->volume ID/NAME
-	DeviceMappings() (map[string]string, error)
+	// instanceID is the ID of the instance where you want to check the device mappings
+	// If not provided, it returns the device mappings of the instance where it's
+	// invoked
+	DeviceMappings(instanceID string) (map[string]string, error)
 	// Enumerate volumes that match given filters. Organize them into
 	// sets identified by setIdentifier.
 	// labels can be nil, setIdentifier can be empty string.
@@ -140,3 +153,14 @@ type Ops interface {
 	// Compute operations in the cloud
 	Compute
 }
+
+// ListInstancesOpts are options for the list instances call
+type ListInstancesOpts struct {
+	// LabelSelector to filter the instances that are listed. The labels are
+	// interpreted by the cloud provider
+	LabelSelector map[string]string
+	// NamePrefix if provided will be used to return all instances whose name
+	// starts with the given prefix. This should be used in cloud providers that
+	// don't support labels in instances
+	NamePrefix string
+}

cloudops.go

@@ -20,6 +20,23 @@ const (
 	ErrExponentialTimeout
 )
 
+// ErrInvalidArgument is the error type to use when invalid args as given to APIs
+type ErrInvalidArgument struct {
+	// Operation is the operation that was being performed
+	Operation string
+	// Reason is an optional reason explaining the invalid argument error
+	Reason string
+}
+
+func (e *ErrInvalidArgument) Error() string {
+	errString := fmt.Sprintf("Operation: %s was given invalid argument", e.Operation)
+	if len(e.Reason) > 0 {
+		errString = fmt.Sprintf("%s. Reason: %s", errString, e.Reason)
+	}
+
+	return errString
+}
+
 // ErrNotSupported is the error type for unsupported operations
 type ErrNotSupported struct {
 	// Operation is the operation not being supported