2014-08-22 2:10 GMT+09:00 Hrvoje Ribicic <[email protected]>: > 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! > As we discussed in the VC, creating a container without the memory limit isn't a good idea. Because it means one container can eat the whole physical memory on the machine and it is really evil behavior. I agree with setting up initial time checking so I will going to make a commit before this one.
> 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
