Re: [PATCH] libxl: support ACPI shutdown flag

Re: [PATCH] libxl: support ACPI shutdown flag

[Jim Fehlig]

Any comments on this small patch?  If possible, I'd like to push it for
the 1.2.4 release.

Thanks!
Jim

Jim Fehlig wrote:
> Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in
> libxlDomainShutdownFlags().  Inspired by similar functionality
> in the Xen xl client.
>
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>
> I considered invoking libxl_send_trigger() immediately when
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the
> end decided to only honor the flag if a "normal" shutdown request
> failed.  This behavior is similar to xl and conforms to the
> virDomainShutdownFlags() docs
>
> "If @flags is set to zero, then the hypervisor will choose the method
> of shutdown it considers best. To have greater control pass one or
> more of the virDomainShutdownFlagValues. The order in which the
> hypervisor tries each shutdown method is undefined, and a hypervisor
> is not required to support all methods."
>
> I'm certainly receptive to only invoking libxl_send_trigger() when
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that
> is a better approach.
>
>  src/libxl/libxl_driver.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b3f8df6..2b30c08 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -868,7 +868,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      int ret = -1;
>      libxlDomainObjPrivatePtr priv;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN, -1);
>  
>      if (!(vm = libxlDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -883,17 +883,35 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      }
>  
>      priv = vm->privateData;
> -    if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) {
> +    ret = libxl_domain_shutdown(priv->ctx, vm->def->id);
> +    if (ret == ERROR_NOPARAVIRT) {
> +        if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) {
> +            VIR_DEBUG("PV control interface not available, "
> +                      "sending ACPI power button event.");
> +            ret = libxl_send_trigger(priv->ctx, vm->def->id,
> +                                     LIBXL_TRIGGER_POWER, 0);
> +        } else {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("PV control interface not available, "
> +                               "preventing external graceful shutdown. "
> +                               "Consider using an ACPI power event."));
> +            ret = -1;
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (ret != 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to shutdown domain '%d' with libxenlight"),
>                         vm->def->id);
> +        ret = -1;
>          goto cleanup;
>      }
>  
> -    /* vm is marked shutoff (or removed from domains list if not persistent)
> +    /* ret == 0 == successful shutdown request
> +     * vm is marked shutoff (or removed from domains list if not persistent)
>       * in shutdown event handler.
>       */
> -    ret = 0;
>  
>   cleanup:
>      if (vm)
>   

Re: [PATCH] libxl: support ACPI shutdown flag

[Ian Campbell]

On Mon, 2014-04-28 at 11:24 -0600, Jim Fehlig wrote:
> Any comments on this small patch?  If possible, I'd like to push it for
> the 1.2.4 release.

FWIW your use of the libxl interfaces seems correct to me, so Ack in
that regard.

Ian.

Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag

[Daniel P. Berrange]

On Tue, Apr 22, 2014 at 11:12:16AM -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in
> libxlDomainShutdownFlags().  Inspired by similar functionality
> in the Xen xl client.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> I considered invoking libxl_send_trigger() immediately when
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the
> end decided to only honor the flag if a "normal" shutdown request
> failed.  This behavior is similar to xl and conforms to the
> virDomainShutdownFlags() docs
> 
> "If @flags is set to zero, then the hypervisor will choose the method
> of shutdown it considers best. To have greater control pass one or
> more of the virDomainShutdownFlagValues. The order in which the
> hypervisor tries each shutdown method is undefined, and a hypervisor
> is not required to support all methods."
> 
> I'm certainly receptive to only invoking libxl_send_trigger() when
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that
> is a better approach.

The way I consider shutdown to operate is slightly different to
how you've done it in your patch

 - flags == 0 -> arbitrary upto the hypervisor to decide.
   
   Here you invoke libxl_domain_shutdown() and raise error
   if this fails. This is a valid implementation, though
   I'd suggest you could choose to try libxl_send_trigger
   too in this scenario

   The QEMU driver considers flags == 0, to be equivalent to
   the union of all flags it supports. ie it effectivel changes
   flags==0 to be flags = (ACPI_POWER_BTN | GUEST_AGENT)

 - flags & VIR_DOMAIN_SHUTDOWN_NNNN -> exclusively try the
   choice specified by the user. 

   When VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is passed, you
   are still trying libxl_domain_shutdown() before you try
   to run libxl_send_trigger(). IMHO this is a bug - it
   should try libxl_send_trigger exclusively if that was
   the only bit that was specified in the flags.

IIUC  libxl_domain_shutdown() will use the Xen paravirt channel
to trigger a controlled shutdown in the guest. Currently we have
the following flags defined

    VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,        /* hypervisor choice */
    VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */
    VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
    VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
    VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */

None of those really map to the Xen paravirt shutdown
so I think we should define a new flag in the API.

    VIR_DOMAIN_SHUTDOWN_PARAVIRT = (1 << 4), /* Use paravirt guest control */

