Thanks, but for publicness, let me paste some note about this topic here. Since LXC version >= 1.0.0, the LXC strictly requires all cgroup subsystems mounted before starting a container. Users can control the list of desired cgroup subsystems for LXC containers by specifying lxc.cgroup.use parameter in LXC system configuration file, and its default value is "@kernel" which means all cgroup kernel subsystems. The LXC hypervisor of Ganeti should mount all cgroup subsystems needed to start a LXC container, and this is why this commit is effective.
2014-07-30 3:04 GMT+09:00 Hrvoje Ribicic <[email protected]>: > 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 >>> >> >
