On 07.11.2018 17:29, John Ferlan wrote:
On 10/8/18 7:21 AM, Nikolay Shirokovskiy wrote:
Let's introduce shutdown reason "daemon" which means we have to kill running domain ourselves as the best action we can take at that moment. Failure to pick up domain on daemon restart is one example of such case. Using reason "crashed" is a bit misleading as it is used when qemu is actually crashed or qemu destroyed on guest panic as result of user choice that is the problem was in qemu/guest itself. So I propose to use "crashed" only for qemu side issues and introduce "daemon" when we have to kill the qemu for good.
There is another example where "daemon" will be useful. If we can not reboot domain we kill it and got "crashed" reason now. Looks like good candidate for "daemon" reason.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 3 ++- src/qemu/qemu_process.c | 11 ++++------- tools/virsh-domain-monitor.c | 3 ++- 4 files changed, 9 insertions(+), 9 deletions(-)
[...]
Now that I've pushed commits 296e05b54 and 8f0f8425d, let's revisit this... Merging in that set of changes with this patch means that instead of:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e9c7618..c4bc9ca 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7988,15 +7988,12 @@ qemuProcessReconnect(void *opaque) /* We can't get the monitor back, so must kill the VM * to remove danger of it ending up running twice if * user tries to start it again later - * If we couldn't get the monitor since QEMU supports - * no-shutdown, we can safely say that the domain - * crashed ... */ - state = VIR_DOMAIN_SHUTOFF_CRASHED; - /* If BeginJob failed, we jumped here without a job, let's hope another + * If BeginJob failed, we jumped here without a job, let's hope another * thread didn't have a chance to start playing with the domain yet * (it's all we can do anyway). */ - qemuProcessStop(driver, obj, state, QEMU_ASYNC_JOB_NONE, stopFlags); + qemuProcessStop(driver, obj, VIR_DOMAIN_SHUTOFF_DAEMON, + QEMU_ASYNC_JOB_NONE, stopFlags); } goto cleanup; } @@ -8035,7 +8032,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, * is no thread that could be doing anything else with the same domain * object. */ - qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, + qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_DAEMON, QEMU_ASYNC_JOB_NONE, 0); qemuDomainRemoveInactiveJobLocked(src->driver, obj);
We'd have:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3bf84834ec..023e187729 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7995,10 +7995,14 @@ qemuProcessReconnect(void *opaque) * * If we cannot get to the monitor when the QEMU command * line used -no-shutdown, then we can safely say that the - * domain crashed; otherwise, we don't really know. */ + * domain crashed; otherwise, if the monitor was started, + * then we can blame ourselves, else we failed before the + * monitor started so we don't really know. */ if (!priv->mon && tryMonReconn && qemuDomainIsUsingNoShutdown(priv)) state = VIR_DOMAIN_SHUTOFF_CRASHED; + else if (priv->mon) + state = VIR_DOMAIN_SHUTOFF_DAEMON; else state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
@@ -8045,7 +8049,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, * is no thread that could be doing anything else with the same domain * object. */ - qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, + qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_DAEMON, QEMU_ASYNC_JOB_NONE, 0);
I think we need to report VIR_DOMAIN_SHUTOFF_UNKNOWN here. This is similar to qemuProcessReconnect before we try to reconnect. May be this hunk need to be placed in distinct patch therefore.
qemuDomainRemoveInactiveJobLocked(src->driver, obj);
[...]
So does this essentially meet/handle the condition you were trying to alter? That is - once we get beyond the monitor reconnection, then if we end up in error that means the daemon has decided to stop things. Leaving the UNKNOWN to be the period prior to attempting to reconnect and CRASHED to the period during which we try to connect to the monitor.
This also captures failures after connection is successful (when priv->mon in qemuConnectMonitor).
Getting more specific failures of why the connection failed is perhaps a future task if it really matters.
This looks good enough for me too.
If this looks good to you let me know, I can merge and push with the previously noted changes and a commit message of:
"This patch introduces a new shutdown reason "daemon" in order to indicate that the daemon needed to force shutdown the domain as the best course of action to take at the moment.
This action would occur during reconnection when processing encounters an error once the monitor reconnection is successful."
Besides the first comment I'm fine with this change. I'm going to send a patch to change reason to VIR_DOMAIN_SHUTOFF_DAEMON in appropriate reboot failure cases too. Also one day let's fix the code not to kill unrelated process. Nikolay