[libvirt] [PATCH] Make virsh setmaxmem balloon only when successful.
After playing around with virsh setmaxmem for a bit, I ran into some surprising behavior; if a hypervisor does not support the virDomainSetMaxMemory() API, but the value specified for setmaxmem is less than the current amount of memory in the domain, the domain would be ballooned down *before* an error was reported. To make this more consistent, run virDomainSetMaxMemory() before trying to shrink; that way, if an error is thrown, no changes to the running domain are made. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- tools/virsh.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 0631590..97bfa20 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2529,19 +2529,19 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) return FALSE; } + if (virDomainSetMaxMemory(dom, kilobytes) != 0) { + vshError(ctl, "%s", _("Unable to change MaxMemorySize")); + virDomainFree(dom); + return FALSE; + } + if (kilobytes < info.memory) { if (virDomainSetMemory(dom, kilobytes) != 0) { - virDomainFree(dom); vshError(ctl, "%s", _("Unable to shrink current MemorySize")); - return FALSE; + ret = FALSE; } } - if (virDomainSetMaxMemory(dom, kilobytes) != 0) { - vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - ret = FALSE; - } - virDomainFree(dom); return ret; } -- 1.6.6.1
On 07/07/10 - 05:35:39PM, Chris Lalancette wrote:
After playing around with virsh setmaxmem for a bit, I ran into some surprising behavior; if a hypervisor does not support the virDomainSetMaxMemory() API, but the value specified for setmaxmem is less than the current amount of memory in the domain, the domain would be ballooned down *before* an error was reported.
To make this more consistent, run virDomainSetMaxMemory() before trying to shrink; that way, if an error is thrown, no changes to the running domain are made.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
ping, any review on this?
--- tools/virsh.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 0631590..97bfa20 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2529,19 +2529,19 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) return FALSE; }
+ if (virDomainSetMaxMemory(dom, kilobytes) != 0) { + vshError(ctl, "%s", _("Unable to change MaxMemorySize")); + virDomainFree(dom); + return FALSE; + } + if (kilobytes < info.memory) { if (virDomainSetMemory(dom, kilobytes) != 0) { - virDomainFree(dom); vshError(ctl, "%s", _("Unable to shrink current MemorySize")); - return FALSE; + ret = FALSE; } }
- if (virDomainSetMaxMemory(dom, kilobytes) != 0) { - vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - ret = FALSE; - } - virDomainFree(dom); return ret; } -- 1.6.6.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- -- Chris Lalancette
After playing around with virsh setmaxmem for a bit, I ran into some surprising behavior; if a hypervisor does not support the virDomainSetMaxMemory() API, but the value specified for setmaxmem is less than the current amount of memory in the domain, the domain would be ballooned down *before* an error was reported.
To make this more consistent, run virDomainSetMaxMemory() before trying to shrink; that way, if an error is thrown, no changes to the running domain are made.
Well, I feel a bit uneasy about setmaxmem command. The manpage says "This should not change the current memory use" so the question is whether we need to do anything else than just setting max memory. On the other hand, it makes sense that if setmaxmem is used for reducing maximum amount of memory, we should make sure domain's current memory isn't bigger than the maximum.
diff --git a/tools/virsh.c b/tools/virsh.c index 0631590..97bfa20 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2529,19 +2529,19 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) return FALSE; }
+ if (virDomainSetMaxMemory(dom, kilobytes) != 0) { + vshError(ctl, "%s", _("Unable to change MaxMemorySize")); + virDomainFree(dom); + return FALSE; + } + if (kilobytes < info.memory) { if (virDomainSetMemory(dom, kilobytes) != 0) { - virDomainFree(dom); vshError(ctl, "%s", _("Unable to shrink current MemorySize")); - return FALSE; + ret = FALSE; } }
- if (virDomainSetMaxMemory(dom, kilobytes) != 0) { - vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - ret = FALSE; - } - virDomainFree(dom); return ret; }
But when we decide to do that and shrink current memory in case it's bigger than the new maximum, what should we do if the shrinking fails? The command would fail while the maximum amount of memory for the domain was in fact updated correctly. Perhaps we should rollback and restore the old maximum if shrinking fails? I think we need to either fix virsh man page or the code or even both. Personally, I'd vote for removing SetMemory call from setmaxmem command and just doing what man page says but I'm not sure we can do that since people may rely on current behavior even though it differs from what's documented. Anyway, the behavior with your patch is much better than before, so ACK to the patch. We can discuss and fix the rest in a separate patch. Jirka
On 07/20/10 - 09:38:45AM, Jiri Denemark wrote:
But when we decide to do that and shrink current memory in case it's bigger than the new maximum, what should we do if the shrinking fails? The command would fail while the maximum amount of memory for the domain was in fact updated correctly. Perhaps we should rollback and restore the old maximum if shrinking fails?
I think we need to either fix virsh man page or the code or even both.
Personally, I'd vote for removing SetMemory call from setmaxmem command and just doing what man page says but I'm not sure we can do that since people may rely on current behavior even though it differs from what's documented.
Yes, this command is a bit sticky with respect to handling errors. First, I don't think that we can change the current behavior; it *is* logical, and like you say, I think people will probably be depending on it. Second, I think we should probably do like you say and if the shrink fails, then rollback the max change. Of course the rollback could fail, in which case everything is confused, but I think that will be a much rarer situation than the ballooning failing. I'll put this on my TODO list.
Anyway, the behavior with your patch is much better than before, so ACK to the patch. We can discuss and fix the rest in a separate patch.
Thanks, I've pushed this now. -- Chris Lalancette
participants (2)
-
Chris Lalancette -
Jiri Denemark