All the code listed here works well and this would be an LGTM, but there is the question of whether we should keep the memory cgroup optional / how to behave if it is disabled.
What I find rather curious here is that ballooning / setting the runtime memory returned quietly rather than raising an error earlier. If we keep the memory cgroup optional, we should throw an error at the beginning if it is not enabled. Let's discuss this during the VC! Apart from that, nitpicks ahoy. On Mon, Aug 18, 2014 at 3:38 AM, Yuto KAWAMURA(kawamuray) < [email protected]> wrote: > This patch implements BalloonInstanceMemory function of the > the BalloonInstanceMemory functionality > LXCHypervisor. > Memory ballooning for the LXC container can be done by using > memory.limit_in_bytes and memory.memsw.limit_in_bytes cgroup parameter. > the ... parameters > The Linux kernel does not kill processes inside the cgroup which is > tried to shrink its memory accounted, so the update for > Possible rewrite (not sure about the accounted part): ... the cgroup whose accounted memory we are shrinking, so ... > memory.memsw.limit_in_bytes can be fail even if the update for > s/be// > memory.limit_in_bytes was succeed, so this function must care about all cgroup memory limit parameters consistency for update. > s/succeed/successful/ ... must care that all cgroup memory limit parameters are consistently updated. > > Signed-off-by: Yuto KAWAMURA(kawamuray) <[email protected]> > --- > lib/hypervisor/hv_lxc.py | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/lib/hypervisor/hv_lxc.py b/lib/hypervisor/hv_lxc.py > index e94862f..8594628 100644 > --- a/lib/hypervisor/hv_lxc.py > +++ b/lib/hypervisor/hv_lxc.py > @@ -23,6 +23,7 @@ > > """ > > +import errno > import os > import os.path > import logging > @@ -749,8 +750,35 @@ class LXCHypervisor(hv_base.BaseHypervisor): > @param mem: actual memory size to use for instance runtime > > """ > - # Currently lxc instances don't have memory limits > - pass > + mem_in_bytes = mem * 1024 ** 2 > + current_mem_usage = self._GetCgroupMemoryLimit(instance.name) > + is_shrink = mem_in_bytes <= current_mem_usage > I hate to be suggesting only English-related changes, but how about changing is_shrink into shrinking? > + > + # memory.memsw.limit_in_bytes is the superlimit of the > memory.limit_in_bytes > + # so the order of setting these parameters is quite important. > + cgparams = ["memory.memsw.limit_in_bytes", "memory.limit_in_bytes"] > + if is_shrink: > + cgparams.reverse() > + > + for i, cgparam in enumerate(cgparams): > + try: > + self._SetCgroupInstanceValue(instance.name, cgparam, > str(mem_in_bytes)) > + except EnvironmentError, err: > + if is_shrink and err.errno == errno.EBUSY: > + logging.warn("Unable to reclaim memory or swap usage from > instance" > + " %s", instance.name) > + # Restore changed parameters for an atomicity > + for restore_param in cgparams[0:i]: > + try: > + self._SetCgroupInstanceValue(instance.name, restore_param, > + str(current_mem_usage)) > + except EnvironmentError, restore_err: > + logging.warn("Can't restore the cgroup parameter %s of %s: > %s", > + restore_param, instance.name, restore_err) > + > + raise HypervisorError("Failed to balloon the memory of %s, can't > set" > + " cgroup parameter %s: %s" % > + (instance.name, cgparam, err)) > > def GetNodeInfo(self, hvparams=None): > """Return information about the node. > -- > 2.0.4 > > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
