Libguestfs - Mar 2017 - Re: [PATCH v2] v2v: -i libvirt: If <vcpu> is missing, calculate it from CPU topology.

---
 v2v/parse_libvirt_xml.ml           | 18 ++++++++++++++++--
 v2v/test-v2v-print-source.expected |  2 +-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/v2v/parse_libvirt_xml.ml b/v2v/parse_libvirt_xml.ml
index 6032c31..2dee274 100644
--- a/v2v/parse_libvirt_xml.ml
+++ b/v2v/parse_libvirt_xml.ml
@@ -50,7 +50,7 @@ let parse_libvirt_xml ?conn xml    let xpathctx =
Xml.xpath_new_context doc in
   let xpath_string = xpath_string xpathctx
   and xpath_int = xpath_int xpathctx
-  and xpath_int_default = xpath_int_default xpathctx
+  (*and xpath_int_default = xpath_int_default xpathctx*)
   and xpath_int64_default = xpath_int64_default xpathctx in
 
   let hypervisor @@ -65,7 +65,6 @@ let parse_libvirt_xml ?conn xml      | Some
s -> s in
   let memory = xpath_int64_default "/domain/memory/text()" (1024L *^
1024L) in
   let memory = memory *^ 1024L in
-  let vcpu = xpath_int_default "/domain/vcpu/text()" 1 in
 
   let cpu_vendor = xpath_string "/domain/cpu/vendor/text()" in
   let cpu_model = xpath_string "/domain/cpu/model/text()" in
@@ -73,6 +72,21 @@ let parse_libvirt_xml ?conn xml    let cpu_cores = xpath_int
"/domain/cpu/topology/@cores" in
   let cpu_threads = xpath_int "/domain/cpu/topology/@threads" in
 
+  (* Get the <vcpu> field from the input XML.  If not set then
+   * try calculating it from the <cpu> <topology> node.  If
that's
+   * not set either, then assume 1 vCPU.
+   *)
+  let vcpu = xpath_int "/domain/vcpu/text()" in
+  let vcpu +    match vcpu, cpu_sockets, cpu_cores, cpu_threads with
+    | Some vcpu, _,    _,    _    -> vcpu
+    | None,      None, None, None -> 1
+    | None,      _,    _,    _    ->
+       let sockets = match cpu_sockets with None -> 1 | Some v -> v in
+       let cores = match cpu_cores with None -> 1 | Some v -> v in
+       let threads = match cpu_threads with None -> 1 | Some v -> v in
+       sockets * cores * threads in
+
   let features      let features = ref [] in
     let obj = Xml.xpath_eval_expression xpathctx "/domain/features/*"
in
diff --git a/v2v/test-v2v-print-source.expected
b/v2v/test-v2v-print-source.expected
index 6e78aad..df40ec9 100644
--- a/v2v/test-v2v-print-source.expected
+++ b/v2v/test-v2v-print-source.expected
@@ -1,7 +1,7 @@
     source name: windows
 hypervisor type: kvm
          memory: 1073741824 (bytes)
-       nr vCPUs: 1
+       nr vCPUs: 64
      CPU vendor: Intel
       CPU model: Broadwell
    CPU topology: sockets: 4 cores/socket: 8 threads/core: 2
-- 
2.10.2

On Thursday, 16 March 2017 22:10:10 CET Richard W.M. Jones
wrote:
> ---
>  v2v/parse_libvirt_xml.ml           | 18 ++++++++++++++++--
>  v2v/test-v2v-print-source.expected |  2 +-
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/v2v/parse_libvirt_xml.ml b/v2v/parse_libvirt_xml.ml
> index 6032c31..2dee274 100644
> --- a/v2v/parse_libvirt_xml.ml
> +++ b/v2v/parse_libvirt_xml.ml
> @@ -50,7 +50,7 @@ let parse_libvirt_xml ?conn xml >    let xpathctx =
Xml.xpath_new_context doc in
>    let xpath_string = xpath_string xpathctx
>    and xpath_int = xpath_int xpathctx
> -  and xpath_int_default = xpath_int_default xpathctx
> +  (*and xpath_int_default = xpath_int_default xpathctx*)
>    and xpath_int64_default = xpath_int64_default xpathctx in
>  
>    let hypervisor > @@ -65,7 +65,6 @@ let parse_libvirt_xml ?conn xml
>      | Some s -> s in
>    let memory = xpath_int64_default "/domain/memory/text()"
(1024L *^ 1024L) in
>    let memory = memory *^ 1024L in
> -  let vcpu = xpath_int_default "/domain/vcpu/text()" 1 in
>  
>    let cpu_vendor = xpath_string "/domain/cpu/vendor/text()" in
>    let cpu_model = xpath_string "/domain/cpu/model/text()" in
> @@ -73,6 +72,21 @@ let parse_libvirt_xml ?conn xml >    let cpu_cores =
xpath_int "/domain/cpu/topology/@cores" in
>    let cpu_threads = xpath_int "/domain/cpu/topology/@threads" in
>  
> +  (* Get the <vcpu> field from the input XML.  If not set then
> +   * try calculating it from the <cpu> <topology> node.  If
that's
> +   * not set either, then assume 1 vCPU.
> +   *)
> +  let vcpu = xpath_int "/domain/vcpu/text()" in
> +  let vcpu > +    match vcpu, cpu_sockets, cpu_cores, cpu_threads with
> +    | Some vcpu, _,    _,    _    -> vcpu
> +    | None,      None, None, None -> 1
> +    | None,      _,    _,    _    ->
> +       let sockets = match cpu_sockets with None -> 1 | Some v -> v
in
> +       let cores = match cpu_cores with None -> 1 | Some v -> v in
> +       let threads = match cpu_threads with None -> 1 | Some v -> v
in
> +       sockets * cores * threads in
Should there be a sockets * cores * threads <> vcpu check as well?

