Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@

[[constraint]]
name = "github.com/vmware/govmomi"
version = "v0.15.0"
version = "v0.18.0"

[[constraint]]
branch = "master"
Expand Down
24 changes: 20 additions & 4 deletions aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const (
)

type awsOps struct {
cloudops.Storage
cloudops.Compute
instanceType string
instance string
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should define a constant Local and error out if instanceID is empty.
The API is confusing when there is added intelligence if a param is not given.
You can also just add a new API like the other ones - DeviceMappingsForInstance

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 {
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion aws/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
17 changes: 15 additions & 2 deletions azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -232,6 +232,13 @@ func (a *azureOps) Attach(diskName string) (string, error) {
return a.waitForAttach(diskName)
}

func (a *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 &&
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion azure/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
96 changes: 73 additions & 23 deletions backoff/exponential.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 (
Expand All @@ -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)
}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
31 changes: 27 additions & 4 deletions cloudops.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,14 @@ 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 zone
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)
Expand All @@ -90,8 +94,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: passthrough

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also at other places.

// 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
Expand All @@ -109,7 +118,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.
Expand Down Expand Up @@ -140,3 +152,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
}
17 changes: 17 additions & 0 deletions errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading