[libvirt] [PATCH 0/2] rework the check for the numa cpus
We introduce a check for numa cpu total count in 5f7b71, But seems this check cannot work well. There are some scenarioes: 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/> the cpus '100' exceed maxvcpus, this setting is not valid (although qemu do not output error). 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> I guess nobody will use this setting if he really want use numa in his vm. (qemu not output error, but we can find some interesting things in vm, all cpus have bind in one numa node) 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-6' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> No need forbid this scenario if scenario 2 is a 'valid' setting. However add new check during parse xml will make vm has broken settings disappear after update libvirtd, so possible solutions: 1. add new check when parse xml, and find a way to avoid vm evaporate. I chose this way and i don't think just drop the invalid settings is a good idea for numa cpus, so i add a new function. 2. add new check when start vm. I think this is a configuration issue, so no need report it so late. 3. just remove this check and do not add new check... :) Luyao Huang (2): conf: introduce a new function to add check avoid losing track conf: rework the cpu check for vm numa settings src/bhyve/bhyve_driver.c | 4 ++-- src/conf/domain_conf.c | 21 ++++++++++++++------- src/conf/domain_conf.h | 1 + src/conf/numa_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/numa_conf.h | 2 +- src/esx/esx_driver.c | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/parallels/parallels_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 4 ++-- src/xen/xen_driver.c | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 18 files changed, 70 insertions(+), 39 deletions(-) -- 1.8.3.1
Add new check in the new function will avoid some vm disappear after restart libvirtd, and the new check will be called when define or create a vm. And after some version, maybe we can move these check to the right place. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/bhyve/bhyve_driver.c | 4 ++-- src/conf/domain_conf.c | 11 +++++++++++ src/conf/domain_conf.h | 1 + src/esx/esx_driver.c | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/parallels/parallels_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 4 ++-- src/xen/xen_driver.c | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 16 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 21db277..3cea3d6 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -495,7 +495,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag virDomainObjPtr vm = NULL; virObjectEventPtr event = NULL; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -895,7 +895,7 @@ bhyveDomainCreateXML(virConnectPtr conn, virObjectEventPtr event = NULL; virCapsPtr caps = NULL; unsigned int start_flags = 0; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY | VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cd6ee22..831f033 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13507,6 +13507,13 @@ virDomainThreadSchedParse(xmlNodePtr node, return -1; } +static int +virDomainDefNewCheck(virDomainDefPtr def) +{ + /*TO DO*/ + return 0; +} + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -15398,6 +15405,10 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainDefPostParse(def, caps, xmlopt) < 0) goto error; + if (flags & VIR_DOMAIN_DEF_PARSE_NEW_CHECK && + virDomainDefNewCheck(def) < 0) + goto error; + /* Auto-add any implied controllers which aren't present */ if (virDomainDefAddImplicitControllers(def) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 84e880a..21341f7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2528,6 +2528,7 @@ typedef enum { /* parse only source half of <disk> */ VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 7, VIR_DOMAIN_DEF_PARSE_VALIDATE = 1 << 8, + VIR_DOMAIN_DEF_PARSE_NEW_CHECK = 1 << 9, } virDomainDefParseFlags; typedef enum { diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 179f44c..db0a309 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3042,7 +3042,7 @@ esxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) char *taskInfoErrorMessage = NULL; virDomainPtr domain = NULL; const char *src; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 05f6eb1..23eb7e4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -862,7 +862,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_VALIDATE, NULL); @@ -2600,7 +2600,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag virDomainPtr dom = NULL; virObjectEventPtr event = NULL; virDomainDefPtr oldDef = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 245000d..38b4206 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -456,7 +456,7 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virDomainDefPtr oldDef = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -1198,7 +1198,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, virObjectEventPtr event = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY | VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d29e35b..fcc2ec1 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -994,7 +994,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla virDomainDefPtr vmdef = NULL; virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -1091,7 +1091,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; const char *progstart[] = {VZCTL, "--quiet", "start", PROGRAM_SENTINEL, NULL}; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index f5e58a8..59e371e 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -695,7 +695,7 @@ parallelsDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int virDomainPtr retdom = NULL; virDomainDefPtr def; virDomainObjPtr olddom = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f4db2e0..97db14e 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3560,7 +3560,7 @@ phypDomainCreateXML(virConnectPtr conn, lparPtr *lpars = uuid_table->lpars; size_t i = 0; char *managed_system = phyp_driver->managed_system; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index becf415..86aa92d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1675,7 +1675,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD; virQEMUCapsPtr qemuCaps = NULL; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY | @@ -7357,7 +7357,7 @@ static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml virQEMUCapsPtr qemuCaps = NULL; virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 07cc032..7e1924e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1745,7 +1745,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, virDomainDefPtr def; virDomainObjPtr dom = NULL; virObjectEventPtr event = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); @@ -2943,7 +2943,7 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn, virDomainObjPtr dom = NULL; virObjectEventPtr event = NULL; virDomainDefPtr oldDef = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 2d59126..d526ed8 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1612,7 +1612,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; virObjectEventPtr event = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY | VIR_DOMAIN_START_VALIDATE, NULL); @@ -2094,7 +2094,7 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virDomainDefPtr def; virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bb4de15..62e148b 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -1855,7 +1855,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags nsresult rc; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainPtr ret = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 3382994..1a486de 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -374,7 +374,7 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla char *vmxPath = NULL; vmwareDomainPtr pDomain = NULL; virVMXContext ctx; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -665,7 +665,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, char *vmxPath = NULL; vmwareDomainPtr pDomain = NULL; virVMXContext ctx; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 3b11e9a..fc31e2d 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -775,7 +775,7 @@ xenUnifiedDomainCreateXML(virConnectPtr conn, xenUnifiedPrivatePtr priv = conn->privateData; virDomainDefPtr def = NULL; virDomainPtr ret = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); @@ -1897,7 +1897,7 @@ xenUnifiedDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int xenUnifiedPrivatePtr priv = conn->privateData; virDomainDefPtr def = NULL; virDomainPtr ret = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index d495f21..06ba1e3 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -553,7 +553,7 @@ xenapiDomainCreateXML(virConnectPtr conn, xen_vm_record *record = NULL; xen_vm vm = NULL; virDomainPtr domP = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; if (!priv->caps) return NULL; @@ -1735,7 +1735,7 @@ xenapiDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla xen_vm_record *record = NULL; xen_vm vm = NULL; virDomainPtr domP = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_NEW_CHECK; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); -- 1.8.3.1
https://bugzilla.redhat.com/show_bug.cgi?id=1176020 We had a check for the vcpu count total number in <numa> before, however this check is not good enough. There are some examples: 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/> 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-6' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/> Use a new check for numa cpus, check if use a cpu exceeds maxvcpus and if duplicate use one cpu in more than one cell. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- src/conf/numa_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/numa_conf.h | 2 +- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 831f033..3a5eb1f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13510,7 +13510,10 @@ virDomainThreadSchedParse(xmlNodePtr node, static int virDomainDefNewCheck(virDomainDefPtr def) { - /*TO DO*/ + if (def->numa && + (virDomainNumaCheckCPU(def->numa, def->maxvcpus) < 0)) + return -1; + return 0; } @@ -14141,13 +14144,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainNumaDefCPUParseXML(def->numa, ctxt) < 0) goto error; - if (virDomainNumaGetCPUCountTotal(def->numa) > def->maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Number of CPUs in <numa> exceeds the" - " <vcpu> count")); - goto error; - } - if (virDomainNumatuneParseXML(def->numa, def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 8a0f686..0191a22 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -790,16 +790,39 @@ virDomainNumaDefCPUFormat(virBufferPtr buf, } -unsigned int -virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa) +int +virDomainNumaCheckCPU(virDomainNumaPtr numa, + unsigned short maxvcpus) { - size_t i; - unsigned int ret = 0; + size_t i,j; - for (i = 0; i < numa->nmem_nodes; i++) - ret += virBitmapCountBits(virDomainNumaGetNodeCpumask(numa, i)); + for (i = 0; i < numa->nmem_nodes; i++) { + virBitmapPtr nodeset = NULL; + ssize_t bit = -1; + + nodeset = virDomainNumaGetNodeCpumask(numa, i); + for (j = 0; j < i; j++) { + if (virBitmapOverlaps(virDomainNumaGetNodeCpumask(numa, j), + nodeset)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot binding one vCPU in 2 NUMA cell" + " %d and %d"), (int) i, (int) j); + return -1; + } + } - return ret; + while ((bit = virBitmapNextSetBit(nodeset, bit)) >= 0) { + if (bit <= maxvcpus-1) + continue; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("vcpu '%d' in <numa> cell '%d' exceeds the maxvcpus"), + (int) bit, (int) i); + return -1; + } + } + + return 0; } diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index ded6e01..d7713c8 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -149,7 +149,7 @@ bool virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune, int virDomainNumaDefCPUParseXML(virDomainNumaPtr def, xmlXPathContextPtr ctxt); int virDomainNumaDefCPUFormat(virBufferPtr buf, virDomainNumaPtr def); -unsigned int virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa); +int virDomainNumaCheckCPU(virDomainNumaPtr numa, unsigned short maxvcpus); #endif /* __NUMA_CONF_H__ */ -- 1.8.3.1
On 02.04.2015 10:02, Luyao Huang wrote:
We introduce a check for numa cpu total count in 5f7b71, But seems this check cannot work well. There are some scenarioes:
1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10):
<vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/>
the cpus '100' exceed maxvcpus, this setting is not valid (although qemu do not output error).
2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
I guess nobody will use this setting if he really want use numa in his vm. (qemu not output error, but we can find some interesting things in vm, all cpus have bind in one numa node)
3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-6' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
No need forbid this scenario if scenario 2 is a 'valid' setting.
However add new check during parse xml will make vm has broken settings disappear after update libvirtd, so possible solutions:
1. add new check when parse xml, and find a way to avoid vm evaporate. I chose this way and i don't think just drop the invalid settings is a good idea for numa cpus, so i add a new function.
2. add new check when start vm. I think this is a configuration issue, so no need report it so late.
3. just remove this check and do not add new check... :)
Luyao Huang (2): conf: introduce a new function to add check avoid losing track conf: rework the cpu check for vm numa settings
src/bhyve/bhyve_driver.c | 4 ++-- src/conf/domain_conf.c | 21 ++++++++++++++------- src/conf/domain_conf.h | 1 + src/conf/numa_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/numa_conf.h | 2 +- src/esx/esx_driver.c | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/parallels/parallels_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 4 ++-- src/xen/xen_driver.c | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 18 files changed, 70 insertions(+), 39 deletions(-)
I agree with renaming and extending the virDomainNumaGetCPUCountTotal(). Even the diff in 2/2 shows the function is utterly broken. However, I think this function can be called unconditionally - even when the flag is not set. Having said that, the flag is redundant. Moreover, when sending a patchset, the sources should compile cleanly after each patch. This is not the case with this one. Looking forward to v2. Michal
On 04/20/2015 10:39 PM, Michal Privoznik wrote:
On 02.04.2015 10:02, Luyao Huang wrote:
We introduce a check for numa cpu total count in 5f7b71, But seems this check cannot work well. There are some scenarioes:
1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10):
<vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/>
the cpus '100' exceed maxvcpus, this setting is not valid (although qemu do not output error).
2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
I guess nobody will use this setting if he really want use numa in his vm. (qemu not output error, but we can find some interesting things in vm, all cpus have bind in one numa node)
3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): <vcpu placement='static'>10</vcpu> <cell id='0' cpus='0-6' memory='512000' unit='KiB'/> <cell id='1' cpus='0-3' memory='512000' unit='KiB'/>
No need forbid this scenario if scenario 2 is a 'valid' setting.
However add new check during parse xml will make vm has broken settings disappear after update libvirtd, so possible solutions:
1. add new check when parse xml, and find a way to avoid vm evaporate. I chose this way and i don't think just drop the invalid settings is a good idea for numa cpus, so i add a new function.
2. add new check when start vm. I think this is a configuration issue, so no need report it so late.
3. just remove this check and do not add new check... :)
Luyao Huang (2): conf: introduce a new function to add check avoid losing track conf: rework the cpu check for vm numa settings
src/bhyve/bhyve_driver.c | 4 ++-- src/conf/domain_conf.c | 21 ++++++++++++++------- src/conf/domain_conf.h | 1 + src/conf/numa_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/numa_conf.h | 2 +- src/esx/esx_driver.c | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/parallels/parallels_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 4 ++-- src/xen/xen_driver.c | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 18 files changed, 70 insertions(+), 39 deletions(-)
I agree with renaming and extending the virDomainNumaGetCPUCountTotal(). Even the diff in 2/2 shows the function is utterly broken. However, I think this function can be called unconditionally - even when the flag is not set. Having said that, the flag is redundant.
Oh, good news to me :) So if this function can be called unconditionally, there is no reason to introduce this new flag.
Moreover, when sending a patchset, the sources should compile cleanly after each patch. This is not the case with this one.
Get it, i will pay more attention for this things next time, thanks for pointing out that.
Looking forward to v2.
Thanks for your review and advise, i will propose a new version these days.
Michal
Luyao
participants (3)
-
lhuang -
Luyao Huang
-
Michal Privoznik