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? Kind Regards, 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
