vSphere Provider - Fix destroy when vm is powered off or has no networks

[dkalleg]

This patch adds a wait when powering on a vm so setupVirtualMachine does
not return until the vm is actually powered on. This allows other
functions to work off the assumption that the current state of the vm
is not in flux. During resourceVSphereVirtualMachineRead(), the wait for
IP would cause a hang for any VM with no network interfaces or for vms
that had been powered off for any reason. This also means that the user
could not delete a vm with no network interfaces or that is powered off.
Checking power state before trying to check for network interfaces.
Resolves #7168

[dkalleg]

This does add an interesting caveat where if you try to run terraform plan when a vm is powered off, it will find a diff between the "discovered networks" and the "declared networks"

    name:                                   "tfDestroyFix" => "tfDestroyFix"
    network_interface.#:                    "0" => "1" (forces new resource)
    network_interface.0.ip_address:         "" => "<computed>"
    network_interface.0.ipv4_address:       "" => "<computed>"
    network_interface.0.ipv4_gateway:       "" => "<computed>"
    network_interface.0.ipv4_prefix_length: "" => "<computed>"
    network_interface.0.ipv6_address:       "" => "<computed>"
    network_interface.0.ipv6_gateway:       "" => "<computed>"
    network_interface.0.ipv6_prefix_length: "" => "<computed>"
    network_interface.0.label:              "" => "VM Public" (forces new resource)
    network_interface.0.mac_address:        "" => "<computed>"
    network_interface.0.subnet_mask:        "" => "<computed>"
    skip_customization:                     "false" => "false"

This is sort of a grey area since handling a powered off vm is not really a handled case at the moment. This prompts a discussion about the ability to handle/recover from different power states, but that should be in a separate issue/PR :) . This PR resolves a known issue of not being able to destroy a vm that is powered off or has no network interfaces.

[dkalleg]

@chrislovecnm @stack72 @phinze
Adding folks for visibility, please review at your convenience.

[chrislovecnm]

When should we be waiting for all ips, one ip, or customization to be done?

This has been a constant thorn in our side, and this PR looks like a good point to address it ;)

[thetuxkeeper]

the WaitForNetIP in the update function (line 622) can be removed. I don't see a reason why it's still there since it's also in the read function, which will be called just a few lines later.
Everything else looks good. Nice improvement/bug fix :)

[dkalleg]

@thetuxkeeper I did a test with that WaitForNetIP() removed and the Update() called the Read() before the vm was up, which resulted in the Read() not finding the networks and messing up the tfstate. I think it's important to have at the end of the Update() so that the Read() happens once the vm is in a stable state.
@chrislovecnm Hi Paranoid, I'm Dan. :) Adding that check. Considering that we're populating networkInterfaces in a way that may or may not have the ipv4_address key, its a good idea.

[chrislovecnm]

@dkalleg we are dev ops guys, not be paranoid gets us woken up at 2am 😄

[thetuxkeeper]

@dkalleg : that's strange. that was the reason why I put the property.DefaultCollector after the WaitForNetIP call in Read() (was the other way round before the WaitForNetIP PR and had this exact problem). That needs some more testing or at least logs since it's not intended that we need to call it twice. It makes it just harder for future changes (bigger refactoring or stuff like that).
But I don't think it's a blocker for this PR. We can think about it in another issue/PR. Will try to reproduce it, when I have time.

[chrislovecnm]

@thetuxkeeper you have a chance to test this?

[dkalleg]

@thetuxkeeper @stack72 @phinze @jen20 @chrislovecnm
Any further actionable feedback?

[dkalleg]

@thetuxkeeper @stack72 @phinze @jen20 @chrislovecnm
Just checking in. Any thoughts on this?

[thetuxkeeper]

Nothing from me. I think we can merge it. If I find the time to run the tests, I'll do it. But would be safer if someone else did it (I'm still struggling with this new cloud stuff on detail level ... :D )

[dkalleg]

@stack72 Could you take a look at this?

[chrislovecnm]

@dkalleg do we need a test for this? Otherwise, what is the status of you running the integration tests? Overall looks great, and I appreciate the effort.

[dkalleg]

@chrislovecnm Sorry, missed this. Tests are passing on my end. This might be a little overly specific of a use case to make a test for.

[dkalleg]

Thanks @stack72 !

[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.