Shutdown VMs before destroying them by 4censord · Pull Request #212 · vatesfr/terraform-provider-xenorchestra

4censord · 2022-10-06T19:24:50Z

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 · 2022-10-26T19:59:02Z

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

4censord · 2022-10-26T23:40:31Z

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 · 2022-10-27T18:03:01Z

Understood! Completely forgot that was a use case!

TIL

ddelnano · 2022-12-09T00:59:50Z

+	vmReq := client.Vm{
+		Id: d.Id(),
+	}
+	err := c.HaltVm(vmReq)


Have you tested what will happen if a VM is already shutdown? I'm thinking about how this would behave if the resourceVmDelete were to partially fail or timeout in the halting step.

Testing this in my environment with the xo-cli shows that this will throw an error:

ddelnano@ddelnano-desktop:~/go/src/github.com/ddelnano/terraform-provider-xenorchestra$ xo-cli vm.stop id=affc9a76-c03f-435f-96e5-962ebfacd0a8 ✖ VM state is halted but should be running JsonRpcError: VM state is halted but should be running at Peer._callee$ (/usr/lib/node_modules/xo-cli/node_modules/json-rpc-peer/dist/index.js:212:17) at tryCatch (/usr/lib/node_modules/xo-cli/node_modules/babel-runtime/node_modules/regenerator-runtime/runtime.js:62:40) at Generator.invoke [as _invoke] (/usr/lib/node_modules/xo-cli/node_modules/babel-runtime/node_modules/regenerator-runtime/runtime.js:296:22) at prototype.<computed> [as next] (/usr/lib/node_modules/xo-cli/node_modules/babel-runtime/node_modules/regenerator-runtime/runtime.js:114:21) at step (/usr/lib/node_modules/xo-cli/node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30) at /usr/lib/node_modules/xo-cli/node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14 at new Promise (<anonymous>) at new F (/usr/lib/node_modules/xo-cli/node_modules/core-js/library/modules/_export.js:36:28) at Peer.<anonymous> (/usr/lib/node_modules/xo-cli/node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12) at Peer.exec (/usr/lib/node_modules/xo-cli/node_modules/json-rpc-peer/dist/index.js:256:21) at Peer.write (/usr/lib/node_modules/xo-cli/node_modules/json-rpc-peer/dist/index.js:367:12) at Xo.<anonymous> (/usr/lib/node_modules/xo-cli/node_modules/jsonrpc-websocket-client/dist/index.js:92:12) at Xo.emit (node:events:513:28) at WebSocket.<anonymous> (/usr/lib/node_modules/xo-cli/node_modules/jsonrpc-websocket-client/dist/websocket-client.js:232:20) at WebSocket.onMessage (/usr/lib/node_modules/xo-cli/node_modules/ws/lib/EventTarget.js:99:16) at WebSocket.emit (node:events:513:28)

I think we should handle this case in the event it is partially applied. We could fetch the VM first and then only call HaltVm if it is running.

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.

In the meantime, I'm running the test suite to validate that the change in its current form is working.

I have updated it so we only shutdown running vms.

This also means that Suspended and Paused vm will still be terminated without a propper shutdown or warning.

I don't see a good method to fixing that tho, as "resuming a paused vm on destroy" might not be the expected action.

This also means that Suspended and Paused vm will still be terminated without a propper shutdown or warning.

This should only trigger during a terraform destroy or when applying a plan that removed the VM. So terraform should make it fairly obvious that it's going to perform a destructive action. My original concern is allowing a terraform destroy or terraform apply that can never converge because it stops the VM but can't proceed with the termination.

What specific scenarios are you worried about that would be unexpected?

ddelnano · 2022-12-09T01:11:38Z

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.

This allows the VM to terminate its work properly.

As discussed in vatesfr#212 Trying to stop a VM that is not running results in an error, so we only do shutdowns for running VMs now Possible power-states are "Running, Halted, Paused, Suspended"

4censord · 2022-12-09T14:28:47Z

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 · 2022-12-09T22:20:55Z

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.

As discussed in #212 Trying to stop a VM that is not running results in an error, so we only do shutdowns for running VMs now Possible power-states are "Running, Halted, Paused, Suspended"

ddelnano · 2022-12-14T19:41:21Z

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

As discussed in #212 Trying to stop a VM that is not running results in an error, so we only do shutdowns for running VMs now Possible power-states are "Running, Halted, Paused, Suspended"

ddelnano · 2022-12-14T23:17:04Z

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.

ddelnano reviewed Dec 9, 2022

View reviewed changes

Shutdown VMs before destroying them

c15010d

This allows the VM to terminate its work properly.

4censord force-pushed the shutdown_vm_before_destroy branch from 92c59d3 to c15010d Compare December 9, 2022 12:12

Only try to shutdown VM if running

433aa95

As discussed in vatesfr#212 Trying to stop a VM that is not running results in an error, so we only do shutdowns for running VMs now Possible power-states are "Running, Halted, Paused, Suspended"

This was referenced Dec 14, 2022

Improve XO client package's handling of VM shutdown #220

Closed

Improve XO client's handling of VM shutdown #221

Merged

ddelnano mentioned this pull request Dec 14, 2022

Enhance xenorchestra_vm to provide a method for graceful termination #222

Open

3 tasks

ddelnano closed this Dec 14, 2022

Navigation Menu

Uh oh!

Shutdown VMs before destroying them - #212

Shutdown VMs before destroying them#212
4censord wants to merge 2 commits into
vatesfr:masterfrom
4censord:shutdown_vm_before_destroy

4censord commented Oct 6, 2022

Uh oh!

userbradley commented Oct 26, 2022

Uh oh!

4censord commented Oct 26, 2022 •

edited

Loading

Uh oh!

userbradley commented Oct 27, 2022

Uh oh!

ddelnano Dec 9, 2022

Uh oh!

ddelnano Dec 9, 2022

Uh oh!

4censord Dec 9, 2022

Uh oh!

4censord Dec 9, 2022

Uh oh!

ddelnano Dec 9, 2022 •

edited

Loading

Uh oh!

ddelnano commented Dec 9, 2022

Uh oh!

4censord commented Dec 9, 2022

Uh oh!

ddelnano commented Dec 9, 2022 •

edited

Loading

Uh oh!

ddelnano commented Dec 14, 2022

Uh oh!

ddelnano commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

4censord commented Oct 6, 2022

Uh oh!

userbradley commented Oct 26, 2022

Uh oh!

4censord commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

userbradley commented Oct 27, 2022

Uh oh!

ddelnano Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

ddelnano Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

4censord Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

4censord Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

ddelnano Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddelnano commented Dec 9, 2022

Uh oh!

4censord commented Dec 9, 2022

Uh oh!

ddelnano commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddelnano commented Dec 14, 2022

Uh oh!

ddelnano commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

4censord commented Oct 26, 2022 •

edited

Loading

ddelnano Dec 9, 2022 •

edited

Loading

ddelnano commented Dec 9, 2022 •

edited

Loading