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.

Sign up for GitHub

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

r/virtual_machine: Add support for remote backed CDROMs #421

Merged
merged 6 commits into from
Mar 7, 2018
Merged

r/virtual_machine: Add support for remote backed CDROMs #421

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52e17d0
r/virtual_machine: Add support for remote backed CDROMs
bill-rich Mar 5, 2018
ddcffe3
Address code review comments
bill-rich Mar 7, 2018
e22deb6
Use empty config and wrap docs
bill-rich Mar 7, 2018
724d71a
Use switch instead of if/else
bill-rich Mar 7, 2018
048a0fe
Fix test names
bill-rich Mar 7, 2018
f133ca8
Add test for client cdrom with clone
bill-rich Mar 7, 2018
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
124 changes: 86 additions & 38 deletions vsphere/internal/virtualdevice/virtual_machine_cdrom_subresource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,20 @@ func CdromSubresourceSchema() map[string]*schema.Schema {
// VirtualDeviceFileBackingInfo
"datastore_id": {
Type: schema.TypeString,
Required: true,
Optional: true,
Description: "The datastore ID the ISO is located on.",
},
"path": {
Type: schema.TypeString,
Required: true,
Optional: true,
Description: "The path to the ISO file on the datastore.",
},
// VirtualCdromRemoteAtapiBackingInfo
"client_device": {
Type: schema.TypeBool,
Optional: true,
Description: "Indicates whether the device should be backed by remote client device",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor can we edit this so it reads:

Indicates whether the device should be mapped to a remote client device.

},
}
structure.MergeSchema(s, subresourceSchema())
return s
Expand Down Expand Up @@ -378,6 +384,25 @@ func CdromPostCloneOperation(d *schema.ResourceData, c *govmomi.Client, l object
return l, spec, nil
}