LGTM otherwise.

Thanks,
-- 
Pino Toscano

On Fri, Mar 17, 2017 at 10:47:57AM +0100, Pino Toscano
wrote:
> On Thursday, 16 March 2017 22:10:10 CET Richard W.M. Jones wrote:
> > ---
> >  v2v/parse_libvirt_xml.ml           | 18 ++++++++++++++++--
> >  v2v/test-v2v-print-source.expected |  2 +-
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/v2v/parse_libvirt_xml.ml b/v2v/parse_libvirt_xml.ml
> > index 6032c31..2dee274 100644
> > --- a/v2v/parse_libvirt_xml.ml
> > +++ b/v2v/parse_libvirt_xml.ml
> > @@ -50,7 +50,7 @@ let parse_libvirt_xml ?conn xml > >    let
xpathctx = Xml.xpath_new_context doc in
> >    let xpath_string = xpath_string xpathctx
> >    and xpath_int = xpath_int xpathctx
> > -  and xpath_int_default = xpath_int_default xpathctx
> > +  (*and xpath_int_default = xpath_int_default xpathctx*)
> >    and xpath_int64_default = xpath_int64_default xpathctx in
> >  
> >    let hypervisor > > @@ -65,7 +65,6 @@ let parse_libvirt_xml
?conn xml > >      | Some s -> s in
> >    let memory = xpath_int64_default "/domain/memory/text()"
(1024L *^ 1024L) in
> >    let memory = memory *^ 1024L in
> > -  let vcpu = xpath_int_default "/domain/vcpu/text()" 1 in
> >  
> >    let cpu_vendor = xpath_string "/domain/cpu/vendor/text()"
in
> >    let cpu_model = xpath_string "/domain/cpu/model/text()"
in
> > @@ -73,6 +72,21 @@ let parse_libvirt_xml ?conn xml > >    let
cpu_cores = xpath_int "/domain/cpu/topology/@cores" in
> >    let cpu_threads = xpath_int
"/domain/cpu/topology/@threads" in
> >  
> > +  (* Get the <vcpu> field from the input XML.  If not set then
> > +   * try calculating it from the <cpu> <topology> node. 
If that's
> > +   * not set either, then assume 1 vCPU.
> > +   *)
> > +  let vcpu = xpath_int "/domain/vcpu/text()" in
> > +  let vcpu > > +    match vcpu, cpu_sockets, cpu_cores,
cpu_threads with
> > +    | Some vcpu, _,    _,    _    -> vcpu
> > +    | None,      None, None, None -> 1
> > +    | None,      _,    _,    _    ->
> > +       let sockets = match cpu_sockets with None -> 1 | Some v
-> v in
> > +       let cores = match cpu_cores with None -> 1 | Some v -> v
in
> > +       let threads = match cpu_threads with None -> 1 | Some v
-> v in
> > +       sockets * cores * threads in
> 
> Should there be a sockets * cores * threads <> vcpu check as well?
Yes, it's already included in v2v/v2v.ml in:

  [PATCH 4/4] v2v: Pass CPU vendor, model and topology from source to target.
  https://www.redhat.com/archives/libguestfs/2017-March/msg00185.html

However it's only a warning because we don't want to fail the
conversion if this metadata (which is basically informational) is
wrong.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW