Hi Ilias, On Tue, Jan 28, 2014 at 9:47 AM, Ilias Tsitsimpis <[email protected]> wrote: > On Tue, Jan 28, 2014 at 09:24AM, Michele Tartara wrote: >> 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 > > Hello Michele, > >> >> 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 is not clear to me why you believe this patch breaks existing Ganeti > installations. All this patch does is to assign a unique *filename* to > the filebased disks that are going to be created from this point on. The > existing disks are not changed. > >> 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. > > I don't see why there is a need for cfgupgrade. The config file does not > change at all. I am not talking about changing the filename of existing > disks. This would require not only a cfgupgrade but also renaming the > disks' filenames. Instead with this patch only the newly created disks > are going to be assigned unique names. > > Is there something I don't understand?
No, my fault, I hadn't noticed this was only changing the name of new disks. Still, given that 2.8 is the "old stable" version, by now, we would prefer not to have other patches there unless it's for really critical bugs. Could you please re-send this on top of 2.9? 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
