On Tue, Jan 28, 2014 at 10:12AM, Michele Tartara wrote: > 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?
Sorry about that, I didn't know that 2.8 was "old stable". I will re-send this on top of 2.9. Thanks, Ilias > > Cheers, > Michele -- Ilias Tsitsimpis OpenPGP public key ID: pub 4096R/25F3E3EE 2012-11-02 Ilias Tsitsimpis <[email protected]> Key fingerprint = 1986 21F5 7024 9B25 F4E2 4EF7 6FB8 90BA 25F3 E3EE
signature.asc
Description: Digital signature
