3:02 a.m.
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
3:02 a.m.
New subject: [libvirt] [PATCH 1/2] conf: introduce a new function to add check avoid losing track
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(a)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
3:02 a.m.
New subject: [libvirt] [PATCH 2/2] conf: rework the cpu check for vm numa settings
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(a)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
9:39 a.m.
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
1:17 a.m.
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
3534
days inactive
3553
days old
4 comments
3 participants
participants (3)
-
lhuang -
Luyao Huang
-
Michal Privoznik