Then I think the pseudo code makes sense

  - If flags == 0
       flags = (PARAVIRT | ACPI_POWER_BTN)

  - If flags & PARAVIRT
       libxl_domain_shutdown()
       if (ret == 0)
          goto done
       if (ret != ERROR_NOPARAVIT) {
          virRaiseError(...)
          goto cleanup;
       }

  - If flags & ACPI_POWER_BTN
       libxl_send_trigger()
       if (ret == 0)
          goto done
       virRaiseError(...)
       goto cleanup;


This gives users/apps full control over which methods are
tried. In the flags==0 case it'll try as many as possible.
If neccessary the user can force it to only do ACPI power,
or only do paravirt.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag

[Ian Campbell]

On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
> IIUC  libxl_domain_shutdown() will use the Xen paravirt channel
> to trigger a controlled shutdown in the guest. Currently we have
> the following flags defined
> 
>     VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,        /* hypervisor choice */
>     VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */
>     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
>     VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
>     VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */
> 
> None of those really map to the Xen paravirt shutdown

It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if
you consider the bit of the kernel which responds to the paravirt
shutdown request as an agent of sorts. With Windows PV drivers I think
this is even literally a userspace component.

Not that having a more explicit type is a bad idea, but I thought I'd
mention it.

Ian.

Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag

[Daniel P. Berrange]

On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
> On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
> > IIUC  libxl_domain_shutdown() will use the Xen paravirt channel
> > to trigger a controlled shutdown in the guest. Currently we have
> > the following flags defined
> > 
> >     VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,        /* hypervisor choice */
> >     VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */
> >     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
> >     VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
> >     VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */
> > 
> > None of those really map to the Xen paravirt shutdown
> 
> It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if
> you consider the bit of the kernel which responds to the paravirt
> shutdown request as an agent of sorts. With Windows PV drivers I think
> this is even literally a userspace component.
> 
> Not that having a more explicit type is a bad idea, but I thought I'd
> mention it.

I guess my rationale for suggesting a new flag is that it is conceivable
that Xen could support the virtio-console devices, in which case it would
be possible for someone to configure a Xen guest with the QEMU (or another)
guest agent provided shutdown facility. In such a scenario, it'd thus be
desirable for the API to be able to distinguish Xen paravirt control from
the guest agent.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag

[Ian Campbell]

On Tue, 2014-04-29 at 11:01 +0100, Daniel P. Berrange wrote:
> On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
> > On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
> > > IIUC  libxl_domain_shutdown() will use the Xen paravirt channel
> > > to trigger a controlled shutdown in the guest. Currently we have
> > > the following flags defined
> > > 
> > >     VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,        /* hypervisor choice */
> > >     VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */
> > >     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
> > >     VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
> > >     VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */
> > > 
> > > None of those really map to the Xen paravirt shutdown
> > 
> > It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if
> > you consider the bit of the kernel which responds to the paravirt
> > shutdown request as an agent of sorts. With Windows PV drivers I think
> > this is even literally a userspace component.
> > 
> > Not that having a more explicit type is a bad idea, but I thought I'd
> > mention it.
> 
> I guess my rationale for suggesting a new flag is that it is conceivable
> that Xen could support the virtio-console devices, in which case it would
> be possible for someone to configure a Xen guest with the QEMU (or another)
> guest agent provided shutdown facility. In such a scenario, it'd thus be
> desirable for the API to be able to distinguish Xen paravirt control from
> the guest agent.

That makes sense.

Ian.

Re: [libvirt] [PATCH] libxl: support ACPI shutdown flag

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Tue, Apr 29, 2014 at 10:53:00AM +0100, Ian Campbell wrote:
>   
>> On Tue, 2014-04-29 at 10:06 +0100, Daniel P. Berrange wrote:
>>     
>>> IIUC  libxl_domain_shutdown() will use the Xen paravirt channel
>>> to trigger a controlled shutdown in the guest. Currently we have
>>> the following flags defined
>>>
>>>     VIR_DOMAIN_SHUTDOWN_DEFAULT        = 0,        /* hypervisor choice */
>>>     VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */
>>>     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
>>>     VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
>>>     VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */
>>>
>>> None of those really map to the Xen paravirt shutdown
>>>       
>> It's not a million miles away from VIR_DOMAIN_SHUTDOWN_GUEST_AGENT if
>> you consider the bit of the kernel which responds to the paravirt
>> shutdown request as an agent of sorts. With Windows PV drivers I think
>> this is even literally a userspace component.
>>
>> Not that having a more explicit type is a bad idea, but I thought I'd
>> mention it.
>>     
>
> I guess my rationale for suggesting a new flag is that it is conceivable
> that Xen could support the virtio-console devices, in which case it would
> be possible for someone to configure a Xen guest with the QEMU (or another)
> guest agent provided shutdown facility. In such a scenario, it'd thus be
> desirable for the API to be able to distinguish Xen paravirt control from
> the guest agent.
>   

Agreed.  I'll rework the patch as suggested.

Regards,
Jim