[libvirt] [PATCH v2 1/2] LXC: add support for persistent config in lxcDomainSetMemoryFlags

[John Ferlan]

On 07/30/2014 11:41 PM, Chen Hanxiao wrote:
> Currently, setmaxmem return success on an active
> domain, but nothing happened.

Not quite true... Prior to these changes...

# virsh -c lxc:/// start vm1
# virsh -c lxc:/// dominfo vm1 | grep memory
Max memory:     500000 KiB
Used memory:    776 KiB
# virsh -c lxc:/// dumpxml vm1 | grep mem
  <memory unit='KiB'>500000</memory>
# cat /sys/fs/cgroup/memory/machine.slice/*/memory.limit_in_bytes
512000000
# virsh -c lxc:/// setmaxmem vm1 525000

# virsh -c lxc:/// dominfo vm1 | grep memory
Max memory:     525000 KiB
Used memory:    776 KiB
# virsh -c lxc:/// dumpxml vm1 | grep mem
  <memory unit='KiB'>525000</memory>
# cat /sys/fs/cgroup/memory/machine.slice/*/memory.limit_in_bytes
512000000
# virsh -c lxc:/// console vm1
Connected to domain vm1
Escape character is ^]
sh-4.2# grep Mem /proc/meminfo
MemTotal:         500000 kB
MemFree:          499076 kB
MemAvailable:    3629632 kB
#


Now I'll agree that technically what one expected to happen didn't since
it's not supported, but something did happen :-)... And of course after
a destroy the maxmem went back to 500000.


> This patch will disable this behaviour,
> also add support persistent config.
> And it will be used in a later patch.

FWIW:
I agree with Peter's observation from your v1 - the two patches should
be one as it is clearer that way what you are doing and leaves no doubt
why the flag is used

I do see that other drivers like qemu & libxl use the MEM_MAXIMUM flag.


So perhaps said differently -

"This patch changes the LXC driver setmaxmem function to support the
'--live', '--config', and '--current' flags by revectoring the code
through the setmem function using the VIR_DOMAIN_MEM_MAXIMUM flag.  The
setmem code is refactored to handle both cases depending on the flag.

The changed maxmem code for the MEM_MAXIMUM path will not allow
modification to the memory values of an active guest unless the --config
switch is used."


> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> ---
> v2: disable changing max memory on an active domain
>     drop useless as_assert
> 
>  src/lxc/lxc_driver.c | 64 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 9e12ecc..d99ab3b 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -721,10 +721,10 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
>      virLXCDomainObjPrivatePtr priv;
>      virLXCDriverPtr driver = dom->conn->privateData;
>      virLXCDriverConfigPtr cfg = NULL;
> -    unsigned long oldmax = 0;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> -                  VIR_DOMAIN_AFFECT_CONFIG, -1);
> +                  VIR_DOMAIN_AFFECT_CONFIG |
> +                  VIR_DOMAIN_MEM_MAXIMUM, -1);

>  
>      if (!(vm = lxcDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -743,32 +743,50 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
>                                          &persistentDef) < 0)
>          goto cleanup;
>  

I see the next set of changes is essentially cut-n-pasted from qemu,
although the changes after the else are the same as the former setmem
function - it's just the git diff that obfuscates that.

The changes do work as I'd expect, so ACK from that point of view.

I can merge the two patches for you and commit, but I'll give it a bit
to make sure no one else has concerns

John

> -    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 (flags & VIR_DOMAIN_MEM_MAXIMUM) {
> +        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                           _("Cannot resize the max memory "
> +                             "on an active domain"));
> +            goto cleanup;
> +        }
>  
> -    if (newmem > oldmax) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       "%s", _("Cannot set memory higher than max memory"));
> -        goto cleanup;
> -    }
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +            persistentDef->mem.max_balloon = newmem;
> +            if (persistentDef->mem.cur_balloon > newmem)
> +                persistentDef->mem.cur_balloon = newmem;
> +            if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +                goto cleanup;
> +        }
> +    } else {
> +        unsigned long oldmax = 0;
>  
> -    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> -        if (virCgroupSetMemory(priv->cgroup, newmem) < 0) {
> -            virReportError(VIR_ERR_OPERATION_FAILED,
> -                           "%s", _("Failed to set memory for domain"));
> -            goto cleanup;
> +        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 (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> -        sa_assert(persistentDef);
> -        persistentDef->mem.cur_balloon = newmem;
> -        if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +        if (newmem > oldmax) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           "%s", _("Cannot set memory higher than max memory"));
>              goto cleanup;
> +        }
> +
> +        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +            if (virCgroupSetMemory(priv->cgroup, newmem) < 0) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               "%s", _("Failed to set memory for domain"));
> +                goto cleanup;
> +            }
> +        }
> +
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +            persistentDef->mem.cur_balloon = newmem;
> +            if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +                goto cleanup;
> +        }
>      }
>  
>      ret = 0;
>