9:25 p.m.
I submitted bug https://bugzilla.redhat.com/show_bug.cgi?id=1038363 for being unable to
raise the persistent mem/vcpu values above a live domain’s maxmem/maxvcpu values (rather
than the persistent maxmem/maxvcpu values). I was asked to submit my patch here for a
wider review.
For memory, check the newmem value against mem.max_baloon of the live def for
non-persistent changes, or mem.max_baloon of the persistent def for persistent changes.
Same theory for setting vcpus/maxvcpus.
--- a/libvirt0/libvirt-1.1.1/src/qemu/qemu_driver.c
+++ b/libvirt0/libvirt-1.1.1/src/qemu/qemu_driver.c
@@ -2236,13 +2236,13 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
} else {
/* resize the current memory */
- if (newmem > vm->def->mem.max_balloon) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("cannot set memory higher than max memory"));
- goto endjob;
- }
-
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+ if (newmem > vm->def->mem.max_balloon) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("cannot set memory higher than max memory"));
+ goto endjob;
+ }
+
priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetBalloon(priv->mon, newmem);
@@ -2262,6 +2262,12 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long
newmem,
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+ if (newmem > persistentDef->mem.max_balloon) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("cannot set memory higher than max memory"));
+ goto endjob;
+ }
+
sa_assert(persistentDef);
persistentDef->mem.cur_balloon = newmem;
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
@@ -4160,6 +4166,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virDomainDefPtr persistentDef;
int ret = -1;
bool maximum;
+ unsigned int maxvcpus;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
qemuAgentCPUInfoPtr cpuinfo = NULL;
@@ -4207,11 +4214,12 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
goto endjob;
}
- if (!maximum && nvcpus > vm->def->maxvcpus) {
+ maxvcpus = (flags & VIR_DOMAIN_AFFECT_LIVE) ? vm->def->maxvcpus :
persistentDef->maxvcpus;
+ if (!maximum && nvcpus > maxvcpus) {
virReportError(VIR_ERR_INVALID_ARG,
_("requested vcpus is greater than max allowable"
" vcpus for the domain: %d > %d"),
- nvcpus, vm->def->maxvcpus);
+ nvcpus, maxvcpus);
goto endjob;
}
11:24 a.m.
On 01/23/2014 08:25 PM, Geoff Franks wrote:
I submitted bug https://bugzilla.redhat.com/show_bug.cgi?id=1038363
for being unable to raise the persistent mem/vcpu values above a live domain’s
maxmem/maxvcpu values (rather than the persistent maxmem/maxvcpu values). I was asked to
submit my patch here for a wider review.
Thanks. You may also want to wrap long lines, and/or use 'git
send-email' to make the patches easier to use, but for a first time
submission, this is enough to let us start review.
For memory, check the newmem value against mem.max_baloon of the live def for
non-persistent changes, or mem.max_baloon of the persistent def for persistent changes.
Same theory for setting vcpus/maxvcpus.
--- a/libvirt0/libvirt-1.1.1/src/qemu/qemu_driver.c
+++ b/libvirt0/libvirt-1.1.1/src/qemu/qemu_driver.c
@@ -2236,13 +2236,13 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
} else {
/* resize the current memory */
- if (newmem > vm->def->mem.max_balloon) {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("cannot set memory higher than max memory"));
- goto endjob;
- }
I think I see what you are complaining about - in the old code, we don't
reject newmem > persistent max, and so we can end up failing halfway
through. But your new code isn't any better - even though you now check
against both limits, you can still fail halfway through.
I'd rather see us check both limits and reject the operation up front if
we can't meet both limits, than to lower the checks down into the
live/persistent branches but risk failure with live modified when the
persistent branch finally detects failure.
It also has the benefit of being a smaller diffstat:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index a7ef209..82d29c4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -2242,8 +2242,16 @@ static int qemuDomainSetMemoryFlags(virDomainPtr
dom, unsigned long newmem,
} else {
/* resize the current memory */
+ unsigned long oldmax = 0;
- if (newmem > vm->def->mem.max_balloon) {
+ if (flags & VIR_DOMAIN_AFFECT_LIVE)
+ oldmax = vm->def->mem.max_balloon;
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+ if (!oldmax || oldmax > persistentDef->mem.max_balloon)
+ oldmax = persistentDef->mem.max_balloon;
+ }
+
+ if (newmem > oldmax) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("cannot set memory higher than max memory"));
goto endjob;
@@ -4207,11 +4214,12 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
unsigned int nvcpus,
goto endjob;
}
- if (!maximum && nvcpus > vm->def->maxvcpus) {
+ maxvcpus = (flags & VIR_DOMAIN_AFFECT_LIVE) ? vm->def->maxvcpus :
persistentDef->maxvcpus;
Long line. Also, I think it suffers from the same problem - if both
_LIVE and _CONFIG are given at once, then we want to cap things at the
lower of the two limits, rather than using just the live max as the limit.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:55 p.m.
On Jan 24, 2014, at 12:24 PM, Eric Blake <eblake(a)redhat.com> wrote:
Thanks. You may also want to wrap long lines, and/or use 'git
send-email' to make the patches easier to use, but for a first time
submission, this is enough to let us start review.
Thanks, I’ll keep that in mind for next time.
I'd rather see us check both limits and reject the operation up
front if
we can't meet both limits, than to lower the checks down into the
live/persistent branches but risk failure with live modified when the
persistent branch finally detects failure.
It also has the benefit of being a smaller diffstat:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index a7ef209..82d29c4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -2242,8 +2242,16 @@ static int qemuDomainSetMemoryFlags(virDomainPtr
dom, unsigned long newmem,
} else {
/* resize the current memory */
+ unsigned long oldmax = 0;
- if (newmem > vm->def->mem.max_balloon) {
+ if (flags & VIR_DOMAIN_AFFECT_LIVE)
+ oldmax = vm->def->mem.max_balloon;
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+ if (!oldmax || oldmax > persistentDef->mem.max_balloon)
+ oldmax = persistentDef->mem.max_balloon;
+ }
+
+ if (newmem > oldmax) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("cannot set memory higher than max memory"));
goto endjob;
> @@ -4207,11 +4214,12 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int
nvcpus,
> goto endjob;
> }
>
> - if (!maximum && nvcpus > vm->def->maxvcpus) {
> + maxvcpus = (flags & VIR_DOMAIN_AFFECT_LIVE) ? vm->def->maxvcpus :
persistentDef->maxvcpus;
Long line. Also, I think it suffers from the same problem - if both
_LIVE and _CONFIG are given at once, then we want to cap things at the
lower of the two limits, rather than using just the live max as the limit.
This looks like it would work for my purposes.
3988
days inactive
3989
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Geoff Franks