> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list