// ValidateCdromConfig takes a CdromSubresource and ensures that the associated configuration is valid
func (r *CdromSubresource) ValidateCdromConfig() error {
log.Printf("[DEBUG] ValidateCdromConfig: Beginning CDROM configuration validation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Subresource debug lines should include the specific resource address. For this log entry, this means it would look like:

log.Printf("[DEBUG] %s: Beginning CDROM configuration validation", r)

Function does not necessarily need to be referenced here.

dsID := r.Get("datastore_id").(string)
path := r.Get("path").(string)
clientDevice := r.Get("client_device").(bool)
if clientDevice {
if dsID != "" || path != "" {
return fmt.Errorf("ValidateCdromConfig: Cannot have both client_device parameter and ISO file parameters (datastore_id, path) set")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the function name from these error messages? They will be exposed to the user during regular output.

}
} else {
if dsID == "" || path == "" {
return fmt.Errorf("ValidateCdromConfig: Either client_device or datastore_id and path must be set")
Copy link
Contributor

Choose a reason for hiding this comment

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

As per above

}
}
log.Printf("[DEBUG] ValidateCdromConfig: Config validation complete")
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should follow the same kind of formatting as the starting line, example:

log.Printf("[DEBUG] %s: Config validation complete", r)

return nil
}

// Create creates a vsphere_virtual_machine cdrom sub-resource.
func (r *CdromSubresource) Create(l object.VirtualDeviceList) ([]types.BaseVirtualDeviceConfigSpec, error) {
log.Printf("[DEBUG] %s: Running create", r)
Expand All @@ -389,30 +414,40 @@ func (r *CdromSubresource) Create(l object.VirtualDeviceList) ([]types.BaseVirtu
}

// We now have the controller on which we can create our device on.
dsID := r.Get("datastore_id").(string)
path := r.Get("path").(string)
ds, err := datastore.FromID(r.client, dsID)
if err != nil {
return nil, fmt.Errorf("cannot find datastore: %s", err)
}
dsProps, err := datastore.Properties(ds)
err = r.ValidateCdromConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

ValidateCdromConfig should not be called here - this should have been done as part of a diff operation - so a function like CdromDiffOperation should have been created similar to the ones seen in disk and network device sub-resources. Each resource should be iterated over and run through the diff validation. This ensures that the validation is done at plan time and does not create confusion when someone attempts to do a create or update and runs into a configuration error they were told was okay before.

if err != nil {
return nil, fmt.Errorf("could not get properties for datastore: %s", err)
}
dsName := dsProps.Name

dsPath := &object.DatastorePath{
Datastore: dsName,
Path: path,
return nil, err
}

dsID := r.Get("datastore_id").(string)
path := r.Get("path").(string)
clientDevice := r.Get("client_device").(bool)
device, err := l.CreateCdrom(ctlr.(*types.VirtualIDEController))
if err != nil {
return nil, err
}
device = l.InsertIso(device, dsPath.String())
l.Connect(device)

if dsID != "" && path != "" {
// If the datastore ID and path are both set, the CDROM will be backed by a file on a datastore
ds, err := datastore.FromID(r.client, dsID)
if err != nil {
return nil, fmt.Errorf("cannot find datastore: %s", err)
}
dsProps, err := datastore.Properties(ds)
if err != nil {
return nil, fmt.Errorf("could not get properties for datastore: %s", err)
}
dsName := dsProps.Name
dsPath := &object.DatastorePath{
Datastore: dsName,
Path: path,
}
device = l.InsertIso(device, dsPath.String())
l.Connect(device)
} else if clientDevice {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try to avoid else if as it's not necessarily good Go.

This actually can be written as a switch:

switch {
case dsID != "" && path != "":
  ...
case clientDevice == true:
  // You might want to use the full-form boolean expression above to be specific about intent
  ...
default:
  // This would be where your "else" goes, if you had one
}

// If set to use the client device, then the CDROM will be backed by a remote device
device.Backing = &types.VirtualCdromRemoteAtapiBackingInfo{
VirtualDeviceRemoteDeviceBackingInfo: types.VirtualDeviceRemoteDeviceBackingInfo{},
}
}
// Done here. Save IDs, push the device to the new device list and return.
if err := r.SaveDevIDs(device, ctlr); err != nil {
return nil, err
Expand All @@ -438,9 +473,10 @@ func (r *CdromSubresource) Read(l object.VirtualDeviceList) error {
if !ok {
return fmt.Errorf("device at %q is not a virtual CDROM device", l.Name(d))
}
if backing, ok := device.Backing.(*types.VirtualCdromIsoBackingInfo); ok {
// Only read backing info if it's available. If not, this is a host
// device/client mapping that is not managed by TF and needs to be deleted.
// Only read backing info if it's available.
if _, ok := device.Backing.(*types.VirtualCdromRemoteAtapiBackingInfo); ok {
r.Set("client_device", true)
} else if backing, ok := device.Backing.(*types.VirtualCdromIsoBackingInfo); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, let's try to avoid else if as it's not necessarily good Go.

Instead, write this as a type switch:

switch backing := device.Backing.(type) {
case *types.VirtualCdromRemoteAtapiBackingInfo:
  r.Set("client_device", true)
case *types.VirtualCdromIsoBackingInfo:
  ...
default:
  return nil, fmt.Errorf("device at %q is not a virtual CDROM device", l.Name(d))
}

dp := &object.DatastorePath{}
if ok := dp.FromString(backing.FileName); !ok {
return fmt.Errorf("could not read datastore path in backing %q", backing.FileName)
Expand Down Expand Up @@ -472,27 +508,39 @@ func (r *CdromSubresource) Update(l object.VirtualDeviceList) ([]types.BaseVirtu
return nil, fmt.Errorf("device at %q is not a virtual CDROM device", l.Name(d))
}

// To update, we just re-insert the ISO as per create, and send it as an edit.
dsID := r.Get("datastore_id").(string)
path := r.Get("path").(string)
ds, err := datastore.FromID(r.client, dsID)
clientDevice := r.Get("client_device").(bool)
err = r.ValidateCdromConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

As per the comment in validation being in Create, this needs to be removed in favour of one that happens during diff customization.

if err != nil {
return nil, fmt.Errorf("cannot find datastore: %s", err)
}
dsProps, err := datastore.Properties(ds)
if err != nil {
return nil, fmt.Errorf("could not get properties for datastore: %s", err)
}
dsName := dsProps.Name

dsPath := &object.DatastorePath{
Datastore: dsName,
Path: path,
return nil, err
}

device = l.InsertIso(device, dsPath.String())
l.Connect(device)
if dsID != "" && path != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a good candidate for a unexported helper as this code looks pretty much the same as it does in Create.

// If the datastore ID and path are both set, the CDROM will be backed by a file on a datastore
// To update, we just re-insert the ISO as per create, and send it as an edit.
ds, err := datastore.FromID(r.client, dsID)
if err != nil {
return nil, fmt.Errorf("cannot find datastore: %s", err)
}
dsProps, err := datastore.Properties(ds)
if err != nil {
return nil, fmt.Errorf("could not get properties for datastore: %s", err)
}
dsName := dsProps.Name

dsPath := &object.DatastorePath{
Datastore: dsName,
Path: path,
}
device = l.InsertIso(device, dsPath.String())
l.Connect(device)
} else if clientDevice {
// If set to use the client device, then the CDROM will be backed by a remote device
device.Backing = &types.VirtualCdromRemoteAtapiBackingInfo{
VirtualDeviceRemoteDeviceBackingInfo: types.VirtualDeviceRemoteDeviceBackingInfo{},
}
}
spec, err := object.VirtualDeviceList{device}.ConfigSpec(types.VirtualDeviceConfigSpecOperationEdit)
if err != nil {
return nil, err
Expand Down
Loading