Shutdown VMs before destroying them #212
Conversation
|
Just trying to understand why you would want to shutdown an instance before nuking it? You're deleting them so why worry? |
|
In my case I am running database clusters. |
|
Understood! Completely forgot that was a use case! TIL |
xoa/resource_xenorchestra_vm.go
Outdated
| vmReq := client.Vm{ | ||
| Id: d.Id(), | ||
| } | ||
| err := c.HaltVm(vmReq) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have updated it so we only shutdown running vms.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
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.
4censord
force-pushed
the
shutdown_vm_before_destroy
branch
from
92c59d3 to
c15010d
Compare
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"
I am fine with either. If this is fine, I could add that |
|
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 |
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"
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"
|
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. |
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 timeoutSee: https://github.com/terra-farm/terraform-provider-xenorchestra/blob/master/client/vm.go#L410