After a kind explanation, I better understand and agree with this patch now. LGTM!
On Mon, Jul 28, 2014 at 6:39 PM, Hrvoje Ribicic <[email protected]> wrote: > Some nitpicks later on, about naming and so on, but some more general > questions here: > > We already pose a requirement in the code that we need to have the cpuset, > devices, and memory (optional outside of GetInstanceInfo) subsystems > enabled. > Memory is probably the only one with a chance of not being enabled on > common configurations. > > With this patch, _EnsureCgroupMounts would not be very useful, as we would > mount the available subsystems (e.g. cpuset, devices), and crash later, > probably when doing a GetInstanceInfo. > This seems contrary to the spirit of the function. > > Also, in which situations would it be useful for the user to set the > required cgroup subsystems? > > > On Thu, Jul 24, 2014 at 2:32 AM, Yuto KAWAMURA(kawamuray) < > [email protected]> wrote: > >> Cgroup kernel subsystems are need to be mounted before lxc-start is >> > > s/are// > > >> executed. >> _EnsureCgroupMounts method ensures all cgroup subsystems in the >> > > The ... in /proc/cgroups > > >> /proc/cgroups are mounted. >> New hvparam lxc_cgroup_use is added to overwrite this prerequired >> > > The new lxc_cgroup_use hvparam is added to overwrite the prerequisite > > Not a nitpick: I would prefer it the name of this parameter would reflect > its use better: > How about lxc_required_subsystems? You can leave out cgroup in the name as > LXC is already there :) > > subsystems list. > > Also, since _GetCgroupMountPoint was changed to function just returns a >> > > ... to a function which returns just ... > > >> constant, calling it in Verify method is meaningless now. >> > > ... in the ... > > >> Instead, calling _EnsureCgroupMounts in Verify can be check the actual >> > > ... can be used to check ... > or > ... checks ... > > >> cgroup mount requirements. >> >> Signed-off-by: Yuto KAWAMURA(kawamuray) <[email protected]> >> --- >> lib/hypervisor/hv_lxc.py | 40 >> +++++++++++++++++++++++++--- >> man/gnt-instance.rst | 11 ++++++++ >> src/Ganeti/Constants.hs | 5 ++++ >> test/py/ganeti.hypervisor.hv_lxc_unittest.py | 2 +- >> 4 files changed, 53 insertions(+), 5 deletions(-) >> >> diff --git a/lib/hypervisor/hv_lxc.py b/lib/hypervisor/hv_lxc.py >> index 8e4ddce..3a0cccf 100644 >> --- a/lib/hypervisor/hv_lxc.py >> +++ b/lib/hypervisor/hv_lxc.py >> @@ -55,7 +55,8 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> """ >> _ROOT_DIR = pathutils.RUN_DIR + "/lxc" >> _CGROUP_ROOT_DIR = _ROOT_DIR + "/cgroup" >> - _PROC_CGROUP_FILE = "/proc/self/cgroup" >> + _PROC_CGROUPS_FILE = "/proc/cgroups" >> + _PROC_SELF_CGROUP_FILE = "/proc/self/cgroup" >> >> _DEVS = [ >> "c 1:3", # /dev/null >> @@ -81,6 +82,7 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> >> PARAMETERS = { >> constants.HV_CPU_MASK: hv_base.OPT_CPU_MASK_CHECK, >> + constants.HV_LXC_CGROUP_USE: hv_base.NO_CHECK, >> constants.HV_LXC_STARTUP_WAIT: hv_base.OPT_NONNEGATIVE_INT_CHECK, >> } >> >> @@ -246,10 +248,10 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> >> """ >> try: >> - cgroup_list = utils.ReadFile(cls._PROC_CGROUP_FILE) >> + cgroup_list = utils.ReadFile(cls._PROC_SELF_CGROUP_FILE) >> except EnvironmentError, err: >> raise HypervisorError("Failed to read %s : %s" % >> - (cls._PROC_CGROUP_FILE, err)) >> + (cls._PROC_SELF_CGROUP_FILE, err)) >> >> cgroups = {} >> for line in filter(None, cgroup_list.split("\n")): >> @@ -466,6 +468,32 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> return "\n".join(out) + "\n" >> >> @classmethod >> + def _GetCgroupEnabledKernelSubsystems(cls): >> + """Return cgroup subsystems list that are enabled in current kernel. >> + >> + """ >> + try: >> + subsys_table = utils.ReadFile(cls._PROC_CGROUPS_FILE) >> + except EnvironmentError, err: >> + raise HypervisorError("Failed to read cgroup info from %s: %s" >> + % (cls._PROC_CGROUPS_FILE, err)) >> + return [x.split(None, 1)[0] for x in subsys_table.split("\n") >> + if x and not x.startswith("#")] >> + >> + @classmethod >> + def _EnsureCgroupMounts(cls, hvparams=None): >> + """Ensures all cgroup subsystems required to run LXC container are >> mounted. >> + >> + """ >> + if hvparams is None or not hvparams[constants.HV_LXC_CGROUP_USE]: >> + enable_subsystems = cls._GetCgroupEnabledKernelSubsystems() >> + else: >> + enable_subsystems = >> hvparams[constants.HV_LXC_CGROUP_USE].split(",") >> + >> + for subsystem in enable_subsystems: >> + cls._GetOrPrepareCgroupSubsysMountPoint(subsystem) >> + >> + @classmethod >> def _PrepareInstanceRootFsBdev(cls, storage_path, stash): >> """Return mountable path for storage_path. >> >> @@ -545,6 +573,10 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> >> """ >> stash = {} >> + >> + # Mount all cgroup fs required to run LXC >> + self._EnsureCgroupMounts(instance.hvparams) >> + >> root_dir = self._InstanceDir(instance.name) >> try: >> utils.EnsureDirs([(root_dir, self._DIR_MODE)]) >> @@ -678,7 +710,7 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> self._ROOT_DIR) >> >> try: >> - self._GetCgroupMountPoint() >> + self._EnsureCgroupMounts(hvparams) >> except errors.HypervisorError, err: >> msgs.append(str(err)) >> >> diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst >> index d74b3c5..27af30d 100644 >> --- a/man/gnt-instance.rst >> +++ b/man/gnt-instance.rst >> @@ -879,6 +879,17 @@ lxc\_startup\_wait >> >> It is set to ``30`` by default. >> >> +lxc\_cgroup\_use >> + Valid for the LXC hypervisor. >> + >> + This option specifies the list of subsystems that are required to >> + ensure mounted before starting LXC container. >> > > ... need to be mounted ... containers. > > >> + Value of this parameter should be a list of cgroup subsystem >> > > The value ... subsystems > > >> + separated by a comma(e.g, "cpuset,devices,memory") >> > > e.g., > Period at the end. > > >> + >> + If this parameter is not specified, list will be build from info >> > > ... a list will be built ... > > >> + of /proc/cgroups. >> > > s/of/in/ > > >> + >> The ``-O (--os-parameters)`` option allows customisation of the OS >> parameters. The actual parameter names and values depend on the OS being >> used, but the syntax is the same key=value. For example, setting a >> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs >> index 965cb4a..e76ab7f 100644 >> --- a/src/Ganeti/Constants.hs >> +++ b/src/Ganeti/Constants.hs >> @@ -1648,6 +1648,9 @@ hvKvmUserShutdown = "user_shutdown" >> hvLxcStartupWait :: String >> hvLxcStartupWait = "lxc_startup_wait" >> >> +hvLxcCgroupUse :: String >> +hvLxcCgroupUse = "lxc_cgroup_use" >> + >> hvMemPath :: String >> hvMemPath = "mem_path" >> >> @@ -1810,6 +1813,7 @@ hvsParameterTypes = Map.fromList >> , (hvKvmSpiceZlibGlzImgCompr, VTypeString) >> , (hvKvmUseChroot, VTypeBool) >> , (hvKvmUserShutdown, VTypeBool) >> + , (hvLxcCgroupUse, VTypeString) >> , (hvLxcStartupWait, VTypeInt) >> , (hvMemPath, VTypeString) >> , (hvMigrationBandwidth, VTypeInt) >> @@ -3891,6 +3895,7 @@ hvcDefaults = >> , (Chroot, Map.fromList [(hvInitScript, PyValueEx "/ganeti-chroot")]) >> , (Lxc, Map.fromList >> [ (hvCpuMask, PyValueEx "") >> + , (hvLxcCgroupUse, PyValueEx "") >> , (hvLxcStartupWait, PyValueEx (30 :: Int)) >> ]) >> ] >> diff --git a/test/py/ganeti.hypervisor.hv_lxc_unittest.py b/test/py/ >> ganeti.hypervisor.hv_lxc_unittest.py >> index 005378f..63f9711 100755 >> --- a/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> +++ b/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> @@ -130,7 +130,7 @@ class TestCgroupReadData(unittest.TestCase): >> def testGetCgroupMountPoint(self): >> self.assertEqual(self.hv._GetCgroupMountPoint(), self.cgroot) >> >> - @patch_object(LXCHypervisor, "_PROC_CGROUP_FILE", >> + @patch_object(LXCHypervisor, "_PROC_SELF_CGROUP_FILE", >> testutils.TestDataFilename("proc_cgroup.txt")) >> def testGetCurrentCgroupSubsysGroups(self): >> expected_groups = { >> -- >> 1.8.5.5 >> >> >
