Terraform-provider-libvirt: Destroying a domain should try to power it off first

[multani]

Version Reports:

Distro version of host:

Debian stretch

Terraform Version Report

v0.11.7

Libvirt version

3.0.0

terraform-provider-libvirt plugin version (git-hash)

58cc06611667bbbdddafacb411cd6bc8da1c2b00

---

Description of Issue/Question

When a domain is destroyed by Terraform, it should be possible to send it a "power-off" signal so that it has the chance to shut down properly.

Althoug the current method is very fast at replacing VMs, it is also very brutal and doesn't offer any chance for the VM to react correctly before disappearing (such as deregistering services upon shutdown, etc.).

This is the last part of that wiki paragraph:

An inelegant shutdown, also known as hard-stop:

# virsh destroy <domain>

This is equivalent to unplugging the power cable.

[dmacvicar]

I think there are flags on the libvirt API that controls the destruction of the domain to do something like this. However this would need some research.

[dmacvicar]

My biggest concern here is that we would need to introduce time-outs which would slow down everything, or am I wrong?

[MalloZup]

@multani just for curiosity, if you are going to destroy/remove forever a VM why (taking your quote) deregistering services upon shutdown is important ? I mean if the VM is removed you should stop the service before removing it, or if you are going to remove a vm the service inside the VM are not anymore important. Or i'm missing something :thinking: ?
I cannot figure out a use case for it. But maybe you know better on the subject.
To me at this point i agree with @dmacvicar that performance can be really impacted by this, especially if we are waiting for services to gracefull shutdown , this can take a while (and it's unpredictable performance depending on services), so i would stay currently with the current destroy without shutdown, unless we are missing really something important.

TIA for your time :sunflower:

[multani]

So, just to take this out: I like the current behavior which allows for very fast destroy/creation cycle and I think it should be kept somehow!

@MalloZup As for the use case: we have some services (Consul and Sensu, in particular) which are running on these VMs (they provide a way to register the VMs automatically in the service discovery when they come in). Destroying the VMs with the current behavior offers no chances at all for these services to signal they are shutting down (properly) and for the rest of the world, these VMs simply stopped working and we have to manually deregister them.
I could test it but if we were giving at least a few seconds to offer the VMs the chance to properly stop the services, that would simplify that procedure: the named services would gracefully shutdown, signaling to the rest of the world they are "leaving" and we won't have to manually cleanup after Terraform.

Also, AFAIK this would be a bit more similar to what's happening with other "instance providers" managed by Terraform, where Terraform requests a shutdown + removal of the VMs via the provider API instead of brutally removing them.

As for the performance impact: As I said at the beginning, we should keep the current behavior possible (aka. "very fast destroy"), definitely.
I'm not sure of the best way to implement my proposal, but I would do something like this:

• provide a shutdown_before_destroy boolean flag which defaults to False
• if nothing is specified, this keeps the current behavior
• if the flag is set to True, send the shutdown signal before sending the destroy signal
• as noted by @dmacvicar, we could also have a shutdown_timeout flag timeout associated defaulting to a reasonable (?) 30 seconds value.

If we have shutdown_before_destroy = False by default, the current "fast" behavior is not affected but we could support my use case for those who want.

[MalloZup]

@multani thx for the reply to your usecase and proposal suggestion for the implementation.

Personally i prefer the shutdown_before_destroy flag rather then shutdown_timeout because we would proper shutdown.
The timeout solution looks simple but imho is safer the better/proper one aka shutdown_before_destroy flag ( we could add maybe a maximal timeout there like 1H or so for waiting the destroy )

We need also finally to research on the libvirt side for using the right flags for this behaviours. i could have a look once i have some free time

[multani]

@MalloZup Actually, I would advocate to have both flags, with the defaults being:

shutdown_before_destroy = false
shutdown_timeout = "30s"

And users could override this as needed. That would prevent any hardcoded values and annoyance because it's too fast/too slow :)

We need also finally to research on the libvirt side for using the right flags for this behaviours.

I had a look and I'm actually surprised because it says the "destroy" call actually calls shutdown as well with a timeout, but it looks like it's driver dependent.
So, it might be better on the Terraform Libvirt provider side to call Shutdown() first and then Destroy()

[zeenix]

Destroying the VMs with the current behavior offers no chances at all for these services to signal they are shutting down (properly) and for the rest of the world

While that's very much true, the clients should be smart enough to detect that based on socket(s) being closed or is that somehow not possible in your case?

[zeenix]

Having said that, I can see maybe how it's not good enough for some cases so I'd like a PR that adds the suggested API, although i don't think we need a boolean flag, just the timeout one is enough and if it's set to 0, we just destroy the domain immediately. In any case, unless @multani (or someone else) is still interested and motivated to provide a PR for this, I suggest we close this.

[MalloZup]

I'm ok to close it and focus on core issues..

[multani]

Destroying the VMs with the current behavior offers no chances at all for these services to signal they are shutting down (properly) and for the rest of the world

While that's very much true, the clients should be smart enough to detect that based on socket(s) being closed or is that somehow not possible in your case?

They are, but then it's a different case between a machine is leaving properly and a machine disappears suddenly.
I'm not against working around the 2nd case, but still, it may be something completely unrelated to the machine disappearing: maybe it crashed and will come back in a minute or so, maybe the service inside crashed and broke the socket connection. There are only 2 different cases that require maybe more lookup on the ops side, that are currently impossible to distinguish from a VM being replaced using the Terraform provider.

Having said that, I can see maybe how it's not good enough for some cases so I'd like a PR that adds the suggested API, although i don't think we need a boolean flag, just the timeout one is enough and if it's set to 0, we just destroy the domain immediately.

If we agree on the interface, I can have a look to propose something in the coming weeks.
If it's decided to keep only the timeout, I guess it should work as:

• send the "shutdown" signal via the API
• wait until one of the following has been reached:
  
  
  
  • if the machine stops before the timeout, destroy it
  
  
  • if the machine didn't stop after timeout duration, destroy it.
  
  

In every cases we destroy it :thinking: but it can be faster in the case where it shuts down properly before the timeout has been reached.

[MalloZup]

I agree with @multani . Imho the boolean flag has it use-case here. @dmacvicar cc.

I'm re-opening. I will have currenlty no time in short-term to do it, but also even if it a stale issue, I feel that this issue belong to remains open.

IMHO it important, is not something that one could workaround easy as other libvirt issue where we just need to add compatibility. (but one can do with XSLT now)

[zeenix]

@MalloZup I'm a bit lost in the details here. Could you please tell me why a separate boolean flag is needed in your opinion?

About keeping the issue open, I strongly suggest not keeping issues open unless it's very important or there is active work going on the issue. In a volunteer-driven project, people get busy and even with their best intentions to work on something, they may never get to do it (or finish it). When issues are stale for a while, I'd suggest we close them with 'Reopen if you can provide a PR for this or is actually affecting you". Issues can alway be re-opened and closed multiple times. Just my two cents.

[zeenix]

If we agree on the interface, I can have a look to propose something in the coming weeks.
If it's decided to keep only the timeout, I guess it should work as:

* send the "shutdown" signal via the API

* wait until one of the following has been reached:

  * if the machine stops before the timeout, destroy it
  * if the machine didn't stop after `timeout` duration, destroy it.

Correct but my suggestion was that we don't send the shutdown if the timeout is set to 0 (which IMO should be the default), thus eliminating the need for a separate boolean flag.

[MalloZup]

@zeenix thx. I overlooked, sorry. I like the approach with timeout only.
@multani CC if you want to send PR with this. TIMEOUT = 0 should be the default value, a user can add more if he need hooks on that.

[zeenix]

@MalloZup Glad we agree. :) On that note, please close this one. @multani can re-open when they send the PR.