Thanks for your kind reviews. I'll apply them and re-send the whole patches :)
2014-07-29 1:54 GMT+09:00 Hrvoje Ribicic <[email protected]>: > Nitpicks present, otherwise LGTM! > > On Thu, Jul 24, 2014 at 2:32 AM, Yuto KAWAMURA(kawamuray) > <[email protected]> wrote: >> >> Now LXC hypervisor requires LXC >= 1.0.0 for all commands used by it. > > > The LXC hypervisor now requires ... > >> >> Adding _VerifyLXCCommands() to provide checking for each commands > > > This patch adds ... which checks for the existence of each command and > satisfactory version. > >> >> existence and version satisfaction. >> >> Signed-off-by: Yuto KAWAMURA(kawamuray) <[email protected]> >> --- >> lib/hypervisor/hv_lxc.py | 66 >> ++++++++++++++++++++++++++++ >> test/py/ganeti.hypervisor.hv_lxc_unittest.py | 50 +++++++++++++++++++++ >> 2 files changed, 116 insertions(+) >> >> diff --git a/lib/hypervisor/hv_lxc.py b/lib/hypervisor/hv_lxc.py >> index 3e1269e..f7c54c6 100644 >> --- a/lib/hypervisor/hv_lxc.py >> +++ b/lib/hypervisor/hv_lxc.py >> @@ -27,6 +27,7 @@ import os >> import os.path >> import logging >> import sys >> +import re >> >> from ganeti import constants >> from ganeti import errors # pylint: disable=W0611 >> @@ -59,6 +60,15 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> _PROC_CGROUPS_FILE = "/proc/cgroups" >> _PROC_SELF_CGROUP_FILE = "/proc/self/cgroup" >> >> + _LXC_MIN_VERSION_REQUIRED = "1.0.0" >> + _LXC_COMMANDS_REQUIRED = [ >> + "lxc-console", >> + "lxc-ls", >> + "lxc-start", >> + "lxc-stop", >> + "lxc-wait", >> + ] >> + >> _DEVS = [ >> "c 1:3", # /dev/null >> "c 1:5", # /dev/zero >> @@ -87,6 +97,9 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> constants.HV_LXC_STARTUP_WAIT: hv_base.OPT_NONNEGATIVE_INT_CHECK, >> } >> >> + # Let beta version following micro version, but don't care about it >> + _LXC_VERSION_RE = re.compile(r"^(\d+)\.(\d+)\.(\d+)") >> + >> def __init__(self): >> hv_base.BaseHypervisor.__init__(self) >> utils.EnsureDirs([ >> @@ -699,6 +712,57 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> user=constants.SSH_CONSOLE_USER, >> command=["lxc-console", "-n", >> instance.name]) >> >> + @classmethod >> + def _ParseLXCVersion(cls, version_string): >> + """Return a parsed result of lxc version string. >> + >> + @return tuple of major, minor and micro version number >> + @rtype tuple(int, int, int) >> + >> + """ >> + match = cls._LXC_VERSION_RE.match(version_string) >> + return tuple(map(int, match.groups())) if match else None >> + >> + @classmethod >> + def _VerifyLXCCommands(cls): >> + """Verify the validity of lxc command line tools. >> + >> + """ > > > Please document the return result with at least a @rtype declaration. > >> >> + version_required = >> cls._ParseLXCVersion(cls._LXC_MIN_VERSION_REQUIRED) >> + msgs = [] >> + for cmd in cls._LXC_COMMANDS_REQUIRED: >> + try: >> + # lxc-ls needs special checking procedure. >> + # there is two different version of lxc-ls, one is written in >> python > > > s/is/are/ > >> >> + # and the other is written in shell script. >> + # we have to ensure python version of lxc-ls command is >> installed. > > > ... the python ... > > the lxc-ls command > or > lxc-ls > >> >> + if cmd == "lxc-ls": >> + help_string = utils.RunCmd(["lxc-ls", "--help"]).output >> + if "--running" not in help_string: >> + # shell script version has no --running switch >> + msgs.append("Python version of 'lxc-ls' command is required." > > > The Python ... > >> >> + " Maybe lxc was installed without >> --enable-python") >> + else: >> + result = utils.RunCmd([cmd, "--version"]) >> + if result.failed: >> + msgs.append("Can't get version info from %s: %s" % >> + (cmd, result.output)) >> + else: >> + version_str = result.stdout.strip() >> + version = cls._ParseLXCVersion(version_str) >> + if version: >> + if version < version_required: >> + msgs.append("LXC version >= %s is required but command %s >> has" >> + " version %s" % >> + (cls._LXC_MIN_VERSION_REQUIRED, cmd, >> version_str)) >> + else: >> + msgs.append("Can't parse version info from %s output: %s" % >> + (cmd, version_str)) >> + except errors.OpExecError: >> + msgs.append("Required command %s not found" % cmd) >> + >> + return msgs >> + >> def Verify(self, hvparams=None): >> """Verify the hypervisor. >> >> @@ -721,6 +785,8 @@ class LXCHypervisor(hv_base.BaseHypervisor): >> except errors.HypervisorError, err: >> msgs.append(str(err)) >> >> + msgs.extend(self._VerifyLXCCommands()) >> + >> return self._FormatVerifyResults(msgs) >> >> @classmethod >> diff --git a/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> b/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> index 63f9711..42b25ba 100755 >> --- a/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> +++ b/test/py/ganeti.hypervisor.hv_lxc_unittest.py >> @@ -24,6 +24,7 @@ >> import unittest >> >> from ganeti import constants >> +from ganeti import errors >> from ganeti import objects >> from ganeti import hypervisor >> from ganeti import utils >> @@ -173,5 +174,54 @@ class TestCgroupReadData(unittest.TestCase): >> getval_mock.return_value = "128" >> self.assertEqual(self.hv._GetCgroupMemoryLimit("instance1"), 128) >> > > \n > >> >> +class TestVerifyLXCCommands(unittest.TestCase): >> + def setUp(self): >> + runcmd_mock = mock.Mock(return_value="") >> + self.RunCmdPatch = patch_object(utils, "RunCmd", runcmd_mock) >> + self.RunCmdPatch.start() >> + version_patch = patch_object(LXCHypervisor, >> "_LXC_MIN_VERSION_REQUIRED", >> + "1.2.3") >> + self._LXC_MIN_VERSION_REQUIRED_Patch = version_patch >> + self._LXC_MIN_VERSION_REQUIRED_Patch.start() >> + self.hvc = LXCHypervisor >> + >> + def tearDown(self): >> + self.RunCmdPatch.stop() >> + self._LXC_MIN_VERSION_REQUIRED_Patch.stop() >> + >> + def testParseLXCVersion(self): >> + self.assertEqual(self.hvc._ParseLXCVersion("1.0.0"), (1, 0, 0)) >> + self.assertEqual(self.hvc._ParseLXCVersion("1.0.0.alpha1"), (1, 0, >> 0)) >> + self.assertEqual(self.hvc._ParseLXCVersion("1.0"), None) >> + self.assertEqual(self.hvc._ParseLXCVersion("1.2a.0"), None) >> + >> + @patch_object(LXCHypervisor, "_LXC_COMMANDS_REQUIRED", ["lxc-stop"]) >> + def testCommandVersion(self): >> + utils.RunCmd.return_value = RunResultOk("1.2.3\n") >> + self.assertFalse(self.hvc._VerifyLXCCommands()) >> + utils.RunCmd.return_value = RunResultOk("1.10.0\n") >> + self.assertFalse(self.hvc._VerifyLXCCommands()) >> + utils.RunCmd.return_value = RunResultOk("1.2.2\n") >> + self.assertTrue(self.hvc._VerifyLXCCommands()) >> + >> + @patch_object(LXCHypervisor, "_LXC_COMMANDS_REQUIRED", ["lxc-stop"]) >> + def testCommandVersionInvalid(self): >> + utils.RunCmd.return_value = utils.RunResult(1, None, "", "", [], >> None, None) >> + self.assertTrue(self.hvc._VerifyLXCCommands()) >> + utils.RunCmd.return_value = RunResultOk("1.2a.0\n") >> + self.assertTrue(self.hvc._VerifyLXCCommands()) >> >> + >> + @patch_object(LXCHypervisor, "_LXC_COMMANDS_REQUIRED", ["lxc-stop"]) >> + def testCommandNotExists(self): >> + utils.RunCmd.side_effect = errors.OpExecError >> + self.assertTrue(self.hvc._VerifyLXCCommands()) >> + >> + @patch_object(LXCHypervisor, "_LXC_COMMANDS_REQUIRED", ["lxc-ls"]) >> + def testVerifyLXCLs(self): >> + utils.RunCmd.return_value = >> RunResultOk("garbage\n--running\ngarbage") >> + self.assertFalse(self.hvc._VerifyLXCCommands()) >> >> + utils.RunCmd.return_value = RunResultOk("foo") >> + self.assertTrue(self.hvc._VerifyLXCCommands()) >> >> + >> if __name__ == "__main__": >> testutils.GanetiTestProgram() >> -- >> 1.8.5.5
