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 > >
