On 01/22/2018 07:36 AM, menno.lageman@xxxxxxxxxx wrote:
> From: Menno Lageman <menno.lageman@xxxxxxxxxx>
>
Nice patch!
> Now that libvirt has support for administration of distances between NUMA cells
> it would be nice to be able to set those with virt-install directly instead of
> having to 'virsh edit' the domain XML manually after installation.
>
> NUMA distances can be specified when configuring NUMA cells by adding one or
> more <cell:distance> pairs for each NUMA cell, defining the distance from
> the cell to itself and to the other NUMA cells.
>
> For example
>
> --cpu cell0.memory=1234,cell0.cpus=0-3,cell1.memory=5678,cell1.cpus=4-7, \
> cell0.dist=0:10,1:21,cell1.dist=0:21,1:10
>
Rather than invent a custom format here, I'd rather make it work in the
same way cell specification works:
--cpu cell0.distances.sibling0.value=10,...
More typing and wordier but nowadays I prefer new command line options
to mirror libvirt XML within reason. It's the best option for long term
maintenance in my experience
Though I think virtinst/cli.py infrastructure may need to be extended to
handle that format. I'll play with it
Small comments inline
> would generate the following XML:
>
> <cpu>
> <numa>
> <cell cpus="0-3" memory="1234">
> <distances>
> <sibling id="0" value="10"/>
> <sibling id="1" value="21"/>
> </distances>
> </cell>
> <cell cpus="4-7" memory="5678">
> <distances>
> <sibling id="0" value="21"/>
> <sibling id="1" value="10"/>
> </distances>
> </cell>
> </numa>
> </cpu>
>
> Default distances (i.e. 10 for local nodes and 20 for remote nodes) may
> be omitted since libvirtd supplies defaults for those, so the above distance
> specification could be shortened to 'cell0.dist=1:21,cell1.dist=0:21'
>
> Signed-off-by: Menno Lageman <menno.lageman@xxxxxxxxxx>
> ---
> man/virt-install.pod | 16 ++++++++++---
> .../compare/virt-install-singleton-config-2.xml | 28 ++++++++++++++++++----
> tests/clitest.py | 2 +-
> virtinst/cli.py | 16 +++++++++++++
> virtinst/cpu.py | 27 +++++++++++++++++++++
> 5 files changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/man/virt-install.pod b/man/virt-install.pod
> index 349e4e6c..389b2d59 100644
> --- a/man/virt-install.pod
> +++ b/man/virt-install.pod
> @@ -236,14 +236,24 @@ may cause issues if migrating the guest to a host without an identical CPU.
> Expose the nearest host CPU model configuration to the guest.
> It is the best CPU which can be used for a guest on any of the hosts.
>
> -=item B<--cpu cell0.memory=1234,cell0.cpus=0-3,cell1.memory=5678,cell1.cpus=4-7>
> +=item B<--cpu cell0.memory=1234,cell0.cpus=0-3,cell1.memory=5678,cell1.cpus=4-7,cell0.dist=0:10,1:21,cell1.dist=0:21,1:10>
>
> Example of specifying two NUMA cells. This will generate XML like:
>
> <cpu>
> <numa>
> - <cell cpus="0-3" memory="1234"/>
> - <cell cpus="4-7" memory="5678"/>
> + <cell cpus="0-3" memory="1234">
> + <distances>
> + <sibling id="0" value="10"/>
> + <sibling id="1" value="21"/>
> + </distances>
> + </cell>
> + <cell cpus="4-7" memory="5678">
> + <distances>
> + <sibling id="0" value="21"/>
> + <sibling id="1" value="10"/>
> + </distances>
> + </cell>
> </numa>
> </cpu>
>
> diff --git a/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml b/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> index c2c641e4..3b9210f5 100644
> --- a/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> +++ b/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> @@ -92,8 +92,18 @@
> <feature policy="forbid" name="foo"/>
> <feature policy="forbid" name="bar"/>
> <numa>
> - <cell id="0" cpus="1,2,3" memory="1024"/>
> - <cell id="1" cpus="5-8" memory="256"/>
> + <cell id="0" cpus="1,2,3" memory="1024">
> + <distances>
> + <sibling id="0" value="10"/>
> + <sibling id="1" value="21"/>
> + </distances>
> + </cell>
> + <cell id="1" cpus="5-8" memory="256">
> + <distances>
> + <sibling id="0" value="21"/>
> + <sibling id="1" value="10"/>
> + </distances>
> + </cell>
> </numa>
> <cache mode="emulate" level="3"/>
> </cpu>
> @@ -247,8 +257,18 @@
> <feature policy="forbid" name="foo"/>
> <feature policy="forbid" name="bar"/>
> <numa>
> - <cell id="0" cpus="1,2,3" memory="1024"/>
> - <cell id="1" cpus="5-8" memory="256"/>
> + <cell id="0" cpus="1,2,3" memory="1024">
> + <distances>
> + <sibling id="0" value="10"/>
> + <sibling id="1" value="21"/>
> + </distances>
> + </cell>
> + <cell id="1" cpus="5-8" memory="256">
> + <distances>
> + <sibling id="0" value="21"/>
> + <sibling id="1" value="10"/>
> + </distances>
> + </cell>
> </numa>
> <cache mode="emulate" level="3"/>
> </cpu>
> diff --git a/tests/clitest.py b/tests/clitest.py
> index 94b6cbb4..d70462fd 100644
> --- a/tests/clitest.py
> +++ b/tests/clitest.py
> @@ -422,7 +422,7 @@ c.add_compare(""" \
> c.add_compare("""--pxe \
> --memory 512,maxmemory=1024 \
> --vcpus 4,cores=2,threads=2,sockets=2 \
> ---cpu foobar,+x2apic,+x2apicagain,-distest,forbid=foo,forbid=bar,disable=distest2,optional=opttest,require=reqtest,match=strict,vendor=meee,cell.id=0,cell.cpus=1,2,3,cell.memory=1024,cell1.id=1,cell1.memory=256,cell1.cpus=5-8,cache.mode=emulate,cache.level=3 \
> +--cpu foobar,+x2apic,+x2apicagain,-distest,forbid=foo,forbid=bar,disable=distest2,optional=opttest,require=reqtest,match=strict,vendor=meee,cell.id=0,cell.cpus=1,2,3,cell.memory=1024,cell1.id=1,cell1.memory=256,cell1.cpus=5-8,cell0.dist=0:10,1:21,cell1.dist=0:21,1:10,cache.mode=emulate,cache.level=3 \
> --metadata title=my-title,description=my-description,uuid=00000000-1111-2222-3333-444444444444 \
> --boot cdrom,fd,hd,network,menu=off,loader=/foo/bar \
> --idmap uid_start=0,uid_target=1000,uid_count=10,gid_start=0,gid_target=1000,gid_count=10 \
> diff --git a/virtinst/cli.py b/virtinst/cli.py
> index b0e4fab5..494738e3 100644
> --- a/virtinst/cli.py
> +++ b/virtinst/cli.py
> @@ -1441,6 +1441,19 @@ class ParserCPU(VirtCLIParser):
> return None
> raise
>
> + def cell_distance_cb(self, inst, val, virtarg):
> + cell = inst
> +
> + if val is None:
> + raise RuntimeError(_("Missing node:distance value"))
> +
> + for distance in val.split(','):
> + if ':' not in distance:
> + raise RuntimeError(_("Invalid node:distance value '%s'") % distance)
> +
> + (node_id, dist) = distance.split(':')
> + cell.add_distance(node_id, dist)
> +
> def set_model_cb(self, inst, val, virtarg):
> if val == "host":
> val = inst.SPECIAL_MODE_HOST_MODEL
> @@ -1519,6 +1532,9 @@ ParserCPU.add_arg("cpus", "cell[0-9]*.cpus", can_comma=True,
> find_inst_cb=ParserCPU.cell_find_inst_cb)
> ParserCPU.add_arg("memory", "cell[0-9]*.memory",
> find_inst_cb=ParserCPU.cell_find_inst_cb)
> +ParserCPU.add_arg(None, "cell[0-9]*.dist", can_comma=True,
> + find_inst_cb=ParserCPU.cell_find_inst_cb,
> + cb=ParserCPU.cell_distance_cb)
>
> # Options for CPU.cache
> ParserCPU.add_arg("mode", "cache.mode", find_inst_cb=ParserCPU.set_l3_cache_cb)
> diff --git a/virtinst/cpu.py b/virtinst/cpu.py
> index 3925106c..c2d768c0 100644
> --- a/virtinst/cpu.py
> +++ b/virtinst/cpu.py
> @@ -20,6 +20,25 @@
> from .xmlbuilder import XMLBuilder, XMLProperty, XMLChildProperty
>
>
> +class _CPUCellDist(XMLBuilder):
> + """
> + Class for generating <distances> child nodes
> + """
> + _XML_ROOT_NAME = "sibling"
> + _XML_PROP_ORDER = ["id", "value"]
> +
> + def validate_node_id(self, node_id):
> + if not node_id or not node_id.isdigit() or int(node_id) < 0:
> + raise RuntimeError(_("Missing or invalid node id '%s'") % node_id)
> +
> + def validate_dist(self, dist):
> + if not dist or not dist.isdigit() or int(dist) < 10 or int(dist) > 255:
> + raise RuntimeError(_("Missing or invalid distance value '%s'") % dist)
> +
I prefer to leave this type of validation to libvirt or the lower layer,
so I think these functions should be dropped. is_int=True below should
also cover the first two checks. If libvirt isn't doing the remaining
validation and you think it should, I suggest sending a patch there.
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
