On Wed, Dec 4, 2013 at 2:50 PM, Santi Raffa <[email protected]> wrote:
> Add parameters to the Gluster disk template so Gluster can manage the > mount point point autonomously. > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/cmdlib/instance.py | 3 +- > lib/cmdlib/instance_storage.py | 7 +++- > lib/cmdlib/instance_utils.py | 2 +- > lib/storage/gluster.py | 68 > +++++++++++++++++++++++++++---------- > src/Ganeti/Constants.hs | 46 +++++++++++++++++++++---- > test/py/cmdlib/instance_unittest.py | 18 +++++++--- > 6 files changed, 113 insertions(+), 31 deletions(-) > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > index 99bc9cf..d3070c4 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -1555,7 +1555,8 @@ class LUInstanceRename(LogicalUnit): > old_name = self.instance.name > > rename_file_storage = False > - if (self.instance.disk_template in constants.DTS_FILEBASED and > + if (self.instance.disk_template in (constants.DT_FILE, > + constants.DT_SHARED_FILE) and > self.op.new_name != self.instance.name): > old_file_storage_dir = os.path.dirname( > self.instance.disks[0].logical_id[1]) > diff --git a/lib/cmdlib/instance_storage.py > b/lib/cmdlib/instance_storage.py > index 641b253..333de1b 100644 > --- a/lib/cmdlib/instance_storage.py > +++ b/lib/cmdlib/instance_storage.py > @@ -460,7 +460,12 @@ def GenerateDiskTemplate( > vg = disk.get(constants.IDISK_VG, vgname) > return (vg, names[idx]) > > - elif template_name in constants.DTS_FILEBASED: > + elif template_name == constants.DT_GLUSTER: > + logical_id_fn = lambda _1, disk_index, _2: \ > + (file_driver, "ganeti/%s.%d" % (instance_uuid, > + disk_index)) > + > + elif template_name in constants.DTS_FILEBASED: # Gluster handled above > logical_id_fn = \ > lambda _, disk_index, disk: (file_driver, > "%s/disk%d" % (file_storage_dir, > diff --git a/lib/cmdlib/instance_utils.py b/lib/cmdlib/instance_utils.py > index 752e66d..9e10fd1 100644 > --- a/lib/cmdlib/instance_utils.py > +++ b/lib/cmdlib/instance_utils.py > @@ -292,7 +292,7 @@ def RemoveDisks(lu, instance, target_node_uuid=None, > ignore_failures=False): > > CheckDiskTemplateEnabled(lu.cfg.GetClusterInfo(), > instance.disk_template) > > - if instance.disk_template in constants.DTS_FILEBASED: > + if instance.disk_template in [constants.DT_FILE, > constants.DT_SHARED_FILE]: > file_storage_dir = os.path.dirname(instance.disks[0].logical_id[1]) > if target_node_uuid: > tgt = target_node_uuid > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index e5829c8..2751d4b 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -33,6 +33,7 @@ from ganeti import utils > from ganeti import errors > from ganeti import netutils > from ganeti import constants > +from ganeti import ssconf > > from ganeti.utils import io > from ganeti.storage import base > @@ -263,28 +264,36 @@ class GlusterVolume(object): > class GlusterStorage(base.BlockDev): > """File device. > > - This class represents a file storage backend device stored on Gluster. > The > - system administrator must mount the Gluster device himself at boot time > before > - Ganeti is run. > + This class represents a file storage backend device stored on Gluster. > Ganeti > + mounts and unmounts the Gluster devices automatically. > > The unique_id for the file device is a (file_driver, file_path) tuple. > > """ > - def __init__(self, unique_id, children, size, params, dyn_params, > - _file_helper_obj=None): > + def __init__(self, unique_id, children, size, params, dyn_params): > """Initalizes a file device backend. > > """ > if children: > base.ThrowError("Invalid setup for file device") > - super(GlusterStorage, self).__init__(unique_id, children, size, > params, > - dyn_params) > - if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2: > - raise ValueError("Invalid configuration data %s" % str(unique_id)) > - self.driver = unique_id[0] > - self.dev_path = unique_id[1] > > - self.file = FileDeviceHelper(self.dev_path) > + try: > + driver, path = unique_id > + except ValueError: # wrong number of arguments > + raise ValueError("Invalid configuration data %s" % repr(unique_id)) > + > + server_addr = params[constants.GLUSTER_HOST] > + port = params[constants.GLUSTER_PORT] > + volume = params[constants.GLUSTER_VOLUME] > + > + self.volume = GlusterVolume(server_addr, port, volume) > + self.path = path > + self.driver = driver > + self.full_path = io.PathJoin(self.volume.mount_point, self.path) > + self.file = None > + > + super(GlusterStorage, self).__init__(unique_id, children, size, > + params, dyn_params) > > self.Attach() > > @@ -329,7 +338,13 @@ class GlusterStorage(base.BlockDev): > @return: True if the removal was successful > > """ > - return self.file.Remove() > + with self.volume: > I don't quite like this 'with' construction here: It hides the fact that there are rather complex operations (mounting and later unmounting) are going on, which can even fail randomly here. Something like try-finally or 'with self.volume.Mount()' would make it much more readable (if I were to choose, I'd go for the try-finally approach). What do you think? The same holds true for all other 'with' constructs for mounting. > + self.file = FileDeviceHelper(self.full_path) > + if self.file.Remove(): > + self.file = None > + return True > + else: > + return False > > def Rename(self, new_id): > """Renames the file. > @@ -359,6 +374,15 @@ class GlusterStorage(base.BlockDev): > @return: True if file exists > > """ > + try: > + self.volume.Mount() > + self.file = FileDeviceHelper(self.full_path) > + self.dev_path = self.full_path > + except Exception as err: > + self.volume.Unmount() > + raise err > + > + self.attached = True > return self.file.Exists() > > def GetActualSize(self): > @@ -386,8 +410,18 @@ class GlusterStorage(base.BlockDev): > if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2: > raise ValueError("Invalid configuration data %s" % str(unique_id)) > > - dev_path = unique_id[1] > + full_path = unique_id[1] > + > + server_addr = params[constants.GLUSTER_HOST] > + port = params[constants.GLUSTER_PORT] > + volume = params[constants.GLUSTER_VOLUME] > + > + volume_obj = GlusterVolume(server_addr, port, volume) > + full_path = io.PathJoin(volume_obj.mount_point, full_path) > + > + # Possible optimization: defer actual creation to first Attach, rather > + # than mounting and unmounting here, then remounting immediately > after. > + with volume_obj: > + FileDeviceHelper.Create(full_path, size, create_folders=True) > > - file_helper = FileDeviceHelper.Create(dev_path, size) > - return GlusterStorage(unique_id, children, size, params, dyn_params, > - _file_helper_obj=file_helper) > + return GlusterStorage(unique_id, children, size, params, dyn_params) > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index 427e5db..654429e 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -2167,7 +2167,11 @@ diskDtTypes = > (drbdMinRate, VTypeInt), > (lvStripes, VTypeInt), > (rbdAccess, VTypeString), > - (rbdPool, VTypeString)] > + (rbdPool, VTypeString), > + (glusterHost, VTypeString), > + (glusterVolume, VTypeString), > + (glusterPort, VTypeInt) > + ] > > diskDtParameters :: FrozenSet String > diskDtParameters = ConstantUtils.mkSet (Map.keys diskDtTypes) > @@ -3773,7 +3777,12 @@ diskLdDefaults = > , (ldpAccess, PyValueEx diskKernelspace) > ]) > , (DTSharedFile, Map.empty) > - , (DTGluster, Map.empty) > + , (DTGluster, Map.fromList > + [ (rbdAccess, PyValueEx diskKernelspace) > + , (glusterHost, PyValueEx glusterHostDefault) > + , (glusterVolume, PyValueEx glusterVolumeDefault) > + , (glusterPort, PyValueEx glusterPortDefault) > + ]) > ] > > diskDtDefaults :: Map DiskTemplate (Map String PyValueEx) > @@ -3806,7 +3815,12 @@ diskDtDefaults = > , (rbdAccess, PyValueEx diskKernelspace) > ]) > , (DTSharedFile, Map.empty) > - , (DTGluster, Map.empty) > + , (DTGluster, Map.fromList > + [ (rbdAccess, PyValueEx diskKernelspace) > + , (glusterHost, PyValueEx glusterHostDefault) > + , (glusterVolume, PyValueEx glusterVolumeDefault) > + , (glusterPort, PyValueEx glusterPortDefault) > + ]) > ] > > niccDefaults :: Map String PyValueEx > @@ -4592,6 +4606,26 @@ jstoreJobsPerArchiveDirectory = 10000 > > -- * Gluster settings > > --- | Where Ganeti should manage Gluster volume mountpoints > -glusterMountpoint :: String > -glusterMountpoint = "/var/run/ganeti/gluster" > +-- | Name of the Gluster host setting > +glusterHost :: String > +glusterHost = "host" > + > +-- | Default value of the Gluster host setting > +glusterHostDefault :: String > +glusterHostDefault = "127.0.0.1" > + > +-- | Name of the Gluster volume setting > +glusterVolume :: String > +glusterVolume = "volume" > + > +-- | Default value of the Gluster volume setting > +glusterVolumeDefault :: String > +glusterVolumeDefault = "gv0" > + > +-- | Name of the Gluster port setting > +glusterPort :: String > +glusterPort = "port" > + > +-- | Default value of the Gluster port setting > +glusterPortDefault :: Int > +glusterPortDefault = 24007 > diff --git a/test/py/cmdlib/instance_unittest.py > b/test/py/cmdlib/instance_unittest.py > index 448d7c9..49a054e 100644 > --- a/test/py/cmdlib/instance_unittest.py > +++ b/test/py/cmdlib/instance_unittest.py > @@ -1106,11 +1106,19 @@ class TestGenerateDiskTemplate(CmdlibTestCase): > disk_template, disk_info, 2, disk_template, > file_storage_dir="/tmp", file_driver=constants.FD_BLKTAP) > > - self.assertEqual(map(operator.attrgetter("logical_id"), result), [ > - (constants.FD_BLKTAP, "/tmp/disk2"), > - (constants.FD_BLKTAP, "/tmp/disk3"), > - (constants.FD_BLKTAP, "/tmp/disk4"), > - ]) > + if disk_template == constants.DT_GLUSTER: > + # Here "inst21662.example.com" is actually the instance UUID, > not its > + # name, so while this result looks wrong, it is actually correct. > + expected = [(constants.FD_BLKTAP, > + 'ganeti/inst21662.example.com.%d' % x) > + for x in (2,3,4)] > + else: > + expected = [(constants.FD_BLKTAP, > + '/tmp/disk%d' % x) > + for x in (2,3,4)] > + > + self.assertEqual(map(operator.attrgetter("logical_id"), result), > + expected) > > def testBlock(self): > disk_info = [{ > -- > 1.8.4.1 > > -- 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
