Hello Ilias, On Tue, Jan 28, 2014 at 9:00 AM, Ilias Tsitsimpis <[email protected]> wrote: > With the new format for cmdline arguments, the user is able to add a > disk to an instance at a specific index. But filebased disks' filenames > have the form "{0}/disk{1}" where '{0}' is the file_storage_dir and > '{1}' is the index of the disk. So if an instance has 3 disks and we > try to create a new one at index 1, the operation will fail because the > filename "{0}/disk1" already exists. > > This patch fixes the above problem and also makes the naming of file and > sharedfile disks uniform with other templates. > > Signed-off-by: Ilias Tsitsimpis <[email protected]> > --- > lib/cmdlib/instance_storage.py | 6 ++++-- > test/py/ganeti.cmdlib_unittest.py | 12 +++++++----- > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage.py > index b1fea8b..c22bedb 100644 > --- a/lib/cmdlib/instance_storage.py > +++ b/lib/cmdlib/instance_storage.py > @@ -52,6 +52,8 @@ _DISK_TEMPLATE_NAME_PREFIX = { > constants.DT_PLAIN: "", > constants.DT_RBD: ".rbd", > constants.DT_EXT: ".ext", > + constants.DT_FILE: ".file", > + constants.DT_SHARED_FILE: ".sharedfile", > } > > > @@ -470,8 +472,8 @@ def GenerateDiskTemplate( > elif template_name in (constants.DT_FILE, constants.DT_SHARED_FILE): > logical_id_fn = \ > lambda _, disk_index, disk: (file_driver, > - "%s/disk%d" % (file_storage_dir, > - disk_index)) > + "%s/%s" % (file_storage_dir, > + names[idx])) > elif template_name == constants.DT_BLOCK: > logical_id_fn = \ > lambda idx, disk_index, disk: (constants.BLOCKDEV_DRIVER_MANUAL, > diff --git a/test/py/ganeti.cmdlib_unittest.py > b/test/py/ganeti.cmdlib_unittest.py > index 21b2c26..95c8f6f 100755 > --- a/test/py/ganeti.cmdlib_unittest.py > +++ b/test/py/ganeti.cmdlib_unittest.py > @@ -23,6 +23,7 @@ > > > import os > +import re > import unittest > import tempfile > import shutil > @@ -1282,11 +1283,12 @@ class TestGenerateDiskTemplate(unittest.TestCase): > req_file_storage=self._AllowFileStorage, > req_shr_file_storage=self._AllowFileStorage) > > - self.assertEqual(map(operator.attrgetter("logical_id"), result), [ > - (constants.FD_BLKTAP, "/tmp/disk2"), > - (constants.FD_BLKTAP, "/tmp/disk3"), > - (constants.FD_BLKTAP, "/tmp/disk4"), > - ]) > + for (idx, disk) in enumerate(result): > + (file_driver, file_storage_dir) = disk.logical_id > + dir_fmt = "^/tmp/.*\.%s\.disk%d$" % (disk_template, idx + 2) > + self.assertEqual(file_driver, constants.FD_BLKTAP) > + # FIXME: use assertIsNotNone when py 2.7 is minimum supported version > + self.assertNotEqual(re.match(dir_fmt, file_storage_dir), None) > > def testBlock(self): > disk_info = [{ > -- > 1.8.5.3
Thanks for your contribution. Unfortunately I see a few problems with this patch. First of all, given that it changes the name of the disks, it would break all existing installations of Ganeti 2.8: a point release should be a drop-in replacement of a previous release of the same stable branch. So, I don't think this should go in 2.8. And probably, for the same reason, not even in 2.9 which is already stable as well. It might go in 2.10, which is not stable yet, but it should definitely include modifications to cfgupgrade to properly handle the upgrade/downgrade from/to a previous version of Ganeti with the old naming scheme. Cheers, Michele -- 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
