All of lore.kernel.org
 help / color / mirror / Atom feed
* [OSSTEST PATCH 0/2] libvirt: Fix save/restore capability check on ARM
@ 2016-10-04 17:02 Ian Jackson
  2016-10-04 17:02 ` [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser Ian Jackson
  2016-10-04 17:02 ` [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised Ian Jackson
  0 siblings, 2 replies; 15+ messages in thread
From: Ian Jackson @ 2016-10-04 17:02 UTC (permalink / raw)
  To: xen-devel; +Cc: libvir-list, Julien Grall, Jim Fehlig

Currently, osstest, the Xen Project's automated test framework,
erroneously thinks that save/restore is supported with libvirt on ARM.
In fact, save/restore is not supported by Xen on ARM at all.

The result is that osstest then actually attempts the save/restore,
and abandons the test job as a failure.  This is not desirable.

In these two patches I try to fix the feature detection to get this
right.  I'd appreciate advice about whether I have done the right
thing.

My code is based partly on empirical observation of the output of
`virsh capabilities' on x86 and ARM.  (See below.)

Thanks,
Ian.


From baroque0, x86.
As left by 101253.test-amd64-amd64-libvirt-pair; osstest "branch" osstest


   <capabilities>

     <host>
       <cpu>
	 <arch>x86_64</arch>
	 <features>
	   <pae/>
	 </features>
	 <model>Haswell-noTSX</model>
	 <topology sockets='1' cores='4' threads='2'/>
	 <feature name='vme'/>
	 <feature name='ds'/>
	 <feature name='acpi'/>
	 <feature name='ht'/>
	 <feature name='tm'/>
	 <feature name='pbe'/>
	 <feature name='dtes64'/>
	 <feature name='monitor'/>
	 <feature name='ds_cpl'/>
	 <feature name='vmx'/>
	 <feature name='smx'/>
	 <feature name='est'/>
	 <feature name='tm2'/>
	 <feature name='xtpr'/>
	 <feature name='pdcm'/>
	 <feature name='f16c'/>
	 <feature name='rdrand'/>
	 <feature name='tsc_adjust'/>
	 <feature name='xsaveopt'/>
	 <feature name='pdpe1gb'/>
	 <feature name='abm'/>
	 <feature name='invtsc'/>
       </cpu>
       <power_management/>
       <migration_features>
	 <live/>
       </migration_features>
       <netprefix>vif</netprefix>
       <topology>
	 <cells num='1'>
	   <cell id='0'>
	     <memory unit='KiB'>9699328</memory>
	     <cpus num='8'>
	       <cpu id='0' socket_id='0' core_id='0' siblings='0-1'/>
	       <cpu id='1' socket_id='0' core_id='0' siblings='0-1'/>
	       <cpu id='2' socket_id='0' core_id='1' siblings='2-3'/>
	       <cpu id='3' socket_id='0' core_id='1' siblings='2-3'/>
	       <cpu id='4' socket_id='0' core_id='2' siblings='4-5'/>
	       <cpu id='5' socket_id='0' core_id='2' siblings='4-5'/>
	       <cpu id='6' socket_id='0' core_id='3' siblings='6-7'/>
	       <cpu id='7' socket_id='0' core_id='3' siblings='6-7'/>
	     </cpus>
	   </cell>
	 </cells>
       </topology>
     </host>

     <guest>
       <os_type>xen</os_type>
       <arch name='x86_64'>
	 <wordsize>64</wordsize>
	 <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator>
	 <machine>xenpv</machine>
	 <domain type='xen'/>
       </arch>
     </guest>

     <guest>
       <os_type>xen</os_type>
       <arch name='i686'>
	 <wordsize>32</wordsize>
	 <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator>
	 <machine>xenpv</machine>
	 <domain type='xen'/>
       </arch>
       <features>
	 <pae/>
       </features>
     </guest>

     <guest>
       <os_type>hvm</os_type>
       <arch name='i686'>
	 <wordsize>32</wordsize>
	 <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator>
	 <loader>/usr/lib/xen/boot/hvmloader</loader>
	 <machine>xenfv</machine>
	 <domain type='xen'/>
       </arch>
       <features>
	 <pae/>
	 <nonpae/>
	 <acpi default='on' toggle='yes'/>
	 <apic default='on' toggle='no'/>
	 <hap default='on' toggle='yes'/>
       </features>
     </guest>

     <guest>
       <os_type>hvm</os_type>
       <arch name='x86_64'>
	 <wordsize>64</wordsize>
	 <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator>
	 <loader>/usr/lib/xen/boot/hvmloader</loader>
	 <machine>xenfv</machine>
	 <domain type='xen'/>
       </arch>
       <features>
	 <acpi default='on' toggle='yes'/>
	 <apic default='on' toggle='no'/>
	 <hap default='on' toggle='yes'/>
       </features>
     </guest>

   </capabilities>



From arndale-lakeside, ARM.
As left by 101251 | test-armhf-armhf-libvirt; osstest "branch" qemu-mainline

   <capabilities>

     <host>
       <cpu>
	 <arch>armv7l</arch>
       </cpu>
       <power_management/>
       <netprefix>vif</netprefix>
       <topology>
	 <cells num='1'>
	   <cell id='0'>
	     <memory unit='KiB'>2097152</memory>
	     <cpus num='2'>
	       <cpu id='0' socket_id='0' core_id='0' siblings='0-1'/>
	       <cpu id='1' socket_id='0' core_id='0' siblings='0-1'/>
	     </cpus>
	   </cell>
	 </cells>
       </topology>
     </host>

     <guest>
       <os_type>xen</os_type>
       <arch name='armv7l'>
	 <wordsize>32</wordsize>
	 <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator>
	 <machine>xenpv</machine>
	 <domain type='xen'/>
       </arch>
     </guest>

   </capabilities>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-04 17:02 [OSSTEST PATCH 0/2] libvirt: Fix save/restore capability check on ARM Ian Jackson
@ 2016-10-04 17:02 ` Ian Jackson
  2016-10-04 17:05   ` Ian Jackson
  2016-10-05 23:50   ` Jim Fehlig
  2016-10-04 17:02 ` [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised Ian Jackson
  1 sibling, 2 replies; 15+ messages in thread
From: Ian Jackson @ 2016-10-04 17:02 UTC (permalink / raw)
  To: xen-devel; +Cc: libvir-list, Julien Grall, Ian Jackson, Jim Fehlig

Do not grep the virsh capabilities output (!)  Instead, parse the XML
using perl's XML modules and look for the specific feature flag using
an XPATH pattern.

AFAICT from looking at the XML, that's

    <capabilities>
      <host>
	<migration_features>
	  <live/>

But the original code does not test for <live/>.

Xen could in principle (and AIUI might in the future, on ARM) support
save/restore but not live migration.  Currently it supports neither.

I don't know whether libvirt's capabilities system can capture this
distinction.  libvirt.git#1d37a4c4 "libxl: detect support for save and
restore" suggests not.

Perhaps relatedly, I am not sure whether this test should be changed
to look for the xpath
  /capabilities/host/migration_features/live
instead.  The schema (libvirt.git/docs/schemas/capability.rng) seems
to suggest that it probably should.

For now, this osstest commit has no ultimate functional change (with
libvirt output as it currently appears to be on real hosts).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 69ff0bb..b7db7af 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -21,6 +21,8 @@ use strict;
 use warnings;
 
 use Osstest::TestSupport;
+use XML::LibXML::XPathContext;
+use XML::LibXML;
 
 sub new {
     my ($class, $ho, $methname,$asset) = @_;
@@ -72,6 +74,17 @@ sub shutdown_wait ($$$) {
     guest_await_destroy($gho,$timeout);
 }
 
+sub _check_capability ($$) {
+    my ($self, $xpath) = @_;
+    my $ho = $self->{Host};
+    my $caps = target_cmd_output_root($ho, 'virsh capabilities');
+    my $stash = open_unique_stashfile('virsh-capabilities');
+    my $dom = XML::LibXML->load_xml(string => $caps);
+    my $xc = XML::LibXML::XPathContext->new($dom);
+    my @nodes = $xc->findnodes($xpath);
+    return !!@nodes;
+}
+
 sub migrate_check ($$) {
     my ($self, $local) = @_;
     my $rc;
@@ -80,9 +93,7 @@ sub migrate_check ($$) {
         # local migration is not supported
         $rc = 1;
     } else {
-	my $ho = $self->{Host};
-	my $caps = target_cmd_output_root($ho, "virsh capabilities");
-	$rc = ($caps =~ m/<migration_features>/) ? 0 : 1
+	$rc = $self->check_capability('/capabilities/host/migration_features');
     }
 
     logm("rc=$rc");
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-04 17:02 [OSSTEST PATCH 0/2] libvirt: Fix save/restore capability check on ARM Ian Jackson
  2016-10-04 17:02 ` [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser Ian Jackson
@ 2016-10-04 17:02 ` Ian Jackson
  2016-10-05  6:46   ` [libvirt] " Martin Kletzander
  2016-10-06  0:06   ` Jim Fehlig
  1 sibling, 2 replies; 15+ messages in thread
From: Ian Jackson @ 2016-10-04 17:02 UTC (permalink / raw)
  To: xen-devel; +Cc: libvir-list, Julien Grall, Ian Jackson, Jim Fehlig

Currently, osstest wrongly thinks that ARM can do save/restore,
because `virsh help' does mention the save command (on all
architectures).

Additionally, check the virth capabilities xpath
   /capabilities/host/migration_features
to try to see whether this host supports migration.

I am not sure if this is the right path to check.  Perhaps
   /capabilities/host/migration_features/live
is more correct, but this may be wrong if Xen comes to support save/restore
on ARM, but not live migration (but perhaps libvirt cannot express this
distinction in which case perhaps it's right after all).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index b7db7af..250fe47 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -111,7 +111,9 @@ sub check_for_command($$) {
 
 sub saverestore_check ($) {
     my ($self) = @_;
-    return check_for_command($self, "save");
+    return
+	_check_capability($self, '/capabilities/host/migration_features') &&
+	check_for_command($self, "save");
 }
 
 sub migrate ($$$$) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-04 17:02 ` [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser Ian Jackson
@ 2016-10-04 17:05   ` Ian Jackson
  2016-10-05 18:36     ` Julien Grall
  2016-10-05 23:50   ` Jim Fehlig
  1 sibling, 1 reply; 15+ messages in thread
From: Ian Jackson @ 2016-10-04 17:05 UTC (permalink / raw)
  To: xen-devel, Julien Grall, Jim Fehlig, libvir-list

Ian Jackson writes ("[OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser"):
> Do not grep the virsh capabilities output (!)  Instead, parse the XML
> using perl's XML modules and look for the specific feature flag using
> an XPATH pattern.
...>  
> +sub _check_capability ($$) {
...
> +	$rc = $self->check_capability('/capabilities/host/migration_features');
                     ^

Missing _.  I didn't test this again before sending it.  I'd still
like a review from libvirt (and Xen/ARM) folks.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [libvirt] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-04 17:02 ` [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised Ian Jackson
@ 2016-10-05  6:46   ` Martin Kletzander
  2016-10-06  0:06   ` Jim Fehlig
  1 sibling, 0 replies; 15+ messages in thread
From: Martin Kletzander @ 2016-10-05  6:46 UTC (permalink / raw)
  To: Ian Jackson; +Cc: libvir-list, xen-devel, Julien Grall


[-- Attachment #1.1: Type: text/plain, Size: 2074 bytes --]

On Tue, Oct 04, 2016 at 06:02:27PM +0100, Ian Jackson wrote:
>Currently, osstest wrongly thinks that ARM can do save/restore,
>because `virsh help' does mention the save command (on all
>architectures).
>
>Additionally, check the virth capabilities xpath
>   /capabilities/host/migration_features
>to try to see whether this host supports migration.
>

I think this is pretty accurate.  At least for now.  I can't test the
code, but it looks fine.  Anyway, to stay in the safe waters, I'll just
comment the libvirt part ;)

>I am not sure if this is the right path to check.  Perhaps
>   /capabilities/host/migration_features/live
>is more correct, but this may be wrong if Xen comes to support save/restore
>on ARM, but not live migration (but perhaps libvirt cannot express this
>distinction in which case perhaps it's right after all).
>

I think it does not matter for now.  If you add support for live
migrations, there will be both elements present in the XML, so whatever
you use you will face the same problem.  We should add a capability for
save/restore so that hypervisors, for which it's different thing than
migration, can distinguish that.  Maybe in the future we'll need to add
this per-guest, but I don't see the point right now.

>Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>CC: Julien Grall <julien.grall@arm.com>
>CC: Jim Fehlig <jfehlig@suse.com>
>---
> Osstest/Toolstack/libvirt.pm | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
>index b7db7af..250fe47 100644
>--- a/Osstest/Toolstack/libvirt.pm
>+++ b/Osstest/Toolstack/libvirt.pm
>@@ -111,7 +111,9 @@ sub check_for_command($$) {
>
> sub saverestore_check ($) {
>     my ($self) = @_;
>-    return check_for_command($self, "save");
>+    return
>+	_check_capability($self, '/capabilities/host/migration_features') &&
>+	check_for_command($self, "save");
> }
>
> sub migrate ($$$$) {
>--
>2.1.4
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-04 17:05   ` Ian Jackson
@ 2016-10-05 18:36     ` Julien Grall
  2016-10-06 10:00       ` Ian Jackson
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2016-10-05 18:36 UTC (permalink / raw)
  To: Ian Jackson, xen-devel, Jim Fehlig, libvir-list

Hi Ian,

On 04/10/2016 10:05, Ian Jackson wrote:
> Ian Jackson writes ("[OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser"):
>> Do not grep the virsh capabilities output (!)  Instead, parse the XML
>> using perl's XML modules and look for the specific feature flag using
>> an XPATH pattern.
> ...>
>> +sub _check_capability ($$) {
> ...
>> +	$rc = $self->check_capability('/capabilities/host/migration_features');
>                      ^
>
> Missing _.  I didn't test this again before sending it.  I'd still
> like a review from libvirt (and Xen/ARM) folks.

I am not sure what kind of input you would need from Xen/ARM. This patch 
set looks very libvirt specific.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-04 17:02 ` [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser Ian Jackson
  2016-10-04 17:05   ` Ian Jackson
@ 2016-10-05 23:50   ` Jim Fehlig
  1 sibling, 0 replies; 15+ messages in thread
From: Jim Fehlig @ 2016-10-05 23:50 UTC (permalink / raw)
  To: Ian Jackson, xen-devel; +Cc: libvir-list, Julien Grall

On 10/04/2016 11:02 AM, Ian Jackson wrote:
> Do not grep the virsh capabilities output (!)  Instead, parse the XML
> using perl's XML modules and look for the specific feature flag using
> an XPATH pattern.
>
> AFAICT from looking at the XML, that's
>
>     <capabilities>
>       <host>
> 	<migration_features>
> 	  <live/>
>
> But the original code does not test for <live/>.
>
> Xen could in principle (and AIUI might in the future, on ARM) support
> save/restore but not live migration.  Currently it supports neither.
>
> I don't know whether libvirt's capabilities system can capture this
> distinction.  libvirt.git#1d37a4c4 "libxl: detect support for save and
> restore" suggests not.

That's correct. But as Martin suggested in 2/2, libvirt could be enhanced to 
report the distinction if/when needed.

>
> Perhaps relatedly, I am not sure whether this test should be changed
> to look for the xpath
>   /capabilities/host/migration_features/live
> instead.  The schema (libvirt.git/docs/schemas/capability.rng) seems
> to suggest that it probably should.

Agreed. migrate_check() should look for
/capabilities/host/migration_features/live.

Regards,
Jim

>
> For now, this osstest commit has no ultimate functional change (with
> libvirt output as it currently appears to be on real hosts).
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> ---
>  Osstest/Toolstack/libvirt.pm | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
> index 69ff0bb..b7db7af 100644
> --- a/Osstest/Toolstack/libvirt.pm
> +++ b/Osstest/Toolstack/libvirt.pm
> @@ -21,6 +21,8 @@ use strict;
>  use warnings;
>
>  use Osstest::TestSupport;
> +use XML::LibXML::XPathContext;
> +use XML::LibXML;
>
>  sub new {
>      my ($class, $ho, $methname,$asset) = @_;
> @@ -72,6 +74,17 @@ sub shutdown_wait ($$$) {
>      guest_await_destroy($gho,$timeout);
>  }
>
> +sub _check_capability ($$) {
> +    my ($self, $xpath) = @_;
> +    my $ho = $self->{Host};
> +    my $caps = target_cmd_output_root($ho, 'virsh capabilities');
> +    my $stash = open_unique_stashfile('virsh-capabilities');
> +    my $dom = XML::LibXML->load_xml(string => $caps);
> +    my $xc = XML::LibXML::XPathContext->new($dom);
> +    my @nodes = $xc->findnodes($xpath);
> +    return !!@nodes;
> +}
> +
>  sub migrate_check ($$) {
>      my ($self, $local) = @_;
>      my $rc;
> @@ -80,9 +93,7 @@ sub migrate_check ($$) {
>          # local migration is not supported
>          $rc = 1;
>      } else {
> -	my $ho = $self->{Host};
> -	my $caps = target_cmd_output_root($ho, "virsh capabilities");
> -	$rc = ($caps =~ m/<migration_features>/) ? 0 : 1
> +	$rc = $self->check_capability('/capabilities/host/migration_features');
>      }
>
>      logm("rc=$rc");
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-04 17:02 ` [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised Ian Jackson
  2016-10-05  6:46   ` [libvirt] " Martin Kletzander
@ 2016-10-06  0:06   ` Jim Fehlig
  2016-10-06  9:43     ` Martin Kletzander
  1 sibling, 1 reply; 15+ messages in thread
From: Jim Fehlig @ 2016-10-06  0:06 UTC (permalink / raw)
  To: Ian Jackson, xen-devel; +Cc: libvir-list, Martin Kletzander, Julien Grall

On 10/04/2016 11:02 AM, Ian Jackson wrote:
> Currently, osstest wrongly thinks that ARM can do save/restore,
> because `virsh help' does mention the save command (on all
> architectures).
>
> Additionally, check the virth capabilities xpath
>    /capabilities/host/migration_features
> to try to see whether this host supports migration.
>
> I am not sure if this is the right path to check.  Perhaps
>    /capabilities/host/migration_features/live
> is more correct, but this may be wrong if Xen comes to support save/restore
> on ARM, but not live migration (but perhaps libvirt cannot express this
> distinction in which case perhaps it's right after all).

Looking at the capabilities generation code again, I see that 
virCapabilitiesNew() takes 'offlineMigrate' and 'liveMigrate' parameters. I 
assume offline in this context means save, copy, restore. Martin, is that 
assumption correct?

If so, I think the saverestore_check() below can look for
/capabilities/host/migration_features. The migration check in 1/2 can look for
/capabilities/host/migration_features/live. Is it fair to assume save/restore is 
available when live migration is supported?

Regards,
Jim

>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Jim Fehlig <jfehlig@suse.com>
> ---
>  Osstest/Toolstack/libvirt.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
> index b7db7af..250fe47 100644
> --- a/Osstest/Toolstack/libvirt.pm
> +++ b/Osstest/Toolstack/libvirt.pm
> @@ -111,7 +111,9 @@ sub check_for_command($$) {
>
>  sub saverestore_check ($) {
>      my ($self) = @_;
> -    return check_for_command($self, "save");
> +    return
> +	_check_capability($self, '/capabilities/host/migration_features') &&
> +	check_for_command($self, "save");
>  }
>
>  sub migrate ($$$$) {
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-06  0:06   ` Jim Fehlig
@ 2016-10-06  9:43     ` Martin Kletzander
  2016-10-06  9:59       ` Ian Jackson
  0 siblings, 1 reply; 15+ messages in thread
From: Martin Kletzander @ 2016-10-06  9:43 UTC (permalink / raw)
  To: Jim Fehlig; +Cc: libvir-list, xen-devel, Julien Grall, Ian Jackson


[-- Attachment #1.1: Type: text/plain, Size: 2180 bytes --]

On Wed, Oct 05, 2016 at 06:06:29PM -0600, Jim Fehlig wrote:
>On 10/04/2016 11:02 AM, Ian Jackson wrote:
>> Currently, osstest wrongly thinks that ARM can do save/restore,
>> because `virsh help' does mention the save command (on all
>> architectures).
>>
>> Additionally, check the virth capabilities xpath
>>    /capabilities/host/migration_features
>> to try to see whether this host supports migration.
>>
>> I am not sure if this is the right path to check.  Perhaps
>>    /capabilities/host/migration_features/live
>> is more correct, but this may be wrong if Xen comes to support save/restore
>> on ARM, but not live migration (but perhaps libvirt cannot express this
>> distinction in which case perhaps it's right after all).
>
>Looking at the capabilities generation code again, I see that
>virCapabilitiesNew() takes 'offlineMigrate' and 'liveMigrate' parameters. I
>assume offline in this context means save, copy, restore. Martin, is that
>assumption correct?
>

The thing is that it's not documented.  I can't even say "enough", it's
more like "at all".  You can have a look at it:

  https://libvirt.org/formatcaps.html

It doesn't even talk about <migration_features/>, just <migration/>, but
I guess that's the same thing.

Since offline migration (as in migrating a domain between hosts without
being running) is not that used in the code and talked about, I'm
guessing offline means save restore.  Looking at the history it was
added before the "offline" migration, so it probably means
save/restore.  To avoid confusion, I would suggest we add either
<offline/> or rather <save/> (the naming is not important) and document
what it means.  And then you can use it exactly how you'd like.  And
you'll be also sure it means what you need it to mean ;)  The patches
will be straigh-forward, let me know if I can help anyhow.

>If so, I think the saverestore_check() below can look for
>/capabilities/host/migration_features. The migration check in 1/2 can look for
>/capabilities/host/migration_features/live. Is it fair to assume save/restore is
>available when live migration is supported?
>

With that you could straight check for <save/> and <live/> ;)

Martin

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-06  9:43     ` Martin Kletzander
@ 2016-10-06  9:59       ` Ian Jackson
  2016-10-06 10:42         ` Martin Kletzander
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Jackson @ 2016-10-06  9:59 UTC (permalink / raw)
  To: Martin Kletzander; +Cc: libvir-list, xen-devel, Jim Fehlig, Julien Grall

Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised"):
> Since offline migration (as in migrating a domain between hosts without
> being running) is not that used in the code and talked about, I'm
> guessing offline means save restore.  Looking at the history it was
> added before the "offline" migration, so it probably means
> save/restore.  To avoid confusion, I would suggest we add either
> <offline/> or rather <save/> (the naming is not important) and document
> what it means.  And then you can use it exactly how you'd like.  And
> you'll be also sure it means what you need it to mean ;)  The patches
> will be straigh-forward, let me know if I can help anyhow.

Except that the point of the exercise is to detect which features are
supported in which versions.  Whatever I do in osstest needs to work
with older libvirt versions, which do not report
  /capabilities/host/migration_features/save
even on x86, where it is supported.  I suppose I could detect
  /capabilities/host/migration_features/live
and assume that save/restore was supported (since it's unlikely that
live migration would be supported but not save/restore).

So for now I think I need to use
  /capabilities/host/migration_features
as a proxy for save/restore ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-05 18:36     ` Julien Grall
@ 2016-10-06 10:00       ` Ian Jackson
  2016-10-11 13:42         ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Jackson @ 2016-10-06 10:00 UTC (permalink / raw)
  To: Julien Grall; +Cc: libvir-list, xen-devel, Jim Fehlig

Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser"):
> On 04/10/2016 10:05, Ian Jackson wrote:
> > Missing _.  I didn't test this again before sending it.  I'd still
> > like a review from libvirt (and Xen/ARM) folks.
> 
> I am not sure what kind of input you would need from Xen/ARM. This patch 
> set looks very libvirt specific.

It is.  Sorry to spam you with the whole thread.  The only thing I
need from Xen/ARM people is a sanity check that my understanding of
the current and likely future supported features is correct.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-06  9:59       ` Ian Jackson
@ 2016-10-06 10:42         ` Martin Kletzander
  2016-10-06 16:44           ` Ian Jackson
  0 siblings, 1 reply; 15+ messages in thread
From: Martin Kletzander @ 2016-10-06 10:42 UTC (permalink / raw)
  To: Ian Jackson; +Cc: libvir-list, xen-devel, Jim Fehlig, Julien Grall


[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]

On Thu, Oct 06, 2016 at 10:59:06AM +0100, Ian Jackson wrote:
>Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised"):
>> Since offline migration (as in migrating a domain between hosts without
>> being running) is not that used in the code and talked about, I'm
>> guessing offline means save restore.  Looking at the history it was
>> added before the "offline" migration, so it probably means
>> save/restore.  To avoid confusion, I would suggest we add either
>> <offline/> or rather <save/> (the naming is not important) and document
>> what it means.  And then you can use it exactly how you'd like.  And
>> you'll be also sure it means what you need it to mean ;)  The patches
>> will be straigh-forward, let me know if I can help anyhow.
>
>Except that the point of the exercise is to detect which features are
>supported in which versions.  Whatever I do in osstest needs to work
>with older libvirt versions, which do not report
>  /capabilities/host/migration_features/save
>even on x86, where it is supported.  I suppose I could detect
>  /capabilities/host/migration_features/live
>and assume that save/restore was supported (since it's unlikely that
>live migration would be supported but not save/restore).
>
>So for now I think I need to use
>  /capabilities/host/migration_features
>as a proxy for save/restore ?
>

Well then, unfortunately you do.

Also, looking at how the code is structured, if you have live migration
but don't have save/restore, you won't have <migration_features/> there
at all.

>Ian.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised
  2016-10-06 10:42         ` Martin Kletzander
@ 2016-10-06 16:44           ` Ian Jackson
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Jackson @ 2016-10-06 16:44 UTC (permalink / raw)
  To: Martin Kletzander; +Cc: libvir-list, xen-devel, Jim Fehlig, Julien Grall

Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised"):
> Well then, unfortunately you do.
> 
> Also, looking at how the code is structured, if you have live migration
> but don't have save/restore, you won't have <migration_features/> there
> at all.

Right.  OK, thanks.  I will add the patch below to my osstest queue.

Ian.

From 5330ff9222e4e611505149945eef7dc074b4f9b5 Mon Sep 17 00:00:00 2001
In-Reply-To: <20161006104255.GP16414@wheatley>
References: <20161006104255.GP16414@wheatley>
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 6 Oct 2016 17:38:29 +0100
Subject: [OSSTEST PATCH 3/2] libvirt: Check /capabilities/host/migration_features/live for live migration
Cc: libvir-list@redhat.com

libvirt is capable of advertising this separately from
/capabilities/host/migration_features, so if save/restore is supported
but live migration is not, this will do the right thing.

We would have preferred libvirt to advertise
  /capabilities/host/migration_features/save
or something, but it doesn't right now, so we continue to use
  /capabilities/host/migration_features
to detect save/restore support.

If libvirt changes its feature presentation, then at some future point
we should change osstest too.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Martin Kletzander <mkletzan@redhat.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 Osstest/Toolstack/libvirt.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 250fe47..81e724d 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -93,7 +93,8 @@ sub migrate_check ($$) {
         # local migration is not supported
         $rc = 1;
     } else {
-	$rc = $self->check_capability('/capabilities/host/migration_features');
+	$rc = $self->check_capability
+	    ('/capabilities/host/migration_features/live');
     }
 
     logm("rc=$rc");
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-06 10:00       ` Ian Jackson
@ 2016-10-11 13:42         ` Julien Grall
  2016-10-11 13:50           ` Ian Jackson
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2016-10-11 13:42 UTC (permalink / raw)
  To: Ian Jackson; +Cc: libvir-list, xen-devel, Jim Fehlig

Hi Ian,

On 06/10/16 11:00, Ian Jackson wrote:
> Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser"):
>> On 04/10/2016 10:05, Ian Jackson wrote:
>>> Missing _.  I didn't test this again before sending it.  I'd still
>>> like a review from libvirt (and Xen/ARM) folks.
>>
>> I am not sure what kind of input you would need from Xen/ARM. This patch
>> set looks very libvirt specific.
>
> It is.  Sorry to spam you with the whole thread.  The only thing I
> need from Xen/ARM people is a sanity check that my understanding of
> the current and likely future supported features is correct.

live migration is not currently the supported but will be in the future. 
FWIW, there is a patch series on xen-devel to support dead migration 
(first step for live migration) [1].

I hope this answer to your question.

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2015-12/msg01053.html


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser
  2016-10-11 13:42         ` Julien Grall
@ 2016-10-11 13:50           ` Ian Jackson
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Jackson @ 2016-10-11 13:50 UTC (permalink / raw)
  To: Julien Grall; +Cc: libvir-list, xen-devel, Jim Fehlig

Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser"):
> live migration is not currently the supported but will be in the future. 
> FWIW, there is a patch series on xen-devel to support dead migration 
> (first step for live migration) [1].
> 
> I hope this answer to your question.

Yes, thanks.

Thanks everyone, the patches (as discessed etc.) are now in the
osstest self-push-gate.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-10-11 13:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-04 17:02 [OSSTEST PATCH 0/2] libvirt: Fix save/restore capability check on ARM Ian Jackson
2016-10-04 17:02 ` [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser Ian Jackson
2016-10-04 17:05   ` Ian Jackson
2016-10-05 18:36     ` Julien Grall
2016-10-06 10:00       ` Ian Jackson
2016-10-11 13:42         ` Julien Grall
2016-10-11 13:50           ` Ian Jackson
2016-10-05 23:50   ` Jim Fehlig
2016-10-04 17:02 ` [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised Ian Jackson
2016-10-05  6:46   ` [libvirt] " Martin Kletzander
2016-10-06  0:06   ` Jim Fehlig
2016-10-06  9:43     ` Martin Kletzander
2016-10-06  9:59       ` Ian Jackson
2016-10-06 10:42         ` Martin Kletzander
2016-10-06 16:44           ` Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.