[PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Campbell]

HVM guests which do not include PV drivers will not shutdown after a
simple "xl shutdown". Add a runvar to indicate that the guest will
shutdown in response to an ACPI power event and apply this to the win7
and winxp test jobs.

Tested with:
    test-amd64-amd64-xl-qemuu-winxpsp3
    test-amd64-amd64-xl-qemuu-win7-amd64
    test-amd64-amd64-xl-qemuu-debianhvm-amd64
    test-amd64-amd64-xl

The first two correctly used the extra flag and the domains shutdown,
the other two correctly didn't use the extra flag and still shutdown.

Not currently implemented for libvirt, since we currently have no
libvirt tests I could try it with.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/xl.pm | 4 +++-
 make-flight             | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 71aefd5..dd12ae1 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -55,7 +55,9 @@ sub shutdown_wait ($$$) {
     my ($self,$gho,$timeout) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho,"$self->{_Command} shutdown -w $gn", $timeout);
+    my $acpi_fallback = guest_var($gho,'acpi_shutdown','false') eq 'true'
+	? "F" : "";
+    target_cmd_root($ho,"$self->{_Command} shutdown -w${acpi_fallback} $gn", $timeout);
 }
 
 sub migrate_check ($) {
diff --git a/make-flight b/make-flight
index 70c1900..202c68d 100755
--- a/make-flight
+++ b/make-flight
@@ -188,6 +188,7 @@ do_hvm_winxp_tests () {
             test-$xenarch$kern-$dom0arch-$toolstack$qemuu_suffix-winxpsp3$vcpus_suffix \
             test-win $toolstack $xenarch $dom0arch $qemuu_runvar \
             win_image=winxpsp3.iso $vcpus_runvars   \
+            win_acpi_shutdown=true \
             all_hostflags=$most_hostflags,hvm
 
   done
@@ -201,6 +202,7 @@ do_hvm_win7_x64_tests () {
   job_create_test test-$xenarch$kern-$dom0arch-xl$qemuu_suffix-win7-amd64 \
             test-win xl $xenarch $dom0arch $qemuu_runvar \
             win_image=win7-x64.iso \
+            win_acpi_shutdown=true \
             all_hostflags=$most_hostflags,hvm
 }
 
-- 
2.1.4

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Campbell]

On Thu, 2015-05-07 at 10:07 +0100, Ian Campbell wrote:
> HVM guests which do not include PV drivers will not shutdown after a
> simple "xl shutdown". Add a runvar to indicate that the guest will
> shutdown in response to an ACPI power event and apply this to the win7
> and winxp test jobs.
> 
> Tested with:
>     test-amd64-amd64-xl-qemuu-winxpsp3
>     test-amd64-amd64-xl-qemuu-win7-amd64
>     test-amd64-amd64-xl-qemuu-debianhvm-amd64
>     test-amd64-amd64-xl
> 
> The first two correctly used the extra flag and the domains shutdown,
> the other two correctly didn't use the extra flag and still shutdown.
> Not currently implemented for libvirt, since we currently have no
> libvirt tests I could try it with.

https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Managing_guest_virtual_machines_with_virsh-Shutting_down_rebooting_and_force_shutdown_of_a_guest_virtual_machine.html
suggests that virsh reboot can take a comma separated list for the
--mode option and will try them each in turn.

It doesn't say the same for virsh shutdown but it might be reasonable to
assume that this is a docs issue not a functionality one.

If that were the case then I think the below would do the trick, but as
I say I don't have an immediately convenient way to test, nor a need
since there are no such tests right now.

Ian.

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 8bd7f4f..e7f4860 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -60,7 +60,11 @@ sub shutdown_wait ($$$) {
     my ($self,$gho,$timeout) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho, "virsh shutdown $gn", 30);
+    my $mode = "paravirt";
+    $mode .= ",acpi"
+	if guest_var($gho,'acpi_shutdown','false') eq 'true';
+
+    target_cmd_root($ho, "virsh shutdown --mode $mode $gn", 30);
     guest_await_destroy($gho,$timeout);
 }

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
> If that were the case then I think the below would do the trick, but as
> I say I don't have an immediately convenient way to test, nor a need
> since there are no such tests right now.

This patch looks plausible.  Subject to comments from Jim I would be
tempted to throw it into osstest alongside the patch to do the same to
xl, with appropriate caveats in the commit message.

Ian.

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Campbell]

On Tue, 2015-05-12 at 16:03 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
> > If that were the case then I think the below would do the trick, but as
> > I say I don't have an immediately convenient way to test, nor a need
> > since there are no such tests right now.
> 
> This patch looks plausible.  Subject to comments from Jim I would be
> tempted to throw it into osstest alongside the patch to do the same to
> xl, with appropriate caveats in the commit message.

