On Wed, Jun 20, 2012 at 05:09:38PM +0800, Osier Yang wrote:
> setNumaParameters tunes the numa setting using cgroup, it's another
> entry except libnuma/numad for numa tuning. And it doesn't set the
> placement, and further more, the formating codes doesn't take this
> into consideration.
>
> How to reproduce:
>
> conn = libvirt.open(None)
> dom = conn.lookupByName('linux')
> param = {'numa_nodeset': '0', 'numa_mode': 1}
> dom.setNumaParameters(param, 2)
>
> % virsh start linux
> error: Failed to start domain rhel6.3rc
> error: (domain_definition):8: error parsing attribute name
> <memory mode='preferred' </numatune>
> -------------------------------^
>
> ---
> By the way, I see problems of setNumaParameters too.
>
> conn = libvirt.open(None)
> dom = conn.lookupByName('linux')
> param = {'numa_mode': 1}
> dom.setNumaParameters(param, 2)
>
> The numa 'mode' will be just ignored, and no 'numatune' XML is formated,
> as neither 'nodeset' nor 'placement' is specified. I'd think it's
> right to ignore it when formating, it's meaningless to only specify
> the 'mode'. However, we might have to fix setNumaParameters to prevent
> setting the numa mode without nodeset, and error out, as it's really a
> bad user experience to see the API call succeeded, but the expected
> XML doesn't show up in the end.
> ---
> src/conf/domain_conf.c | 17 ++++++++++-------
> 1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 81c6308..c44d89d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12795,23 +12795,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> const char *placement;
>
> mode = virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode);
> - virBufferAsprintf(buf, " <memory mode='%s' ", mode);
> + virBufferAsprintf(buf, " <memory mode='%s'", mode);
>
> - if (def->numatune.memory.placement_mode ==
> - VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
> + if (def->numatune.memory.nodemask) {
> nodemask = virDomainCpuSetFormat(def->numatune.memory.nodemask,
> - VIR_DOMAIN_CPUMASK_LEN);
> + VIR_DOMAIN_CPUMASK_LEN);
> if (nodemask == NULL) {
> virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("failed to format nodeset for "
> "NUMA memory tuning"));
> goto cleanup;
> }
> - virBufferAsprintf(buf, "nodeset='%s'/>\n", nodemask);
> + virBufferAsprintf(buf, " nodeset='%s'/>\n", nodemask);
> VIR_FREE(nodemask);
> - } else if (def->numatune.memory.placement_mode) {
> + } else if (def->numatune.memory.placement_mode ==
> + VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
> placement = virDomainNumatuneMemPlacementModeTypeToString(def->numatune.memory.placement_mode);
> - virBufferAsprintf(buf, "placement='%s'/>\n", placement);
> + virBufferAsprintf(buf, " placement='%s'/>\n", placement);
> + } else {
> + /* Should not hit here. */
> + virBufferAddLit(buf, "/>\n");
> }
> virBufferAddLit(buf, " </numatune>\n");
> }
The fact that we had such a horrific XML formatting bug shows that
there is a gap in our test coverage. Please add additional test
cases to cover this scenario
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list