[libvirt] parallels: add 2 functions needed by OpenStack

Hello, I would like to apologize my for my inattention, that led to unformatted subjects. I'll try to prevent this in the future. -- Regards, Alexander Burluka

From: A.Burluka <aburluka@parallels.com> OpenStack Nova requires this function to start VM instance. Cpumask info is obtained via prlctl utility. Unlike KVM, Parallels Cloud Server is unable to set cpu affinity mask for every VCpu. Mask is unique for all VCpu. You can set it using 'prlctl set <vm_id|vm_name> --cpumask <{n[,n,n1-n2]|all}>' command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7' would set this mask to yy---yyy. --- src/parallels/parallels_driver.c | 169 +++++++++++++++++++++++++++++++++++++- src/parallels/parallels_utils.h | 1 + 2 files changed, 167 insertions(+), 3 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ab59599..f1d5ecc 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -108,6 +108,7 @@ parallelsDomObjFreePrivate(void *p) if (!pdom) return; + VIR_FREE(pdom->cpumask); VIR_FREE(pdom->uuid); VIR_FREE(pdom->home); VIR_FREE(p); @@ -654,6 +655,9 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) if (VIR_ALLOC(def) < 0) goto cleanup; + if (VIR_ALLOC(pdom) < 0) + goto cleanup; + def->virtType = VIR_DOMAIN_VIRT_PARALLELS; def->id = -1; @@ -716,6 +720,17 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; } + if (!(tmp = virJSONValueObjectGetString(jobj3, "mask"))) { + /* Absence of this field means that all domains cpus are available */ + if (VIR_STRDUP(pdom->cpumask, "all") < 0) { + goto cleanup; + } + } else { + if (VIR_STRDUP(pdom->cpumask, tmp) < 0) { + goto cleanup; + } + } + if (!(jobj3 = virJSONValueObjectGet(jobj2, "memory"))) { parallelsParseError(); goto cleanup; @@ -757,9 +772,6 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) def->os.arch = VIR_ARCH_X86_64; - if (VIR_ALLOC(pdom) < 0) - goto cleanup; - if (virJSONValueObjectGetNumberUint(jobj, "EnvID", &x) < 0) goto cleanup; pdom->id = x; @@ -2300,6 +2312,156 @@ static int parallelsConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) } +static int +parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata, + virBitmapPtr *cpumask, + int hostcpus) +{ + int ret = -1; + int cpunum = -1; + int prevcpunum = -1; + int offset = 0; + const char *it = privatedomdata->cpumask; + bool isrange = false; + size_t i; + int cpunums[512] = { 0 }; + size_t count = 0; + + if (STREQ(it, "all")) { + if (!(*cpumask = virBitmapNew(hostcpus))) + goto cleanup; + virBitmapSetAll(*cpumask); + } else { + while (sscanf(it, "%d%n", &cpunum, &offset)) { + char delim = 0; + if (isrange) { + for (i = prevcpunum + 1; i <= cpunum; ++i) { + cpunums[count++] = i; + } + } else { + cpunums[count++] = cpunum; + } + + it += offset; + + if (sscanf(it, "%c%n", &delim, &offset) == EOF) { + break; + } else { + it += offset; + switch (delim) { + case ',': + isrange = false; + break; + case '-': + isrange = true; + prevcpunum = cpunum; + break; + default: + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid cpumask format '%s'"), + privatedomdata->cpumask); + goto cleanup; + break; + } + } + } + if (!(*cpumask = virBitmapNew(cpunums[count-1] + 1))) + goto cleanup; + virBitmapClearAll(*cpumask); + for (i = 0; i < count; ++i) { + if (virBitmapSetBit(*cpumask, cpunums[i]) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set %d bit in cpumask"), + cpunums[i]); + goto cleanup; + } + } + } + + return 0; + cleanup: + virBitmapFree(*cpumask); + return ret; +} + +static int +parallelsDomainGetVcpus(virDomainPtr domain, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen) +{ + parallelsConnPtr privconn = domain->conn->privateData; + parallelsDomObjPtr privdomdata = NULL; + virDomainObjPtr privdom = NULL; + size_t i; + int v, maxcpu, hostcpus; + int ret = -1; + + parallelsDriverLock(privconn); + privdom = virDomainObjListFindByUUID(privconn->domains, domain->uuid); + parallelsDriverUnlock(privconn); + + if (privdom == NULL) { + parallelsDomNotFoundError(domain); + goto cleanup; + } + + if (!virDomainObjIsActive(privdom)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", + _("cannot list vcpu pinning for an inactive domain")); + goto cleanup; + } + + privdomdata = privdom->privateData; + if ((hostcpus = nodeGetCPUCount()) < 0) + goto cleanup; + + maxcpu = maplen * 8; + if (maxcpu > hostcpus) + maxcpu = hostcpus; + + if (maxinfo >= 1) { + if (info != NULL) { + memset(info, 0, sizeof(*info) * maxinfo); + for (i = 0; i < maxinfo; i++) { + info[i].number = i; + info[i].state = VIR_VCPU_RUNNING; + } + } + if (cpumaps != NULL) { + unsigned char *tmpmap = NULL; + int tmpmapLen = 0; + virBitmapPtr map = NULL; + + memset(cpumaps, 0, maplen * maxinfo); + if (parallelsNodeGetCpuMask(privdomdata, &map, hostcpus) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get cpu affinity mask ")); + goto cleanup; + } + virBitmapToData(map, &tmpmap, &tmpmapLen); + if (tmpmapLen > maplen) + tmpmapLen = maplen; + + for (v = 0; v < maxinfo; v++) { + unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, v); + memcpy(cpumap, tmpmap, tmpmapLen); + } + VIR_FREE(tmpmap); + virBitmapFree(map); + } + } + ret = maxinfo; + + cleanup: + if (privdom) + virObjectUnlock(privdom); + return ret; +} + + static virDriver parallelsDriver = { .no = VIR_DRV_PARALLELS, .name = "Parallels", @@ -2323,6 +2485,7 @@ static virDriver parallelsDriver = { .domainGetXMLDesc = parallelsDomainGetXMLDesc, /* 0.10.0 */ .domainIsPersistent = parallelsDomainIsPersistent, /* 0.10.0 */ .domainGetAutostart = parallelsDomainGetAutostart, /* 0.10.0 */ + .domainGetVcpus = parallelsDomainGetVcpus, /* 1.2.6 */ .domainSuspend = parallelsDomainSuspend, /* 0.10.0 */ .domainResume = parallelsDomainResume, /* 0.10.0 */ .domainDestroy = parallelsDomainDestroy, /* 0.10.0 */ diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h index 6215553..e88af1c 100644 --- a/src/parallels/parallels_utils.h +++ b/src/parallels/parallels_utils.h @@ -54,6 +54,7 @@ struct parallelsDomObj { int id; char *uuid; char *home; + char *cpumask; }; typedef struct parallelsDomObj *parallelsDomObjPtr; -- 1.7.1