Shall I fold it into the xl one, or do it separately?

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Campbell]

On Tue, 2015-05-12 at 16:19 +0100, Ian Campbell wrote:
> On Tue, 2015-05-12 at 16:03 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
> > > If that were the case then I think the below would do the trick, but as
> > > I say I don't have an immediately convenient way to test, nor a need
> > > since there are no such tests right now.
> > 
> > This patch looks plausible.  Subject to comments from Jim I would be
> > tempted to throw it into osstest alongside the patch to do the same to
> > xl, with appropriate caveats in the commit message.
> 
> Shall I fold it into the xl one, or do it separately?

You acked the xl one, so here is a separate one.

Jim, does this look at all plausible to you?

Ian.

>From f560b7eed781a93567d69bcf90260c7475854766 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Thu, 7 May 2015 16:37:18 +0100
Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown

This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
fallback for shutdown". Currently there are no jobs which test HVM
guests with libvirt and so this is completely untested (but at least
should be harmless to current jobs).

This relies on an assumption that "virsh shutdown" behaves the same as
"virsh reboot" and accepts a comma separated list of methods to try
given to the --mode argument.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 8bd7f4f..e7f4860 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -60,7 +60,11 @@ sub shutdown_wait ($$$) {
     my ($self,$gho,$timeout) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho, "virsh shutdown $gn", 30);
+    my $mode = "paravirt";
+    $mode .= ",acpi"
+	if guest_var($gho,'acpi_shutdown','false') eq 'true';
+
+    target_cmd_root($ho, "virsh shutdown --mode $mode $gn", 30);
     guest_await_destroy($gho,$timeout);
 }
 
-- 
2.1.4

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2015-05-12 at 16:19 +0100, Ian Campbell wrote:
>   
>> On Tue, 2015-05-12 at 16:03 +0100, Ian Jackson wrote:
>>     
>>> Ian Campbell writes ("Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
>>>       
>>>> If that were the case then I think the below would do the trick, but as
>>>> I say I don't have an immediately convenient way to test, nor a need
>>>> since there are no such tests right now.
>>>>         
>>> This patch looks plausible.  Subject to comments from Jim I would be
>>> tempted to throw it into osstest alongside the patch to do the same to
>>> xl, with appropriate caveats in the commit message.
>>>       
>> Shall I fold it into the xl one, or do it separately?
>>     
>
> You acked the xl one, so here is a separate one.
>
> Jim, does this look at all plausible to you?
>   

Yes, looks good.

> Ian.
>
> From f560b7eed781a93567d69bcf90260c7475854766 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 7 May 2015 16:37:18 +0100
> Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown
>
> This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
> fallback for shutdown". Currently there are no jobs which test HVM
> guests with libvirt and so this is completely untested (but at least
> should be harmless to current jobs).
>
> This relies on an assumption that "virsh shutdown" behaves the same as
> "virsh reboot" and accepts a comma separated list of methods to try
> given to the --mode argument.
>   

That assumption would be correct, and I just verified it is working as
expected.

Regards,
Jim

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jim Fehlig <jfehlig@suse.com>
> ---
>  Osstest/Toolstack/libvirt.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
> index 8bd7f4f..e7f4860 100644
> --- a/Osstest/Toolstack/libvirt.pm
> +++ b/Osstest/Toolstack/libvirt.pm
> @@ -60,7 +60,11 @@ sub shutdown_wait ($$$) {
>      my ($self,$gho,$timeout) = @_;
>      my $ho = $self->{Host};
>      my $gn = $gho->{Name};
> -    target_cmd_root($ho, "virsh shutdown $gn", 30);
> +    my $mode = "paravirt";
> +    $mode .= ",acpi"
> +	if guest_var($gho,'acpi_shutdown','false') eq 'true';
> +
> +    target_cmd_root($ho, "virsh shutdown --mode $mode $gn", 30);
>      guest_await_destroy($gho,$timeout);
>  }
>  
>   

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Campbell]

On Tue, 2015-05-12 at 14:36 -0600, Jim Fehlig wrote:
> > Ian.
> >
> > From f560b7eed781a93567d69bcf90260c7475854766 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Thu, 7 May 2015 16:37:18 +0100
> > Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown
> >
> > This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
> > fallback for shutdown". Currently there are no jobs which test HVM
> > guests with libvirt and so this is completely untested (but at least
> > should be harmless to current jobs).
> >
> > This relies on an assumption that "virsh shutdown" behaves the same as
> > "virsh reboot" and accepts a comma separated list of methods to try
> > given to the --mode argument.
> >   
> 
> That assumption would be correct, and I just verified it is working as
> expected.

