LGTM, thanks.
On Tue, Dec 17, 2013 at 10:28 AM, Santi Raffa <[email protected]> wrote: > The shared file and gluster disk templates should not report their disk > space information like file does, because they do not behave the same. > > If a cluster pulls from the same, shared source of storage then it is > not correct, or useful, to report per-node disk availability, as the > information is not node-specific. > > This change introduces the Shared File storage type for which no > per-node measuring of disk resources is made. > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/backend.py | 1 + > lib/bootstrap.py | 9 ++++++--- > lib/client/gnt_node.py | 1 + > lib/cmdlib/cluster.py | 11 +++++++---- > lib/cmdlib/common.py | 4 ++-- > lib/cmdlib/node.py | 21 +++++++++++++++------ > lib/storage/container.py | 1 + > lib/utils/storage.py | 4 ++-- > qa/qa_cluster.py | 6 ++++-- > qa/qa_config.py | 4 ++-- > qa/qa_instance.py | 2 +- > src/Ganeti/Constants.hs | 18 ++++++++++++------ > src/Ganeti/Storage/Utils.hs | 2 -- > src/Ganeti/Types.hs | 9 +++++++-- > test/py/ganeti.backend_unittest.py | 14 ++++++++------ > test/py/ganeti.utils.storage_unittest.py | 6 ++++-- > 16 files changed, 73 insertions(+), 40 deletions(-) > > diff --git a/lib/backend.py b/lib/backend.py > index 8a0672e..b329864 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -778,6 +778,7 @@ _STORAGE_TYPE_INFO_FN = { > constants.ST_FILE: _GetFileStorageSpaceInfo, > constants.ST_LVM_PV: _GetLvmPvSpaceInfo, > constants.ST_LVM_VG: _GetLvmVgSpaceInfo, > + constants.ST_SHARED_FILE: None, > constants.ST_RADOS: None, > } > > diff --git a/lib/bootstrap.py b/lib/bootstrap.py > index 24f4245..0c0b6d1 100644 > --- a/lib/bootstrap.py > +++ b/lib/bootstrap.py > @@ -387,13 +387,16 @@ def _PrepareFileBasedStorage( > @param default_dir: default file storage directory when > C{file_storage_dir} > is 'None' > @type file_disk_template: string > - @param file_disk_template: a disk template whose storage type is > 'ST_FILE' > + @param file_disk_template: a disk template whose storage type is > 'ST_FILE' or > + 'ST_SHARED_FILE' > @rtype: string > @returns: the name of the actual file storage directory > > """ > - assert (file_disk_template in > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE)) > + assert (file_disk_template in > utils.storage.GetDiskTemplatesOfStorageTypes( > + constants.ST_FILE, constants.ST_SHARED_FILE > + )) > + > if file_storage_dir is None: > file_storage_dir = default_dir > if not acceptance_fn: > diff --git a/lib/client/gnt_node.py b/lib/client/gnt_node.py > index ada6a7a..47bfb5c 100644 > --- a/lib/client/gnt_node.py > +++ b/lib/client/gnt_node.py > @@ -89,6 +89,7 @@ _USER_STORAGE_TYPE = { > constants.ST_FILE: "file", > constants.ST_LVM_PV: "lvm-pv", > constants.ST_LVM_VG: "lvm-vg", > + constants.ST_SHARED_FILE: "sharedfile", > } > > _STORAGE_TYPE_OPT = \ > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index 32fe0d3..5534f94 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -654,8 +654,9 @@ def CheckFileBasedStoragePathVsEnabledDiskTemplates( > path should be checked > > """ > - assert (file_disk_template in > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE)) > + assert (file_disk_template in > utils.storage.GetDiskTemplatesOfStorageTypes( > + constants.ST_FILE, constants.ST_SHARED_FILE > + )) > file_storage_enabled = file_disk_template in enabled_disk_templates > if file_storage_dir is not None: > if file_storage_dir == "": > @@ -2575,8 +2576,10 @@ class LUClusterVerifyGroup(LogicalUnit, > _VerifyErrors): > in case something goes wrong in this verification step > > """ > - assert (file_disk_template in > - > utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE)) > + assert (file_disk_template in > utils.storage.GetDiskTemplatesOfStorageTypes( > + constants.ST_FILE, constants.ST_SHARED_FILE > + )) > + > cluster = self.cfg.GetClusterInfo() > if cluster.IsDiskTemplateEnabled(file_disk_template): > self._ErrorIf( > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 8dc09d1..77b6f20 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -1104,7 +1104,7 @@ def CheckStorageTypeEnabled(cluster, storage_type): > CheckStorageTypeEnabled(cluster, constants.ST_LVM_VG) > else: > possible_disk_templates = \ > - utils.storage.GetDiskTemplatesOfStorageType(storage_type) > + utils.storage.GetDiskTemplatesOfStorageTypes(storage_type) > for disk_template in possible_disk_templates: > if disk_template in cluster.enabled_disk_templates: > return > @@ -1174,7 +1174,7 @@ def CheckDiskAccessModeConsistency(parameters, cfg, > group=None): > access = parameters[disk_template].get(constants.LDP_ACCESS, > constants.DISK_KERNELSPACE) > > - if dt not in constants.DTS_HAVE_ACCESS > + if disk_template not in constants.DTS_HAVE_ACCESS: > continue > > #Check the combination of instance hypervisor, disk template and > access > diff --git a/lib/cmdlib/node.py b/lib/cmdlib/node.py > index c8be6d69..e5e53bf 100644 > --- a/lib/cmdlib/node.py > +++ b/lib/cmdlib/node.py > @@ -1112,11 +1112,19 @@ def _GetStorageTypeArgs(cfg, storage_type): > > """ > # Special case for file storage > - if storage_type == constants.ST_FILE: > - # storage.FileStorage wants a list of storage directories > - return [[cfg.GetFileStorageDir(), cfg.GetSharedFileStorageDir()]] > > - return [] > + if storage_type == constants.ST_FILE: > + return [[cfg.GetFileStorageDir()]] > + elif storage_type == constants.ST_SHARED_FILE: > + dts = cfg.GetClusterInfo().enabled_disk_templates > + paths = [] > + if constants.DT_SHARED_FILE in dts: > + paths.append(cfg.GetSharedFileStorageDir()) > + if constants.DT_GLUSTER in dts: > + paths.append(cfg.GetGlusterStorageDir()) > + return [paths] > + else: > + return [] > > > class LUNodeModifyStorage(NoHooksLU): > @@ -1302,13 +1310,14 @@ class LUNodeQueryStorage(NoHooksLU): > self.storage_type = self.op.storage_type > else: > self.storage_type = self._DetermineStorageType() > - if self.storage_type not in constants.STS_REPORT: > + supported_storage_types = constants.STS_REPORT_NODE_STORAGE > + if self.storage_type not in supported_storage_types: > raise errors.OpPrereqError( > "Storage reporting for storage type '%s' is not supported. > Please" > " use the --storage-type option to specify one of the > supported" > " storage types (%s) or set the default disk template to one > that" > " supports storage reporting." % > - (self.storage_type, utils.CommaJoin(constants.STS_REPORT))) > + (self.storage_type, utils.CommaJoin(supported_storage_types))) > > def Exec(self, feedback_fn): > """Computes the list of nodes and their attributes. > diff --git a/lib/storage/container.py b/lib/storage/container.py > index d77d80b..1c9c747 100644 > --- a/lib/storage/container.py > +++ b/lib/storage/container.py > @@ -468,6 +468,7 @@ _STORAGE_TYPES = { > constants.ST_FILE: FileStorage, > constants.ST_LVM_PV: LvmPvStorage, > constants.ST_LVM_VG: LvmVgStorage, > + constants.ST_SHARED_FILE: FileStorage, > } > > > diff --git a/lib/utils/storage.py b/lib/utils/storage.py > index ab8443a..e0328ff 100644 > --- a/lib/utils/storage.py > +++ b/lib/utils/storage.py > @@ -27,11 +27,11 @@ import logging > from ganeti import constants > > > -def GetDiskTemplatesOfStorageType(storage_type): > +def GetDiskTemplatesOfStorageTypes(*storage_types): > """Given the storage type, returns a list of disk templates based on > that > storage type.""" > return [dt for dt in constants.DISK_TEMPLATES > - if constants.MAP_DISK_TEMPLATE_STORAGE_TYPE[dt] == storage_type] > + if constants.MAP_DISK_TEMPLATE_STORAGE_TYPE[dt] in > storage_types] > > > def IsDiskTemplateEnabled(disk_template, enabled_disk_templates): > diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py > index 119472b..af792a0 100644 > --- a/qa/qa_cluster.py > +++ b/qa/qa_cluster.py > @@ -498,8 +498,10 @@ def TestClusterModifyFileBasedStorageDir( > > # Get some non-file-based disk template to disable file storage > other_disk_template = _GetOtherEnabledDiskTemplate( > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE), > - enabled_disk_templates) > + utils.storage.GetDiskTemplatesOfStorageTypes(constants.ST_FILE, > + > constants.ST_SHARED_FILE), > + enabled_disk_templates > + ) > > file_storage_dir = qa_config.get(dir_config_key, default_dir) > invalid_file_storage_dir = "/boot/" > diff --git a/qa/qa_config.py b/qa/qa_config.py > index 29d17c5..bc650ce 100644 > --- a/qa/qa_config.py > +++ b/qa/qa_config.py > @@ -480,9 +480,9 @@ class _QaConfig(object): > """ > enabled_disk_templates = self.GetEnabledDiskTemplates() > if storage_type == constants.ST_LVM_PV: > - disk_templates = > utils.GetDiskTemplatesOfStorageType(constants.ST_LVM_VG) > + disk_templates = > utils.GetDiskTemplatesOfStorageTypes(constants.ST_LVM_VG) > else: > - disk_templates = utils.GetDiskTemplatesOfStorageType(storage_type) > + disk_templates = utils.GetDiskTemplatesOfStorageTypes(storage_type) > return > bool(set(enabled_disk_templates).intersection(set(disk_templates))) > > def AreSpindlesSupported(self): > diff --git a/qa/qa_instance.py b/qa/qa_instance.py > index f3e4710..afbc853 100644 > --- a/qa/qa_instance.py > +++ b/qa/qa_instance.py > @@ -135,7 +135,7 @@ def _DestroyInstanceDisks(instance): > vols = info["volumes"] > for node in info["nodes"]: > AssertCommand(["lvremove", "-f"] + vols, node=node) > - elif info["storage-type"] == constants.ST_FILE: > + elif info["storage-type"] in (constants.ST_FILE, > constants.ST_SHARED_FILE): > # Note that this works for both file and sharedfile, and this is > intended. > storage_dir = qa_config.get("file-storage-dir", > pathutils.DEFAULT_FILE_STORAGE_DIR) > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index fa7ccf7..a98ac49 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -659,6 +659,9 @@ stExt = Types.storageTypeToRaw StorageExt > stFile :: String > stFile = Types.storageTypeToRaw StorageFile > > +stSharedFile :: String > +stSharedFile = Types.storageTypeToRaw StorageSharedFile > + > stLvmPv :: String > stLvmPv = Types.storageTypeToRaw StorageLvmPv > > @@ -671,13 +674,16 @@ stRados = Types.storageTypeToRaw StorageRados > storageTypes :: FrozenSet String > storageTypes = ConstantUtils.mkSet $ map Types.storageTypeToRaw > [minBound..] > > --- | The set of storage types for which storage reporting is available > --- > --- FIXME: Remove this, once storage reporting is available for all > --- types. > +-- | The set of storage types for which full storage reporting is > available > stsReport :: FrozenSet String > stsReport = ConstantUtils.mkSet [stFile, stLvmPv, stLvmVg] > > +-- | The set of storage types for which node storage reporting is > available > +-- | (as used by LUQueryNodeStorage) > +stsReportNodeStorage :: FrozenSet String > +stsReportNodeStorage = ConstantUtils.union stsReport $ > + ConstantUtils.mkSet > [stSharedFile] > + > -- * Storage fields > -- ** First two are valid in LU context only, not passed to backend > > @@ -818,12 +824,12 @@ mapDiskTemplateStorageType = > [(DTBlock, StorageBlock), > (DTDrbd8, StorageLvmVg), > (DTExt, StorageExt), > - (DTSharedFile, StorageFile), > + (DTSharedFile, StorageSharedFile), > (DTFile, StorageFile), > (DTDiskless, StorageDiskless), > (DTPlain, StorageLvmVg), > (DTRbd, StorageRados), > - (DTGluster, StorageFile)] > + (DTGluster, StorageSharedFile)] > > -- | The set of network-mirrored disk templates > dtsIntMirror :: FrozenSet String > diff --git a/src/Ganeti/Storage/Utils.hs b/src/Ganeti/Storage/Utils.hs > index 64fd190..7957207 100644 > --- a/src/Ganeti/Storage/Utils.hs > +++ b/src/Ganeti/Storage/Utils.hs > @@ -44,8 +44,6 @@ getDefaultStorageKey cfg T.DTDrbd8 = > clusterVolumeGroupName $ configCluster cfg > getDefaultStorageKey cfg T.DTPlain = clusterVolumeGroupName $ > configCluster cfg > getDefaultStorageKey cfg T.DTFile = > Just (clusterFileStorageDir $ configCluster cfg) > -getDefaultStorageKey cfg T.DTSharedFile = > - Just (clusterSharedFileStorageDir $ configCluster cfg) > getDefaultStorageKey _ _ = Nothing > > -- | Get the cluster's default spindle storage unit > diff --git a/src/Ganeti/Types.hs b/src/Ganeti/Types.hs > index 3200336..42201d9 100644 > --- a/src/Ganeti/Types.hs > +++ b/src/Ganeti/Types.hs > @@ -459,6 +459,7 @@ $(THH.makeJSONInstance ''OobStatus) > -- | Storage type. > $(THH.declareLADT ''String "StorageType" > [ ("StorageFile", "file") > + , ("StorageSharedFile", "sharedfile") > , ("StorageLvmPv", "lvm-pv") > , ("StorageLvmVg", "lvm-vg") > , ("StorageDiskless", "diskless") > @@ -481,6 +482,7 @@ data StorageUnitRaw = SURaw StorageType StorageKey > > -- | Full storage unit with storage-type-specific parameters > data StorageUnit = SUFile StorageKey > + | SUSharedFile StorageKey > | SULvmPv StorageKey SPExclusiveStorage > | SULvmVg StorageKey SPExclusiveStorage > | SUDiskless StorageKey > @@ -491,6 +493,7 @@ data StorageUnit = SUFile StorageKey > > instance Show StorageUnit where > show (SUFile key) = showSUSimple StorageFile key > + show (SUSharedFile key) = showSUSimple StorageSharedFile key > show (SULvmPv key es) = showSULvm StorageLvmPv key es > show (SULvmVg key es) = showSULvm StorageLvmVg key es > show (SUDiskless key) = showSUSimple StorageDiskless key > @@ -500,6 +503,7 @@ instance Show StorageUnit where > > instance JSON StorageUnit where > showJSON (SUFile key) = showJSON (StorageFile, key, []::[String]) > + showJSON (SUSharedFile key) = showJSON (StorageSharedFile, key, > []::[String]) > showJSON (SULvmPv key es) = showJSON (StorageLvmPv, key, [es]) > showJSON (SULvmVg key es) = showJSON (StorageLvmVg, key, [es]) > showJSON (SUDiskless key) = showJSON (StorageDiskless, key, > []::[String]) > @@ -525,13 +529,13 @@ showSULvm st sk es = show (storageTypeToRaw st, sk, > [es]) > diskTemplateToStorageType :: DiskTemplate -> StorageType > diskTemplateToStorageType DTExt = StorageExt > diskTemplateToStorageType DTFile = StorageFile > -diskTemplateToStorageType DTSharedFile = StorageFile > +diskTemplateToStorageType DTSharedFile = StorageSharedFile > diskTemplateToStorageType DTDrbd8 = StorageLvmVg > diskTemplateToStorageType DTPlain = StorageLvmVg > diskTemplateToStorageType DTRbd = StorageRados > diskTemplateToStorageType DTDiskless = StorageDiskless > diskTemplateToStorageType DTBlock = StorageBlock > -diskTemplateToStorageType DTGluster = StorageFile > +diskTemplateToStorageType DTGluster = StorageSharedFile > > -- | Equips a raw storage unit with its parameters > addParamsToStorageUnit :: SPExclusiveStorage -> StorageUnitRaw -> > StorageUnit > @@ -539,6 +543,7 @@ addParamsToStorageUnit _ (SURaw StorageBlock key) = > SUBlock key > addParamsToStorageUnit _ (SURaw StorageDiskless key) = SUDiskless key > addParamsToStorageUnit _ (SURaw StorageExt key) = SUExt key > addParamsToStorageUnit _ (SURaw StorageFile key) = SUFile key > +addParamsToStorageUnit _ (SURaw StorageSharedFile key) = SUSharedFile key > addParamsToStorageUnit es (SURaw StorageLvmPv key) = SULvmPv key es > addParamsToStorageUnit es (SURaw StorageLvmVg key) = SULvmVg key es > addParamsToStorageUnit _ (SURaw StorageRados key) = SURados key > diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ > ganeti.backend_unittest.py > index 226c8f3..033419e 100755 > --- a/test/py/ganeti.backend_unittest.py > +++ b/test/py/ganeti.backend_unittest.py > @@ -839,20 +839,22 @@ class TestGetNodeInfo(unittest.TestCase): > class TestSpaceReportingConstants(unittest.TestCase): > """Ensures consistency between STS_REPORT and backend. > > - These tests ensure, that the constant 'STS_REPORT' is consitent > + These tests ensure, that the constant 'STS_REPORT' is consistent > with the implementation of invoking space reporting functions > in backend.py. Once space reporting is available for all types, > the constant can be removed and these tests as well. > > """ > + > + REPORTING = set(constants.STS_REPORT) > + NOT_REPORTING = set(constants.STORAGE_TYPES) - REPORTING > + > def testAllReportingTypesHaveAReportingFunction(self): > - for storage_type in constants.STS_REPORT: > + for storage_type in TestSpaceReportingConstants.REPORTING: > self.assertTrue(backend._STORAGE_TYPE_INFO_FN[storage_type] is not > None) > > - def testAllNotReportingTypesDoneHaveFunction(self): > - non_reporting_types = set(constants.STORAGE_TYPES)\ > - - set(constants.STS_REPORT) > - for storage_type in non_reporting_types: > + def testAllNotReportingTypesDontHaveFunction(self): > + for storage_type in TestSpaceReportingConstants.NOT_REPORTING: > self.assertEqual(None, backend._STORAGE_TYPE_INFO_FN[storage_type]) > > > diff --git a/test/py/ganeti.utils.storage_unittest.py b/test/py/ > ganeti.utils.storage_unittest.py > index 92b1648..ac607ed 100755 > --- a/test/py/ganeti.utils.storage_unittest.py > +++ b/test/py/ganeti.utils.storage_unittest.py > @@ -61,7 +61,7 @@ class > TestGetStorageUnitForDiskTemplate(unittest.TestCase): > (storage_type, storage_key) = \ > storage._GetDefaultStorageUnitForDiskTemplate(self._cfg, > > constants.DT_SHARED_FILE) > - self.assertEqual(storage_type, constants.ST_FILE) > + self.assertEqual(storage_type, constants.ST_SHARED_FILE) > self.assertEqual(storage_key, self._cluster.shared_file_storage_dir) > > def testGetDefaultStorageUnitForDiskTemplateDiskless(self): > @@ -80,7 +80,9 @@ class TestGetStorageUnits(unittest.TestCase): > self._cfg = mock.Mock() > > def testGetStorageUnits(self): > - disk_templates = constants.DTS_FILEBASED > + disk_templates = constants.DTS_FILEBASED - frozenset( > + storage.GetDiskTemplatesOfStorageTypes(constants.ST_SHARED_FILE) > + ) > storage_units = storage.GetStorageUnits(self._cfg, disk_templates) > self.assertEqual(len(storage_units), len(disk_templates)) > > -- > 1.7.10.4 > > -- 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