On Mon, Jun 02, 2014 at 06:06:38PM +0400, Alexander Burluka wrote:
From: A.Burluka <aburluka@parallels.com>
OpenStack Nova requires this function to start VM instance. Cpumask info is obtained via prlctl utility.
Hmm, what error did you get from openstack ? The two uses of the 'dom.vcpus' function are both wrapped in try/except so that it is considered non-fatal if libvirt doesn't provide this.
Unlike KVM, Parallels Cloud Server is unable to set cpu affinity mask for every VCpu. Mask is unique for all VCpu. You can set it using 'prlctl set <vm_id|vm_name> --cpumask <{n[,n,n1-n2]|all}>' command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7' would set this mask to yy---yyy.
Are you talking about container based virt here or full machine based virt ? IIUC Parallels can do both ? With container based virt, does parallels have the concept of 'vcpus' at all ? We don't have that in LXC at least. I don't think it makes sense to support the virDomainGetVCPUs function if this is only about container virt. I'd be more inclined to fix openstack so it doesn't fail 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 Mon, Jun 02, 2014 at 06:06:38PM +0400, Alexander Burluka wrote:
From: A.Burluka <aburluka@parallels.com>
OpenStack Nova requires this function to start VM instance. Cpumask info is obtained via prlctl utility. Unlike KVM, Parallels Cloud Server is unable to set cpu affinity mask for every VCpu. Mask is unique for all VCpu. You can set it using 'prlctl set <vm_id|vm_name> --cpumask <{n[,n,n1-n2]|all}>' command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7' would set this mask to yy---yyy. --- src/parallels/parallels_driver.c | 169 +++++++++++++++++++++++++++++++++++++- src/parallels/parallels_utils.h | 1 + 2 files changed, 167 insertions(+), 3 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ab59599..f1d5ecc 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -108,6 +108,7 @@ parallelsDomObjFreePrivate(void *p) if (!pdom) return;
+ VIR_FREE(pdom->cpumask); VIR_FREE(pdom->uuid); VIR_FREE(pdom->home); VIR_FREE(p); @@ -654,6 +655,9 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) if (VIR_ALLOC(def) < 0) goto cleanup;
+ if (VIR_ALLOC(pdom) < 0) + goto cleanup; + def->virtType = VIR_DOMAIN_VIRT_PARALLELS; def->id = -1;
@@ -716,6 +720,17 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; }
+ if (!(tmp = virJSONValueObjectGetString(jobj3, "mask"))) { + /* Absence of this field means that all domains cpus are available */ + if (VIR_STRDUP(pdom->cpumask, "all") < 0) { + goto cleanup; + } + } else { + if (VIR_STRDUP(pdom->cpumask, tmp) < 0) { + goto cleanup; + } + }
See my comment later about using 'virBitmap' - this is where you shoudl parse the string into a bitmap instance.
+static int +parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata, + virBitmapPtr *cpumask, + int hostcpus) +{ + int ret = -1; + int cpunum = -1; + int prevcpunum = -1; + int offset = 0; + const char *it = privatedomdata->cpumask; + bool isrange = false; + size_t i; + int cpunums[512] = { 0 }; + size_t count = 0; + + if (STREQ(it, "all")) { + if (!(*cpumask = virBitmapNew(hostcpus))) + goto cleanup; + virBitmapSetAll(*cpumask); + } else { + while (sscanf(it, "%d%n", &cpunum, &offset)) { + char delim = 0; + if (isrange) { + for (i = prevcpunum + 1; i <= cpunum; ++i) { + cpunums[count++] = i; + } + } else { + cpunums[count++] = cpunum; + } + + it += offset; + + if (sscanf(it, "%c%n", &delim, &offset) == EOF) { + break;
What is the format that parallels uses for 'cpumask' - ideally you would just use 'virBitmapParse' for parsing it, not write another parser
+static int +parallelsDomainGetVcpus(virDomainPtr domain, + virVcpuInfoPtr info, + int maxinfo, + unsigned char *cpumaps, + int maplen)
Generally parameters should all be aligned with the first parameter
diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h index 6215553..e88af1c 100644 --- a/src/parallels/parallels_utils.h +++ b/src/parallels/parallels_utils.h @@ -54,6 +54,7 @@ struct parallelsDomObj { int id; char *uuid; char *home; + char *cpumask;
A while back we converted all 'char *cpumask' variables to be 'virBitmapPtr cpumask' since it makes the code nicer if we can actually have things in fully parsed format internally, only converting to string format when really needed. Conceptually this looks ok - main issue is using virBitmap for the internal data structure & changing the parser. 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 :|

From: A.Burluka <aburluka@parallels.com> Openstack Nova (starting at icehouse release) requires this function to start VM. Because the information is taken from host capabilities xml, I've expanded it and add cpu section in it. --- src/parallels/parallels_driver.c | 34 ++++++++++++++++++++++++++++++++-- 1 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index f1d5ecc..3aa73f0 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -51,6 +51,7 @@ #include "nodeinfo.h" #include "c-ctype.h" #include "virstring.h" +#include "cpu/cpu.h" #include "parallels_driver.h" #include "parallels_utils.h" @@ -117,7 +118,9 @@ parallelsDomObjFreePrivate(void *p) static virCapsPtr parallelsBuildCapabilities(void) { - virCapsPtr caps; + virCapsPtr caps = NULL; + virCPUDefPtr cpu = NULL; + virCPUDataPtr data = NULL; virCapsGuestPtr guest; if ((caps = virCapabilitiesNew(virArchFromHost(), @@ -147,11 +150,25 @@ parallelsBuildCapabilities(void) "parallels", NULL, NULL, 0, NULL) == NULL) goto error; + if (VIR_ALLOC(cpu) < 0) + goto error; + + cpu->arch = caps->host.arch; + cpu->type = VIR_CPU_TYPE_HOST; + caps->host.cpu = cpu; + + if (!(data = cpuNodeData(cpu->arch)) + || cpuDecode(cpu, data, NULL, 0, NULL) < 0) { + goto error; + } + + cleanup: + cpuDataFree(data); return caps; error: virObjectUnref(caps); - return NULL; + goto cleanup; } static char * @@ -2312,6 +2329,18 @@ static int parallelsConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) } +static char * +parallelsConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, + const char **xmlCPUs, + unsigned int ncpus, + unsigned int flags) +{ + virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL); + + return cpuBaselineXML(xmlCPUs, ncpus, NULL, 0, flags); +} + + static int parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata, virBitmapPtr *cpumask, @@ -2471,6 +2500,7 @@ static virDriver parallelsDriver = { .connectGetHostname = parallelsConnectGetHostname, /* 0.10.0 */ .nodeGetInfo = parallelsNodeGetInfo, /* 0.10.0 */ .connectGetCapabilities = parallelsConnectGetCapabilities, /* 0.10.0 */ + .connectBaselineCPU = parallelsConnectBaselineCPU, /* 1.2.6 */ .connectListDomains = parallelsConnectListDomains, /* 0.10.0 */ .connectNumOfDomains = parallelsConnectNumOfDomains, /* 0.10.0 */ .connectListDefinedDomains = parallelsConnectListDefinedDomains, /* 0.10.0 */ -- 1.7.1

On Mon, Jun 02, 2014 at 06:06:39PM +0400, Alexander Burluka wrote:
From: A.Burluka <aburluka@parallels.com>
Openstack Nova (starting at icehouse release) requires this function to start VM. Because the information is taken from host capabilities xml, I've expanded it and add cpu section in it.
Small nit-pick - it'd be nice to split this patch in 2. One patch doing virConnectBaselineCPU impl as $subject describes, and one patch including CPU info in the capabilities XML.
index f1d5ecc..3aa73f0 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -51,6 +51,7 @@ #include "nodeinfo.h" #include "c-ctype.h" #include "virstring.h" +#include "cpu/cpu.h"
#include "parallels_driver.h" #include "parallels_utils.h" @@ -117,7 +118,9 @@ parallelsDomObjFreePrivate(void *p) static virCapsPtr parallelsBuildCapabilities(void) { - virCapsPtr caps; + virCapsPtr caps = NULL; + virCPUDefPtr cpu = NULL; + virCPUDataPtr data = NULL; virCapsGuestPtr guest;
if ((caps = virCapabilitiesNew(virArchFromHost(), @@ -147,11 +150,25 @@ parallelsBuildCapabilities(void) "parallels", NULL, NULL, 0, NULL) == NULL) goto error;
+ if (VIR_ALLOC(cpu) < 0) + goto error; + + cpu->arch = caps->host.arch; + cpu->type = VIR_CPU_TYPE_HOST; + caps->host.cpu = cpu;
I think you should also fill in the sockets/cores/threads info while you're doing this, since openstack is likely to start using this data too in the near future. eg virNodeInfo nodeinfo; ... if (nodeGetInfo(&nodeinfo)) goto error; ... cpu->sockets = nodeinfo.sockets; cpu->cores = nodeinfo.cores; cpu->threads = nodeinfo.threads; 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 :|
participants (2)
-
Alexander Burluka
-
Daniel P. Berrange