Thanks. I updated the commit message to reflect this feedback. Can I add
your ack to?

>From 682b446b1ee30e6b26b7138805bdd0ca89d1942d Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Thu, 7 May 2015 16:37:18 +0100
Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown

This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
fallback for shutdown". Currently there are no jobs which test HVM
guests with libvirt and so this is completely untested in the context
of osstest (but at least should be harmless to current jobs).

This relies on an assumption that "virsh shutdown" behaves the same as
"virsh reboot" and accepts a comma separated list of methods to try
given to the --mode argument, which Jim has tested and confirmed to be
true.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 8bd7f4f..e7f4860 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -60,7 +60,11 @@ sub shutdown_wait ($$$) {
     my ($self,$gho,$timeout) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho, "virsh shutdown $gn", 30);
+    my $mode = "paravirt";
+    $mode .= ",acpi"
+	if guest_var($gho,'acpi_shutdown','false') eq 'true';
+
+    target_cmd_root($ho, "virsh shutdown --mode $mode $gn", 30);
     guest_await_destroy($gho,$timeout);
 }
 
-- 
2.1.4

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2015-05-12 at 14:36 -0600, Jim Fehlig wrote:
>   
>>> Ian.
>>>
>>> From f560b7eed781a93567d69bcf90260c7475854766 Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@citrix.com>
>>> Date: Thu, 7 May 2015 16:37:18 +0100
>>> Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown
>>>
>>> This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
>>> fallback for shutdown". Currently there are no jobs which test HVM
>>> guests with libvirt and so this is completely untested (but at least
>>> should be harmless to current jobs).
>>>
>>> This relies on an assumption that "virsh shutdown" behaves the same as
>>> "virsh reboot" and accepts a comma separated list of methods to try
>>> given to the --mode argument.
>>>   
>>>       
>> That assumption would be correct, and I just verified it is working as
>> expected.
>>     
>
> Thanks. I updated the commit message to reflect this feedback. Can I add
> your ack to?
>   

Yes.

> From 682b446b1ee30e6b26b7138805bdd0ca89d1942d Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 7 May 2015 16:37:18 +0100
> Subject: [PATCH] Toolstack::libvirt: Support for ACPI fallback for shutdown
>
> This is the libvirt counterpart to "Toolstack::xl: Support for ACPI
> fallback for shutdown". Currently there are no jobs which test HVM
> guests with libvirt and so this is completely untested in the context
> of osstest (but at least should be harmless to current jobs).
>
> This relies on an assumption that "virsh shutdown" behaves the same as
> "virsh reboot" and accepts a comma separated list of methods to try
> given to the --mode argument, which Jim has tested and confirmed to be
> true.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jim Fehlig <jfehlig@suse.com>
> ---
>  Osstest/Toolstack/libvirt.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
> index 8bd7f4f..e7f4860 100644
> --- a/Osstest/Toolstack/libvirt.pm
> +++ b/Osstest/Toolstack/libvirt.pm
> @@ -60,7 +60,11 @@ sub shutdown_wait ($$$) {
>      my ($self,$gho,$timeout) = @_;
>      my $ho = $self->{Host};
>      my $gn = $gho->{Name};
> -    target_cmd_root($ho, "virsh shutdown $gn", 30);
> +    my $mode = "paravirt";
> +    $mode .= ",acpi"
> +	if guest_var($gho,'acpi_shutdown','false') eq 'true';
> +
> +    target_cmd_root($ho, "virsh shutdown --mode $mode $gn", 30);
>      guest_await_destroy($gho,$timeout);
>  }
>   

Acked by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
> You acked the xl one, so here is a separate one.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

Re: [PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST] Toolstack::xl: Support for ACPI fallback for shutdown"):
> HVM guests which do not include PV drivers will not shutdown after a
> simple "xl shutdown". Add a runvar to indicate that the guest will
> shutdown in response to an ACPI power event and apply this to the win7
> and winxp test jobs.
> 
> Tested with:
>     test-amd64-amd64-xl-qemuu-winxpsp3
>     test-amd64-amd64-xl-qemuu-win7-amd64
>     test-amd64-amd64-xl-qemuu-debianhvm-amd64
>     test-amd64-amd64-xl
> 
> The first two correctly used the extra flag and the domains shutdown,
> the other two correctly didn't use the extra flag and still shutdown.
> 
> Not currently implemented for libvirt, since we currently have no
> libvirt tests I could try it with.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>