Re: [PATCH] libvirt: add daemon itself as shutdown reason

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 16.10.2018 15:48, 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.
> 
> How about (although reading below may shed some more context):
> 
> 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 Reconnect processing when it's
> determined that either the QEMU monitor no longer exists for
> some unknown reason or creation of a thread to reconnect the
> domain failed.

Mostly I'm fine with this one except "monitor no longer exists"
is condition for reason "crashed" or "unknown" (Martin's contribution)

> 
>>
>> 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.
> 
> If you feel this way, then a followup patch could/should be posted;
> otherwise, this'll be lost to commit message heaven where all good ideas
> go to die ;-).

Sure!

> 
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>>  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(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
>> index fdd2d6b..11fdab5 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -145,6 +145,7 @@ typedef enum {
>>      VIR_DOMAIN_SHUTOFF_FAILED = 6,      /* domain failed to start */
>>      VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was
>>                                             * taken while domain was shutoff */
>> +    VIR_DOMAIN_SHUTOFF_DAEMON = 8,      /* daemon have to kill domain */
> 
> s/have to/decides to/
> 
>>  # ifdef VIR_ENUM_SENTINELS
>>      VIR_DOMAIN_SHUTOFF_LAST
>>  # endif
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9911d56..e441723 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -779,7 +779,8 @@ VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST,
>>                "migrated",
>>                "saved",
>>                "failed",
>> -              "from snapshot")
>> +              "from snapshot",
>> +              "daemon")
>>  
>>  VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
>>                "unknown",
>> 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
> 
> Since we're changing anyway, let's put the stop there, e.g.
> "s/later/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);
>>  
> 
> When qemuProcessReconnectHelper was introduced (commit d38897a5d)
> reconnection failure used VIR_DOMAIN_SHUTOFF_FAILED; however, that was
> changed in commit bda2f17d to either VIR_DOMAIN_SHUTOFF_CRASHED or
> VIR_DOMAIN_SHUTOFF_UNKNOWN. When QEMU_CAPS_NO_SHUTDOWN checking was
> removed in commit fe35b1ad6 the conditional state was just left at
> VIR_DOMAIN_SHUTOFF_CRASHED.
> 
> Still I agree w/ Martin's thoughts, which unfortunately wasn't commented
> on in the code leading to the latter revert commit losing that unknown
> reason. The reason for the "-no-shutdown" *did* change and that probably
> should have been reflected in the qemuProcessReconnect logic.
> 
> So I think perhaps we should have an initial patch which references or
> uses that first paragraph in order to change the code to:
> 
>         /* We cannot get the monitor back, so kill the VM to
>          * remove possibility of it ending up running twice if
>          * user tries to start it again later.
>          *
>          * 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. */
>         if (priv->monJSON && priv->allowReboot == VIR_TRISTATE_BOOL_YES)
>             state = VIR_DOMAIN_SHUTOFF_CRASHED;
>         else
>             state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
> 
> and uses the short commit msg of "qemu: Restore lost shutdown reason".
> 
> Then this followup would change the "UNKNOWN" to "DAEMON" keeping the
> CRASHED otherwise.
> 

Then we will get "crashed" reason always for modern qemu and domains
that can reboot which is not true.

However I see now that the issue is more complicated then I thought. I agree
with Martin too but we have to additionally check that connect(2) for domain
monitor socket was called and error was ECONNREFUSED. So finally state is
resolved like this:

if (connect(2) was not called) {
    state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
else if (connect(2) result is NOT ECONNREFUSED) {
    state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
} else if (connect(2) result is ECONNREFUSED) {
    if (priv->monJSON && priv->allowReboot == VIR_TRISTATE_BOOL_YES)
        state = VIR_DOMAIN_SHUTOFF_CRASHED;
    else
        state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
} else {
    state = VIR_DOMAIN_SHUTOFF_DAEMON;
}

Moreover we should not later kill process by remembered pid in all cases except
last one in the above code because we can kill some other process.

Looks like a bit complicated, not sure should we pursuit this one or just left
VIR_DOMAIN_SHUTOFF_UNKNOWN only (in this case we can introduce
VIR_DOMAIN_SHUTOFF_DAEMON for reboot failures where reason is obvisous).

Nikolay

> 
>> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
>> index 3a26363..f0ad558 100644
>> --- a/tools/virsh-domain-monitor.c
>> +++ b/tools/virsh-domain-monitor.c
>> @@ -212,7 +212,8 @@ VIR_ENUM_IMPL(virshDomainShutoffReason,
>>                N_("migrated"),
>>                N_("saved"),
>>                N_("failed"),
>> -              N_("from snapshot"))
>> +              N_("from snapshot"),
>> +              N_("daemon"))
>>  
>>  VIR_ENUM_DECL(virshDomainCrashedReason)
>>  VIR_ENUM_IMPL(virshDomainCrashedReason,
>>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux