Adding controller creation logic

[dkalleg]

If a scsi or ide controller does not exist at the time a new disk
or cdrom needs it, we need to add that controller to the vm.
Also adding logic to handle reading either a flat or sparse
disk from vSphere.

[dkalleg]

FYI, my opinion is that the flat/sparse logic is a bit of a work around, the real fix would restructuring the hierarchy of govmomi type structs. The disk.Backing attribute we are returned is of type type.BaseVirtualDeviceBackingInfo. There are multiple types that inherit this, this PR focusing on types.VirtualDiskFlatVer2BackingInfo and types.VirtualDiskSparseVer2BackingInfo because those are the only I've seen returned, and those are the only that have FileName and Uuid attributes we need. We may be given either of these types, which is why I've added the extra type assertion logic. It may be cleaner to add a layer in the hierarchy between these two and BaseVirtualDeviceBackingInfo. Hoping to hear some opinions on this; whether the logic here is enough in the long term or if we should talk to govmomi folks.

[dkalleg]

@stack72 I should have mentioned this before to help prioritization: This is a fix for a bug in the addHardDisk function. Without this, adding a disk as IDE in common cases will result in an unhandled Terraform exception: "no available IDE controller". The flat/sparse logic is handling disk dereferencing in the same case.

[chrislovecnm]

@dkalleg if this is a bug fix, which unit test should have caught this??

[dkalleg]

I would have expected testAccCheckVSphereVirtualMachineConfig_initType to catch it since it adds both a scsi and an ide disk. Knowing that we saw a pass suggests that the circumstances of the test gave both an ide and scsi controller were created. HOW they were created, I've yet to figure out.. Could have something to do with the specific template I was testing with.

[chrislovecnm]

@dkalleg ready for merge?

[dkalleg]

If nobody has comments, then it's ready to go.

[stack72]

Hi @dkalleg / @chrislovecnm

As long as you guys are then this is fine with me :) Can we make sure and follow up with some tests on it?

P.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.