search

vatesfr / terraform-provider-xenorchestra

Xen Orchestra provider for Terraform
MIT License
151 stars 32 forks source link

Shutdown VMs before destroying them #212

Closed 4censord closed 1 year ago

4censord commented 1 year ago

Instead of simply terminating the vm without any warning, this tries to shut down the vm first. This allows the VM to terminate its work properly.

Because of how client.HaltVm() is implemented, this has a not configurable 2 minutes timeout See: https://github.com/terra-farm/terraform-provider-xenorchestra/blob/master/client/vm.go#L410

userbradley commented 1 year ago

Just trying to understand why you would want to shutdown an instance before nuking it? You're deleting them so why worry?

4censord commented 1 year ago

In my case I am running database clusters. For upgrades, I recreate VMS one by one via terraform. If I simply delete VMs, the cluster has to perform "recovery" steps to regain its quorum. This for example stops all DB writes for some time. If instead the vm shuts down cleanly, and then gets replaced the cluster doesn't go into a degraded state.

userbradley commented 1 year ago

Understood! Completely forgot that was a use case!

TIL

ddelnano commented 1 year ago

Thanks for the contribution and agree that for the situation you mentioned would benefit from a graceful deletion. Left a comment about handling a potential error case and providing test coverage for it.

4censord commented 1 year ago

Ideally we'd have a test case for this situation as well. I'm happy to write that test or provide details on how that could be done. Please let me know what you'd prefer.

I am fine with either. I hadn't yet thought about how to test this, so i would just take the brute force type approach of creating 4 VMs, setting them to the different power states and having them be destroyed.

If this is fine, I could add that

ddelnano commented 1 year ago

I don't think we need to test all 4 states. I think testing two use cases is fine: running VM is terminated and stopped VM is terminated. The former will already have coverage since the terraform test runner performs a destroy when the test is done.

In order to test the stopped case, we can stop the VM mid test case by using the PreConfig setting. Please see the TestAccXenorchestraVm_attachDisconnectedDisk test for an example of this (source). Then we can finish the test with a resource.TestCase with the Destroy field set to true.

ddelnano commented 1 year ago

@4censord I should have a PR open soon that includes the extra testing. #220 created complications along the way so I am addressing that first.

ddelnano commented 1 year ago

I'm going to close this in favor of #222. Adding the acceptance testing was more difficult than I anticipated (see new PR for details).

I preserved your commit history as part of my changes.