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

2014-08-12 Thread 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: 50 KiB
Used memory:776 KiB
# virsh -c lxc:/// dumpxml vm1 | grep mem
  memory unit='KiB'50/memory
# cat /sys/fs/cgroup/memory/machine.slice/*/memory.limit_in_bytes
51200
# 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
51200
# virsh -c lxc:/// console vm1
Connected to domain vm1
Escape character is ^]
sh-4.2# grep Mem /proc/meminfo
MemTotal: 50 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 50.


 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 chenhanx...@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  

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

2014-07-30 Thread Chen Hanxiao
Currently, setmaxmem return success on an active
domain, but nothing happened.
This patch will disable this behaviour,
also add support persistent config.
And it will be used in a later patch.

Signed-off-by: Chen Hanxiao chenhanx...@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;
 
-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;
-- 
1.9.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list