New compute methods for vSphere, storage operations by instance ID, misc vSphere fixes #23
Conversation
…isc vSphere fixes - Fix error types 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 <[email protected]>
Signed-off-by: Harsh Desai <[email protected]>
|
Can you split the vendor code and the actual cloudops changes into different commits ? |
| err error | ||
| ) | ||
|
|
||
| if len(instanceID) == 0 { |
There was a problem hiding this comment.
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
| @@ -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 | |||
| @@ -408,7 +415,7 @@ func (s *gceOps) DeleteInstance(instanceID string, zone string, timeout time.Dur | |||
| instanceID, operation.Name, operation.Status) | |||
| } | |||
|
|
|||
| _, err = task.DoRetryWithTimeout(f, timeout, retrySeconds*time.Second) | |||
| _, err = task.DoRetryWithTimeout(f, 2*time.Minute, retrySeconds*time.Second) | |||
There was a problem hiding this comment.
nit: add this as a constant.
| require.NotNil(t, info, "got nil instance info from inspect") | ||
| require.NotEmpty(t, info.Zone, "inspect must returns instance zone") | ||
|
|
||
| instances, err = driver.ListInstances(&cloudops.ListInstancesOpts{ |
There was a problem hiding this comment.
Can add a UT using LabelSelectors
| StoragePod string | ||
| // Datastore is the VMFS datastore to use for the VM | ||
| Datastore string | ||
| // ResourcePool is the resource pool to use to place to VM |
There was a problem hiding this comment.
nit: ResourcePool is the resource pool where the VM is placed
| return folder, nil | ||
| } | ||
|
|
||
| func (ops *vsphereOps) getFinderAndDC(ctx context.Context, datacenter string) ( |
There was a problem hiding this comment.
nit:
| func (ops *vsphereOps) getFinderAndDC(ctx context.Context, datacenter string) ( | |
| func (ops *vsphereOps) getFinderAndDC( | |
| ctx context.Context, | |
| datacenter string, | |
| ) (*find.Finder, *object.Datacenter, error) { |
| } | ||
|
|
||
| func (ops *vsphereOps) ListInstances(opts *cloudops.ListInstancesOpts) ( | ||
| []*cloudops.InstanceInfo, error) { |
There was a problem hiding this comment.
nit:
| []*cloudops.InstanceInfo, error) { | |
| func (ops *vsphereOps) ListInstances( | |
| opts *cloudops.ListInstancesOpts, | |
| ) ([]*cloudops.InstanceInfo, error) { |
|
|
||
| var datacenter string | ||
| if opts != nil && opts.LabelSelector != nil { | ||
| datacenter = opts.LabelSelector["datacenter"] |
There was a problem hiding this comment.
nit: "datacenter" seems to be used at multiple places. It can be a constant.
| return false, err | ||
| } | ||
|
|
||
| return o.Runtime.ConnectionState != "invalid", nil |
There was a problem hiding this comment.
Aren't there any constants defined in vsphere library for such states?
| return o.Config.Name, nil | ||
| } | ||
|
|
||
| // We treat a VM's zone as the vSphere cluster in which the vm resizes. |
There was a problem hiding this comment.
nit:
| // We treat a VM's zone as the vSphere cluster in which the vm resizes. | |
| // We treat a VM's zone as the vSphere cluster in which the vm resides. |
Signed-off-by: Harsh Desai [email protected]