r/virtual_machine: Add support for remote backed CDROMs #421
Conversation
Previously, only ISO backed CDROMs were supported. This adds the ability to use a remote client device instead.
There was a problem hiding this comment.
@bill-rich, amazing effort! Thank you very much for this.
I've attached a number of review comments. The most important thing is that we get this out of Create and Update and over into CustomizeDiff. This will ensure the validation is done at diff-time versus apply-time.
Looking forward to getting this in! 🎉
| "client_device": { | ||
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| Description: "Indicates whether the device should be backed by remote client device", |
There was a problem hiding this comment.
Minor can we edit this so it reads:
Indicates whether the device should be mapped to a remote client device.
| @@ -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") | |||
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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") |
| return fmt.Errorf("ValidateCdromConfig: Either client_device or datastore_id and path must be set") | ||
| } | ||
| } | ||
| log.Printf("[DEBUG] ValidateCdromConfig: Config validation complete") |
There was a problem hiding this comment.
This line should follow the same kind of formatting as the starting line, example:
log.Printf("[DEBUG] %s: Config validation complete", r)
| } | ||
| device = l.InsertIso(device, dsPath.String()) | ||
| l.Connect(device) | ||
| } else if clientDevice { |
There was a problem hiding this comment.
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
}| } | ||
|
|
||
| device = l.InsertIso(device, dsPath.String()) | ||
| l.Connect(device) | ||
| if dsID != "" && path != "" { |
There was a problem hiding this comment.
This might be a good candidate for a unexported helper as this code looks pretty much the same as it does in Create.
| } | ||
|
|
||
| cdrom { | ||
| client_device = true |
There was a problem hiding this comment.
Remember to filter the fixture code through terraform fmt - to fix these formatting issues that will normally be hidden from you in the editor.
| size = 20 | ||
| } | ||
|
|
||
| cdrom { |
There was a problem hiding this comment.
This should be (and will be when it's run through terraform fmt -) cdrom {}.
| size = 20 | ||
| } | ||
|
|
||
| cdrom { |
There was a problem hiding this comment.
Formatting needs to be fixed here too.
Fix up comments and logging, move cdrom validation step to CustomizeDiff, use switches, move reused code to a helper function, and fix HCL formatting.
Use an empty config step for tests where there is an expected failure in CustomizeDiff, and wrap cdrom docs at 80 chars.
|
LGTM! |
This does a few things: * Looks like some acceptance test were broken - first one was due to something changing in RancherOS that does not allow for the proper hot-remove of virtual NICs anymore. I've adjusted this test so it powers off the VM first, which is not so much an issue as we aren't testing for a hot-remove anyway, but more to make sure that things are consistent in state. * The vApp property tests were missing settings for thin_provisioned and eagerly_scrub, which should be coming from the template data sources. These were failing as a result as the CoreOS OVA does not use thin provisioned disks. * Finally, #421 introduced a change where we were actually failing if we didn't recognize the CD drive type. There are a few we still don't support, namely passthrough CD drives. I've changed this to a debug log message, and a unsetting of all parameters. This should ensure that devices that aren't recognized but need to be changed to a recognized device are properly accounted for in the diff, in addition to removing unrecognized devices in the event no CDROM drive is necessary.
ghost
locked and limited conversation to collaborators
Previously, only ISO backed CDROMs were supported. This adds the ability to use
a remote client device instead.