[libvirt] [PATCH v2 1/3] conf: Format numatune XML correctly while placement is none
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> -------------------------------^ --- 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"); } -- 1.7.7.3
supportFeatures is also added for the requirement of VIR_DRV_FEATURE_TYPED_PARAM_STRING support. --- src/test/test_driver.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 177 insertions(+), 0 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b3b774d..f3c06a7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2884,6 +2884,169 @@ error: return ret; } +static int +testDomainSetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + int i; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1); + if (virTypedParameterArrayValidate(params, nparams, + VIR_DOMAIN_NUMA_MODE, + VIR_TYPED_PARAM_INT, + VIR_DOMAIN_NUMA_NODESET, + VIR_TYPED_PARAM_STRING, + NULL) < 0) + return -1; + + testDriverLock(privconn); + + vm = virDomainFindByUUID(&privconn->domains, dom->uuid); + + if (vm == NULL) { + testError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), dom->uuid); + goto cleanup; + } + + ret = 0; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { + vm->def->numatune.memory.mode = params[i].value.i; + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { + bool savedmask; + char oldnodemask[VIR_DOMAIN_CPUMASK_LEN]; + + /* update vm->def here so that dumpxml can read the new + * values from vm->def. */ + savedmask = false; + if (!vm->def->numatune.memory.nodemask) { + if (VIR_ALLOC_N(vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + } else { + memcpy(oldnodemask, vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); + savedmask = true; + } + + if (virDomainCpuSetParse(params[i].value.s, + 0, + vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + if (savedmask) + memcpy(vm->def->numatune.memory.nodemask, + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); + else + VIR_FREE(vm->def->numatune.memory.nodemask); + ret = -1; + continue; + } + } + } + + if (ret == 0) + virDomainObjAssignDef(vm, vm->def, false); +cleanup: + if (vm) + virDomainObjUnlock(vm); + testDriverUnlock(privconn); + return ret; +} + +#define TEST_NB_NUMA_PARAM 2 + +static int +testDomainGetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + int i; + virDomainObjPtr vm = NULL; + char *nodeset = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + testDriverLock(privconn); + + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; + + vm = virDomainFindByUUID(&privconn->domains, dom->uuid); + + if (vm == NULL) { + testError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), dom->uuid); + goto cleanup; + } + + if ((*nparams) == 0) { + *nparams = TEST_NB_NUMA_PARAM; + ret = 0; + goto cleanup; + } + + for (i = 0; i < TEST_NB_NUMA_PARAM && i < *nparams; i++) { + virMemoryParameterPtr param = ¶ms[i]; + + switch (i) { + case 0: /* fill numa mode here */ + if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_MODE, + VIR_TYPED_PARAM_INT, 0) < 0) + goto cleanup; + param->value.i = vm->def->numatune.memory.mode; + break; + + case 1: /* fill numa nodeset here */ + if (vm->def->numatune.memory.nodemask) + nodeset = virDomainCpuSetFormat(vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); + else + nodeset = strdup(""); + + if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET, + VIR_TYPED_PARAM_STRING, nodeset) < 0) + goto cleanup; + + nodeset = NULL; + + break; + + default: + break; + /* should not hit here */ + } + } + + if (*nparams > TEST_NB_NUMA_PARAM) + *nparams = TEST_NB_NUMA_PARAM; + ret = 0; + +cleanup: + VIR_FREE(nodeset); + if (vm) + virDomainObjUnlock(vm); + testDriverUnlock(privconn); + return ret; +} + static virDrvOpenStatus testOpenNetwork(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -5534,12 +5697,24 @@ static int testListAllDomains(virConnectPtr conn, return ret; } +/* Which features are supported by this driver? */ +static int +testSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int feature) +{ + switch (feature) { + case VIR_DRV_FEATURE_TYPED_PARAM_STRING: + return 1; + default: + return 0; + } +} static virDriver testDriver = { .no = VIR_DRV_TEST, .name = "Test", .open = testOpen, /* 0.1.1 */ .close = testClose, /* 0.1.1 */ + .supports_feature = testSupportsFeature, /* 0.5.0 */ .version = testGetVersion, /* 0.1.1 */ .getHostname = virGetHostname, /* 0.6.3 */ .getMaxVcpus = testGetMaxVCPUs, /* 0.3.2 */ @@ -5603,6 +5778,8 @@ static virDriver testDriver = { .domainEventRegisterAny = testDomainEventRegisterAny, /* 0.8.0 */ .domainEventDeregisterAny = testDomainEventDeregisterAny, /* 0.8.0 */ .isAlive = testIsAlive, /* 0.9.8 */ + .domainSetNumaParameters = testDomainSetNumaParameters, /* 0.9.9 */ + .domainGetNumaParameters = testDomainGetNumaParameters, /* 0.9.9 */ }; static virNetworkDriver testNetworkDriver = { -- 1.7.7.3
The test driver free()s the test domain conf each time when the connection is closed, so it's not able to test the APIs which change the domain conf on the fly with virsh. This patch is to add a new test program domapitest.c, which tests the APIs by directly invoking the C APIs. Currently there is only one test in it for setNumaParameters testing, but similiar APIs testing can be added there. --- tests/Makefile.am | 7 ++- tests/domapitest.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 1 deletions(-) create mode 100644 tests/domapitest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index e3bd6d1..3ddc43b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -91,7 +91,7 @@ test_programs = virshtest sockettest \ virhashtest virnetmessagetest virnetsockettest \ utiltest virnettlscontexttest shunloadtest \ virtimetest viruritest virkeyfiletest \ - virauthconfigtest + virauthconfigtest domapitest if WITH_DRIVER_MODULES test_programs += virdrivermoduletest @@ -452,6 +452,11 @@ commandhelper_CFLAGS = -Dabs_builddir="\"`pwd`\"" $(AM_CFLAGS) commandhelper_LDADD = $(LDADDS) commandhelper_LDFLAGS = -static +domapitest_SOURCES = \ + domapitest.c \ + testutils.c testutils.h +domapitest_LDADD = $(LDADDS) + if WITH_LIBVIRTD libvirtdconftest_SOURCES = \ libvirtdconftest.c testutils.h testutils.c \ diff --git a/tests/domapitest.c b/tests/domapitest.c new file mode 100644 index 0000000..c093eb9 --- /dev/null +++ b/tests/domapitest.c @@ -0,0 +1,151 @@ +/* + * domapitest.c: Test the APIs which changes domain conf + * + * Copyright (C) 2010-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Osier Yang <jyang@redhat.com> + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#include "internal.h" +#include "xml.h" +#include "util.h" +#include "testutils.h" +#include "memory.h" +#include "virtypedparam.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_TEST + +static const char *domname = "test"; +static const char *uri = "test:///default"; + +static int +testSetNumaParameters(const void *data) { + virDomainPtr dom = (virDomainPtr) data; + int nparams = 2; + virTypedParameterPtr params = NULL; + int flags = 0; + char *actual = NULL; + int ret = -1; + const char *exp = "\ +<domain type='test' id='1'>\n\ + <name>test</name>\n\ + <uuid>6695eb01-f6a4-8304-79aa-97f2502e193f</uuid>\n\ + <memory unit='KiB'>8388608</memory>\n\ + <currentMemory unit='KiB'>2097152</currentMemory>\n\ + <vcpu placement='static'>2</vcpu>\n\ + <numatune>\n\ + <memory mode='strict' nodeset='10'/>\n\ + </numatune>\n\ + <os>\n\ + <type arch='i686'>hvm</type>\n\ + <boot dev='hd'/>\n\ + </os>\n\ + <clock offset='utc'/>\n\ + <on_poweroff>destroy</on_poweroff>\n\ + <on_reboot>restart</on_reboot>\n\ + <on_crash>destroy</on_crash>\n\ + <devices>\n\ + </devices>\n\ +</domain>\n"; + + if (VIR_ALLOC_N(params, nparams) < 0){ + virReportOOMError(); + return -1; + } + + params[0].type = VIR_TYPED_PARAM_INT; + if (!virStrcpy(params[0].field, + VIR_DOMAIN_NUMA_MODE, + sizeof(params[0].field))) { + virReportOOMError(); + goto fail; + } + params[0].value.i = VIR_DOMAIN_NUMATUNE_MEM_STRICT; + + params[1].type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(params[1].field, + VIR_DOMAIN_NUMA_NODESET, + sizeof(params[1].field))) { + virReportOOMError(); + goto fail; + } + params[1].value.s = strdup("10"); + + if (virDomainSetNumaParameters(dom, params, nparams, flags) != 0) { + fprintf(stderr, "virDomainSetNumaParameters failed\n"); + goto fail; + } + + if (!(actual = virDomainGetXMLDesc(dom, flags))) { + fprintf(stderr, "virDomainGetXMLDesc failed\n"); + goto fail; + } + + if (STRNEQ(exp, actual)) { + virtTestDifference(stderr, exp, actual); + goto fail; + } + + ret = 0; + +fail: + virTypedParameterArrayClear(params, nparams); + VIR_FREE(actual); + return ret; +} + +static int +mymain(void) +{ + virConnectPtr conn; + virDomainPtr dom; + int ret = -1; + + conn = virConnectOpen(uri); + if (conn == NULL) { + fprintf(stderr, "virConnectOpen failed\n"); + return EXIT_FAILURE; + } + + dom = virDomainLookupByName(conn, domname); + if (dom == NULL) { + fprintf(stderr, "virDomainLookupByName failed\n"); + goto fail; + } + + if (virtTestRun("setNumaParameters", + 1, testSetNumaParameters, dom) != 0) + goto fail; + + ret = 0; + +fail: + /* testClose frees the domain object */ + if(conn) + virConnectClose(conn); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.7.7.3
ping? On 2012年06月25日 12:28, 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> -------------------------------^ --- 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"); }
On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
ping?
On 2012年06月25日 12:28, 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> -------------------------------^ --- 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 this bug existed shows that the test suite for the XML parser is incomplete. Please resubmit with an extra test XML datafile for the test suite to validate this scenario. Regards, 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 :|
On 2012年07月10日 17:41, Daniel P. Berrange wrote:
On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
ping?
On 2012年06月25日 12:28, 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> -------------------------------^ --- 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 this bug existed shows that the test suite for the XML parser is incomplete. Please resubmit with an extra test XML datafile for the test suite to validate this scenario.
we already has good enough XMLs for the test suite: % ls tests/qemuxml2argvdata/qemuxml2argv-numa qemuxml2argv-numad.args qemuxml2argv-numad-auto-vcpu-static-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-cpuset.args qemuxml2argv-numad-static-memory-auto-vcpu.args qemuxml2argv-numad-auto-memory-vcpu-cpuset.xml qemuxml2argv-numad-static-memory-auto-vcpu.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.args qemuxml2argv-numad-static-vcpu-no-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.xml qemuxml2argv-numad.xml qemuxml2argv-numad-auto-vcpu-no-numatune.xml qemuxml2argv-numatune-memory.args qemuxml2argv-numad-auto-vcpu-static-numatune.args qemuxml2argv-numatune-memory.xml The problem is we have two entries to change the numatune config, and they share the same XML syntax (btw, I was thinking it's a bad idea to do so, it could just cause crasy results). XML parser actually ensures the placement mode can be always set with either 'static' or 'auto', but API via cgroup don't set placement mode as placement is meaningless for it, so IMHO no need to add XMLs to test the parser, instead we need to add tests to test the API. Regards, Osier
On 07/10/2012 04:00 AM, Osier Yang wrote:
On 2012年07月10日 17:41, Daniel P. Berrange wrote:
On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
ping?
On 2012年06月25日 12:28, 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.
+ } else { + /* Should not hit here. */ + virBufferAddLit(buf, "/>\n");
If we can't get here, then we have a bigger bug. Fixing the output is wrong if we have an earlier bug generating an inconsistent struct in the first place.
The fact that this bug existed shows that the test suite for the XML parser is incomplete. Please resubmit with an extra test XML datafile for the test suite to validate this scenario.
I agree.
we already has good enough XMLs for the test suite:
% ls tests/qemuxml2argvdata/qemuxml2argv-numa qemuxml2argv-numad.args qemuxml2argv-numad-auto-vcpu-static-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-cpuset.args qemuxml2argv-numad-static-memory-auto-vcpu.args qemuxml2argv-numad-auto-memory-vcpu-cpuset.xml qemuxml2argv-numad-static-memory-auto-vcpu.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.args qemuxml2argv-numad-static-vcpu-no-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.xml qemuxml2argv-numad.xml qemuxml2argv-numad-auto-vcpu-no-numatune.xml qemuxml2argv-numatune-memory.args qemuxml2argv-numad-auto-vcpu-static-numatune.args qemuxml2argv-numatune-memory.xml
These may have used <numatune>, but none of them used it in the manner that would have been detected by round trip parsing. Ergo, we are missing a test, either a test that invalid input data is rejected earlier on, or a test that when input data omits information, then round trip handing of that data doesn't get the output into an inconsistent state when it adds in the defaults for the omitted information.
The problem is we have two entries to change the numatune config, and they share the same XML syntax (btw, I was thinking it's a bad idea to do so, it could just cause crasy results).
Which two entries are you thinking of? Maybe it is a bad design decision, but we're stuck with it, so we should at least support what we have documented.
XML parser actually ensures the placement mode can be always set with either 'static' or 'auto', but API via cgroup don't set placement mode as placement is meaningless for it, so IMHO no need to add XMLs to test the parser, instead we need to add tests to test the API.
The TCK is a great place to test the API. But maybe the real bug here is that if you hotplug a request for a numatune and it goes through cgroup, then we should be updating the conf struct to reflect either static or auto, even though cgroup doesn't directly care, so that the output function then works in all cases. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年07月31日 01:37, Eric Blake wrote:
On 07/10/2012 04:00 AM, Osier Yang wrote:
On 2012年07月10日 17:41, Daniel P. Berrange wrote:
On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
ping?
On 2012年06月25日 12:28, 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.
+ } else { + /* Should not hit here. */ + virBufferAddLit(buf, "/>\n");
If we can't get here, then we have a bigger bug.
We won't get here anyway, either fixing the output or the internal inconsistent struct. If fixing the output, then just as this patch did, checking if the nodeset is set (this prohibit the case which doesn't set placement for internal struct - like setNumaParameters does, the current output formating code doesn't take it into consideration). If fixing setNumaParameters to set a placement even if 'placement' doesn't make sense for cgroup, we also can't get the 'else' branch. Fixing the output is
wrong if we have an earlier bug generating an inconsistent struct in the first place.
Agreed.
The fact that this bug existed shows that the test suite for the XML parser is incomplete. Please resubmit with an extra test XML datafile for the test suite to validate this scenario.
I agree.
we already has good enough XMLs for the test suite:
% ls tests/qemuxml2argvdata/qemuxml2argv-numa qemuxml2argv-numad.args qemuxml2argv-numad-auto-vcpu-static-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-cpuset.args qemuxml2argv-numad-static-memory-auto-vcpu.args qemuxml2argv-numad-auto-memory-vcpu-cpuset.xml qemuxml2argv-numad-static-memory-auto-vcpu.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.args qemuxml2argv-numad-static-vcpu-no-numatune.xml qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.xml qemuxml2argv-numad.xml qemuxml2argv-numad-auto-vcpu-no-numatune.xml qemuxml2argv-numatune-memory.args qemuxml2argv-numad-auto-vcpu-static-numatune.args qemuxml2argv-numatune-memory.xml
These may have used<numatune>, but none of them used it in the manner that would have been detected by round trip parsing.
Yeah. The reason for why I think the testing xml datafiles are enough is we considered nearly everything from parsing p.o.v. Ergo, we are
missing a test, either a test that invalid input data is rejected earlier on,
And the xml datafiles cover this. or a test that when input data omits information, then round
trip handing of that data doesn't get the output into an inconsistent state when it adds in the defaults for the omitted information.
Yes, we miss such a test. Which doesn't relates with the parsing, it's monodirectional from the API -> internal struct -> output. So no xml datafiles. That was why I didn't understand Daniel said xml datafile is needed.
The problem is we have two entries to change the numatune config, and they share the same XML syntax (btw, I was thinking it's a bad idea to do so, it could just cause crasy results).
Which two entries are you thinking of?
One is libnuma, one is cgroup, so I meant the entries underlying. they use same syntax, different implementation, and for same driver. Which could just make things a mess. We should avoid design like this in future. Maybe it is a bad design
decision, but we're stuck with it, so we should at least support what we have documented.
XML parser actually ensures the placement mode can be always set with either 'static' or 'auto', but API via cgroup don't set placement mode as placement is meaningless for it, so IMHO no need to add XMLs to test the parser, instead we need to add tests to test the API.
The TCK is a great place to test the API.
Guess I forgot this apprently. :-) But maybe the real bug here
is that if you hotplug a request for a numatune and it goes through cgroup, then we should be updating the conf struct to reflect either static or auto, even though cgroup doesn't directly care, so that the output function then works in all cases.
So, think we get a consensus: Add the test in TCK, and defaults the internal struct so that it doesn't fall into inconsistence. I will go this way and post v3. Thanks for the opinions! Regards, Osier
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang