On Wed, Sep 25, 2013 at 12:11 PM, Jose A. Lopes <[email protected]>wrote:
> Functions 'BuildVersion' and 'SplitVersion' are no longer needed by > the constants and, given that they are not constants, they should be > moved elsewhere. Since they are only used by 'cfgupgrade' and tests, > these functions are moved to 'lib/utils/version.py' and references to > them updated. Note that in 'lib/server/masterd.py', local variable > 'version' is renamed 'ver' to avoid redefining the import > 'ganeti.utils.version'. > > Signed-off-by: Jose A. Lopes <[email protected]> > --- > lib/constants.py | 43 ------------------------ > lib/server/masterd.py | 12 ++++--- > lib/utils/version.py | 64 > ++++++++++++++++++++++++++++++++++++ > test/py/cfgupgrade_unittest.py | 52 +++++++++++++++-------------- > test/py/ganeti.constants_unittest.py | 20 ++++++----- > tools/cfgupgrade | 11 ++++--- > tools/cfgupgrade12 | 4 ++- > 7 files changed, 118 insertions(+), 88 deletions(-) > create mode 100644 lib/utils/version.py > > diff --git a/lib/constants.py b/lib/constants.py > index 67a1cc5..218e937 100644 > --- a/lib/constants.py > +++ b/lib/constants.py > @@ -44,49 +44,6 @@ VCS_VERSION = _vcsversion.VCS_VERSION > EXPORT_VERSION = 0 > RAPI_VERSION = 2 > > - > -# Format for CONFIG_VERSION: > -# 01 03 0123 = 01030123 > -# ^^ ^^ ^^^^ > -# | | + Configuration version/revision > -# | + Minor version > -# + Major version > -# > -# It is stored as an integer. Make sure not to write an octal number. > - > -# BuildVersion and SplitVersion must be in here because we can't import > other > -# modules. The cfgupgrade tool must be able to read and write version > numbers > -# and thus requires these functions. To avoid code duplication, they're > kept in > -# here. > - > -def BuildVersion(major, minor, revision): > - """Calculates int version number from major, minor and revision numbers. > - > - Returns: int representing version number > - > - """ > - assert isinstance(major, int) > - assert isinstance(minor, int) > - assert isinstance(revision, int) > - return (1000000 * major + > - 10000 * minor + > - 1 * revision) > - > - > -def SplitVersion(version): > - """Splits version number stored in an int. > - > - Returns: tuple; (major, minor, revision) > - > - """ > - assert isinstance(version, int) > - > - (major, remainder) = divmod(version, 1000000) > - (minor, revision) = divmod(remainder, 10000) > - > - return (major, minor, revision) > - > - > CONFIG_MAJOR = _constants.CONFIG_MAJOR > CONFIG_MINOR = _constants.CONFIG_MINOR > CONFIG_REVISION = _constants.CONFIG_REVISION > diff --git a/lib/server/masterd.py b/lib/server/masterd.py > index a41b203..dd1f5eb 100644 > --- a/lib/server/masterd.py > +++ b/lib/server/masterd.py > @@ -61,6 +61,8 @@ from ganeti import runtime > from ganeti import pathutils > from ganeti import ht > > +from ganeti.utils import version > + > > CLIENT_REQUEST_WORKERS = 16 > > @@ -90,7 +92,7 @@ class ClientRequestWorker(workerpool.BaseWorker): > client_ops = ClientOps(server) > > try: > - (method, args, version) = luxi.ParseRequest(message) > + (method, args, ver) = luxi.ParseRequest(message) > except luxi.ProtocolError, err: > logging.error("Protocol Error: %s", err) > client.close_log() > @@ -99,9 +101,9 @@ class ClientRequestWorker(workerpool.BaseWorker): > success = False > try: > # Verify client's version if there was one in the request > - if version is not None and version != constants.LUXI_VERSION: > + if ver is not None and ver != constants.LUXI_VERSION: > raise errors.LuxiError("LUXI version mismatch, server %s, request > %s" % > - (constants.LUXI_VERSION, version)) > + (constants.LUXI_VERSION, ver)) > > result = client_ops.handle_request(method, args) > success = True > @@ -685,8 +687,8 @@ def CheckMasterd(options, args): > try: > config.ConfigWriter() > except errors.ConfigVersionMismatch, err: > - v1 = "%s.%s.%s" % constants.SplitVersion(err.args[0]) > - v2 = "%s.%s.%s" % constants.SplitVersion(err.args[1]) > + v1 = "%s.%s.%s" % version.SplitVersion(err.args[0]) > + v2 = "%s.%s.%s" % version.SplitVersion(err.args[1]) > print >> sys.stderr, \ > ("Configuration version mismatch. The current Ganeti software" > " expects version %s, but the on-disk configuration file has" > diff --git a/lib/utils/version.py b/lib/utils/version.py > new file mode 100644 > index 0000000..b291dc8 > --- /dev/null > +++ b/lib/utils/version.py > @@ -0,0 +1,64 @@ > +#!/usr/bin/python > +# > + > +# Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Google Inc. > Please add 2013. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, but > +# WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +# General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > +# 02110-1301, USA. > + > + > +"""Version utilities.""" > + > + > +# Format for CONFIG_VERSION: > +# 01 03 0123 = 01030123 > +# ^^ ^^ ^^^^ > +# | | + Configuration version/revision > +# | + Minor version > +# + Major version > +# > +# It is stored as an integer. Make sure not to write an octal number. > + > +# BuildVersion and SplitVersion must be in here because we can't import > other > +# modules. The cfgupgrade tool must be able to read and write version > numbers > +# and thus requires these functions. To avoid code duplication, they're > kept in > +# here. > + > +def BuildVersion(major, minor, revision): > + """Calculates int version number from major, minor and revision numbers. > + > + Returns: int representing version number > + > + """ > + assert isinstance(major, int) > + assert isinstance(minor, int) > + assert isinstance(revision, int) > + return (1000000 * major + > + 10000 * minor + > + 1 * revision) > + > + > +def SplitVersion(version): > + """Splits version number stored in an int. > + > + Returns: tuple; (major, minor, revision) > + > + """ > + assert isinstance(version, int) > + > + (major, remainder) = divmod(version, 1000000) > + (minor, revision) = divmod(remainder, 10000) > + > + return (major, minor, revision) > diff --git a/test/py/cfgupgrade_unittest.py > b/test/py/cfgupgrade_unittest.py > index 2e0aadf..a08b167 100755 > --- a/test/py/cfgupgrade_unittest.py > +++ b/test/py/cfgupgrade_unittest.py > @@ -34,6 +34,8 @@ from ganeti import errors > from ganeti import serializer > from ganeti import netutils > > +from ganeti.utils import version > + > import testutils > > > @@ -199,7 +201,7 @@ class TestCfgupgrade(unittest.TestCase): > > self.assertFalse(os.path.exists(os.path.dirname(self.rapi_users_path))) > > utils.WriteFile(self.rapi_users_path_pre24, data="some user\n") > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), False) > > self.assertTrue(os.path.isdir(os.path.dirname(self.rapi_users_path))) > self.assert_(os.path.islink(self.rapi_users_path_pre24)) > @@ -215,7 +217,7 @@ class TestCfgupgrade(unittest.TestCase): > > os.mkdir(os.path.dirname(self.rapi_users_path)) > utils.WriteFile(self.rapi_users_path, data="other user\n") > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), False) > > self.assert_(os.path.islink(self.rapi_users_path_pre24)) > self.assert_(os.path.isfile(self.rapi_users_path)) > @@ -232,7 +234,7 @@ class TestCfgupgrade(unittest.TestCase): > os.symlink(self.rapi_users_path, self.rapi_users_path_pre24) > utils.WriteFile(self.rapi_users_path, data="hello world\n") > > - self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 2, 0), False) > > self.assert_(os.path.isfile(self.rapi_users_path) and > not os.path.islink(self.rapi_users_path)) > @@ -251,7 +253,7 @@ class TestCfgupgrade(unittest.TestCase): > utils.WriteFile(self.rapi_users_path_pre24, data="hello world\n") > > self.assertRaises(Exception, self._TestSimpleUpgrade, > - constants.BuildVersion(2, 2, 0), False) > + version.BuildVersion(2, 2, 0), False) > > for path in [self.rapi_users_path, self.rapi_users_path_pre24]: > self.assert_(os.path.isfile(path) and not os.path.islink(path)) > @@ -264,7 +266,7 @@ class TestCfgupgrade(unittest.TestCase): > self.assertFalse(os.path.exists(self.rapi_users_path_pre24)) > > utils.WriteFile(self.rapi_users_path_pre24, data="some user\n") > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), True) > > self.assertFalse(os.path.isdir(os.path.dirname(self.rapi_users_path))) > self.assertTrue(os.path.isfile(self.rapi_users_path_pre24) and > @@ -277,7 +279,7 @@ class TestCfgupgrade(unittest.TestCase): > > os.mkdir(os.path.dirname(self.rapi_users_path)) > utils.WriteFile(self.rapi_users_path, data="other user\n") > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), True) > > self.assertTrue(os.path.isfile(self.rapi_users_path) and > not os.path.islink(self.rapi_users_path)) > @@ -292,7 +294,7 @@ class TestCfgupgrade(unittest.TestCase): > os.symlink(self.rapi_users_path, self.rapi_users_path_pre24) > utils.WriteFile(self.rapi_users_path, data="hello world\n") > > - self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 2, 0), True) > > self.assertTrue(os.path.islink(self.rapi_users_path_pre24)) > self.assertTrue(os.path.isfile(self.rapi_users_path) and > @@ -305,7 +307,7 @@ class TestCfgupgrade(unittest.TestCase): > def testFileStoragePathsDryRun(self): > self.assertFalse(os.path.exists(self.file_storage_paths)) > > - self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), True, > + self._TestSimpleUpgrade(version.BuildVersion(2, 6, 0), True, > file_storage_dir=self.tmpdir, > shared_file_storage_dir="/tmp") > > @@ -314,7 +316,7 @@ class TestCfgupgrade(unittest.TestCase): > def testFileStoragePathsBoth(self): > self.assertFalse(os.path.exists(self.file_storage_paths)) > > - self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), False, > + self._TestSimpleUpgrade(version.BuildVersion(2, 6, 0), False, > file_storage_dir=self.tmpdir, > shared_file_storage_dir="/tmp") > > @@ -330,7 +332,7 @@ class TestCfgupgrade(unittest.TestCase): > def testFileStoragePathsSharedOnly(self): > self.assertFalse(os.path.exists(self.file_storage_paths)) > > - self._TestSimpleUpgrade(constants.BuildVersion(2, 5, 0), False, > + self._TestSimpleUpgrade(version.BuildVersion(2, 5, 0), False, > file_storage_dir=None, > shared_file_storage_dir=self.tmpdir) > > @@ -341,28 +343,28 @@ class TestCfgupgrade(unittest.TestCase): > self.assertFalse(lines) > > def testUpgradeFrom_2_0(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 0, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 0, 0), False) > > def testUpgradeFrom_2_1(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 1, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 1, 0), False) > > def testUpgradeFrom_2_2(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 2, 0), False) > > def testUpgradeFrom_2_3(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), False) > > def testUpgradeFrom_2_4(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 4, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 4, 0), False) > > def testUpgradeFrom_2_5(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 5, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 5, 0), False) > > def testUpgradeFrom_2_6(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 6, 0), False) > > def testUpgradeFrom_2_7(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 7, 0), False) > + self._TestSimpleUpgrade(version.BuildVersion(2, 7, 0), False) > > def testUpgradeFullConfigFrom_2_7(self): > self._TestUpgradeFromFile("cluster_config_2.7.json", False) > @@ -431,25 +433,25 @@ class TestCfgupgrade(unittest.TestCase): > self._RunDowngradeTwice() > > def testUpgradeDryRunFrom_2_0(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 0, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 0, 0), True) > > def testUpgradeDryRunFrom_2_1(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 1, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 1, 0), True) > > def testUpgradeDryRunFrom_2_2(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 2, 0), True) > > def testUpgradeDryRunFrom_2_3(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 3, 0), True) > > def testUpgradeDryRunFrom_2_4(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 4, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 4, 0), True) > > def testUpgradeDryRunFrom_2_5(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 5, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 5, 0), True) > > def testUpgradeDryRunFrom_2_6(self): > - self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), True) > + self._TestSimpleUpgrade(version.BuildVersion(2, 6, 0), True) > > def testUpgradeCurrentDryRun(self): > self._TestSimpleUpgrade(constants.CONFIG_VERSION, True) > diff --git a/test/py/ganeti.constants_unittest.py b/test/py/ > ganeti.constants_unittest.py > index cb1cdf1..166b963 100755 > --- a/test/py/ganeti.constants_unittest.py > +++ b/test/py/ganeti.constants_unittest.py > @@ -30,6 +30,8 @@ from ganeti import constants > from ganeti import locking > from ganeti import utils > > +from ganeti.utils import version > + > import testutils > > > @@ -46,16 +48,16 @@ class TestConstants(unittest.TestCase): > self.failUnless(constants.CONFIG_VERSION >= 0 and > constants.CONFIG_VERSION <= 99999999) > > - self.failUnless(constants.BuildVersion(0, 0, 0) == 0) > - self.failUnless(constants.BuildVersion(10, 10, 1010) == 10101010) > - self.failUnless(constants.BuildVersion(12, 34, 5678) == 12345678) > - self.failUnless(constants.BuildVersion(99, 99, 9999) == 99999999) > + self.failUnless(version.BuildVersion(0, 0, 0) == 0) > + self.failUnless(version.BuildVersion(10, 10, 1010) == 10101010) > + self.failUnless(version.BuildVersion(12, 34, 5678) == 12345678) > + self.failUnless(version.BuildVersion(99, 99, 9999) == 99999999) > > - self.failUnless(constants.SplitVersion(00000000) == (0, 0, 0)) > - self.failUnless(constants.SplitVersion(10101010) == (10, 10, 1010)) > - self.failUnless(constants.SplitVersion(12345678) == (12, 34, 5678)) > - self.failUnless(constants.SplitVersion(99999999) == (99, 99, 9999)) > - self.failUnless(constants.SplitVersion(constants.CONFIG_VERSION) == > + self.failUnless(version.SplitVersion(00000000) == (0, 0, 0)) > + self.failUnless(version.SplitVersion(10101010) == (10, 10, 1010)) > + self.failUnless(version.SplitVersion(12345678) == (12, 34, 5678)) > + self.failUnless(version.SplitVersion(99999999) == (99, 99, 9999)) > + self.failUnless(version.SplitVersion(constants.CONFIG_VERSION) == > (constants.CONFIG_MAJOR, constants.CONFIG_MINOR, > constants.CONFIG_REVISION)) > > diff --git a/tools/cfgupgrade b/tools/cfgupgrade > index 648d732..1b30ea9 100755 > --- a/tools/cfgupgrade > +++ b/tools/cfgupgrade > @@ -44,6 +44,8 @@ from ganeti import config > from ganeti import netutils > from ganeti import pathutils > > +from ganeti.utils import version > + > > options = None > args = None > @@ -347,8 +349,7 @@ def UpgradeInstanceIndices(config_data): > > > def UpgradeAll(config_data): > - config_data["version"] = constants.BuildVersion(TARGET_MAJOR, > - TARGET_MINOR, 0) > + config_data["version"] = version.BuildVersion(TARGET_MAJOR, > TARGET_MINOR, 0) > UpgradeRapiUsers() > UpgradeWatcher() > UpgradeFileStoragePaths(config_data) > @@ -379,8 +380,8 @@ def DowngradeNicParamsVLAN(nics, owner): > def DowngradeAll(config_data): > # Any code specific to a particular version should be labeled that way, > so > # it can be removed when updating to the next version. > - config_data["version"] = constants.BuildVersion(DOWNGRADE_MAJOR, > - DOWNGRADE_MINOR, 0) > + config_data["version"] = version.BuildVersion(DOWNGRADE_MAJOR, > + DOWNGRADE_MINOR, 0) > DowngradeInstances(config_data) > > > @@ -482,7 +483,7 @@ def main(): > raise Error("Unable to determine configuration version") > > (config_major, config_minor, config_revision) = \ > - constants.SplitVersion(config_version) > + version.SplitVersion(config_version) > > logging.info("Found configuration version %s (%d.%d.%d)", > config_version, config_major, config_minor, > config_revision) > diff --git a/tools/cfgupgrade12 b/tools/cfgupgrade12 > index dd76ce9..f684c9e 100755 > --- a/tools/cfgupgrade12 > +++ b/tools/cfgupgrade12 > @@ -49,6 +49,8 @@ from ganeti import utils > from ganeti import cli > from ganeti import pathutils > > +from ganeti.utils import version > + > > options = None > args = None > @@ -345,7 +347,7 @@ def main(): > raise Error("Unsupported configuration version: %s" % > old_config_version) > if "version" not in config_data: > - config_data["version"] = constants.BuildVersion(2, 0, 0) > + config_data["version"] = version.BuildVersion(2, 0, 0) > if F_SERIAL not in config_data: > config_data[F_SERIAL] = 1 > > -- > 1.8.4 > > Rest LGTM, thanks. -- Thomas Thrainer | Software Engineer | [email protected] | 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
