LGTM, thanks.
On Mon, Dec 9, 2013 at 9:36 PM, Santi Raffa <[email protected]> wrote: > On Thu, Dec 5, 2013 at 11:29 AM, Thomas Thrainer <[email protected]> > wrote: > > Why can't we add ST_SHARED_FILE in STS_REPORT? It does do (dummy) > reporting, > > right? In any case, I think having a special case here is not so great, > so > > either go for and stick to the dummy reporting, or don't claim that it's > > supported at all (which also defeats the purpose of the > > _DeclineToProvideSharedFileStorageInfo function, right?). > > I found out that the culprit was my implementation of > getDefaultStorageKey. If my understanding is correct, gnt-node list > causes a node storage query to be sent to luxid; luxid uses > getDefaultStorageKey, ignoring stsReport, and the query reached > _DeclineToProvideSharedFileStorageInfo; > _DeclineToProvideSharedFileStorageInfo refused to provide information, > which confused luxid and in turn gnt-node list. Of course, this > happens entirely at runtime. > > Once I do that, the only thing stopping me from adding ST_SHARED_FILE > to STS_REPORT is that if I do so, I _must_ have some > _DeclineToProvideSharedFileStorageInfo function, even if it takes the > non-implemented behavior (as specified in > _ComputeStorageDataFromSpaceInfoByTemplate) and returns: > > {"type": constants.ST_SHARED_FILE, "name": path, > "storage_size": 0, "storage_free": 0} > > ...given that this seems to be the default value all iAllocators get, > my previous worries about those zeroes having a special meaning in > _all_ iAllocators are essentially vanished. > > So, now I have two choices: > > * Add a new list, STS_REPORT_NODE_STORAGE, that only gets used in the > functions I "whitelist" (so far, mainly LUNodeQueryStorage); see the > interdiff below. > * Add Gluster to STS_REPORT anyway and hope nothing I didn't check breaks. > > My local QA works either way, but I might've disabled important bits > and pieces in order to let it run in the first place in a manageable > amount of time on an existing cluster. I consider the former approach > safer, but if the latter works without breaking ours and third party > stuff, that makes the code simpler. > > > diff --git a/lib/backend.py b/lib/backend.py > index 51b76d0..5f04438 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -755,14 +755,6 @@ def GetNodeInfo(storage_units, hv_specs): > return (bootid, storage_info, hv_info) > > > -def _DeclineToProvideSharedFileStorageInfo(path, _): > - """Returns no storage data for the Shared File disk template. > - > - """ > - return {"type": constants.ST_SHARED_FILE, > - "name": path} > - > - > def _GetFileStorageSpaceInfo(path, params): > """Wrapper around filestorage.GetSpaceInfo. > > @@ -786,7 +778,7 @@ _STORAGE_TYPE_INFO_FN = { > constants.ST_FILE: _GetFileStorageSpaceInfo, > constants.ST_LVM_PV: _GetLvmPvSpaceInfo, > constants.ST_LVM_VG: _GetLvmVgSpaceInfo, > - constants.ST_SHARED_FILE: _DeclineToProvideSharedFileStorageInfo, > + constants.ST_SHARED_FILE: _GetSharedFileDummySpaceInfo, > constants.ST_RADOS: None, > } > > diff --git a/lib/bootstrap.py b/lib/bootstrap.py > index a0185ff..0c0b6d1 100644 > --- a/lib/bootstrap.py > +++ b/lib/bootstrap.py > @@ -393,9 +393,10 @@ def _PrepareFileBasedStorage( > @returns: the name of the actual file storage directory > > """ > - assert (file_disk_template in > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE, > - > constants.ST_SHARED_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/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index a16d45a..5534f94 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -654,9 +654,9 @@ def CheckFileBasedStoragePathVsEnabledDiskTemplates( > path should be checked > > """ > - assert (file_disk_template in > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE, > - > constants.ST_SHARED_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 == "": > @@ -2576,10 +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, > - > constants.ST_SHARED_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 a95fddc..e5e53bf 100644 > --- a/lib/cmdlib/node.py > +++ b/lib/cmdlib/node.py > @@ -1310,8 +1310,7 @@ class LUNodeQueryStorage(NoHooksLU): > self.storage_type = self.op.storage_type > else: > self.storage_type = self._DetermineStorageType() > - supported_storage_types = set(constants.STS_REPORT) > - supported_storage_types.add(constants.ST_SHARED_FILE) > + 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" > diff --git a/lib/utils/storage.py b/lib/utils/storage.py > index e3b6cb9..e0328ff 100644 > --- a/lib/utils/storage.py > +++ b/lib/utils/storage.py > @@ -27,7 +27,7 @@ import logging > from ganeti import constants > > > -def GetDiskTemplatesOfStorageType(*storage_types): > +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 > diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py > index ef1d40b..af792a0 100644 > --- a/qa/qa_cluster.py > +++ b/qa/qa_cluster.py > @@ -498,8 +498,8 @@ def TestClusterModifyFileBasedStorageDir( > > # Get some non-file-based disk template to disable file storage > other_disk_template = _GetOtherEnabledDiskTemplate( > - utils.storage.GetDiskTemplatesOfStorageType(constants.ST_FILE, > - constants.ST_SHARED_FILE), > + utils.storage.GetDiskTemplatesOfStorageTypes(constants.ST_FILE, > + > constants.ST_SHARED_FILE), > enabled_disk_templates > ) > > 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/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index 5a2ce25..a98ac49 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -674,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 > > diff --git a/test/py/ganeti.backend_unittest.py > b/test/py/ganeti.backend_unittest.py > index 6651cd9..39fca14 100755 > --- a/test/py/ganeti.backend_unittest.py > +++ b/test/py/ganeti.backend_unittest.py > @@ -846,9 +846,7 @@ class TestSpaceReportingConstants(unittest.TestCase): > > """ > > - REPORTING = set(constants.STS_REPORT) > - REPORTING.add(constants.ST_SHARED_FILE) > - > + REPORTING = set(constants.STS_REPORT_NODE_STORAGE) > NOT_REPORTING = set(constants.STORAGE_TYPES) - REPORTING > > def testAllReportingTypesHaveAReportingFunction(self): > diff --git a/test/py/ganeti.utils.storage_unittest.py > b/test/py/ganeti.utils.storage_unittest.py > index c48cd0c..ac607ed 100755 > --- a/test/py/ganeti.utils.storage_unittest.py > +++ b/test/py/ganeti.utils.storage_unittest.py > @@ -81,7 +81,7 @@ class TestGetStorageUnits(unittest.TestCase): > > def testGetStorageUnits(self): > disk_templates = constants.DTS_FILEBASED - frozenset( > - storage.GetDiskTemplatesOfStorageType(constants.ST_SHARED_FILE) > + storage.GetDiskTemplatesOfStorageTypes(constants.ST_SHARED_FILE) > ) > storage_units = storage.GetStorageUnits(self._cfg, disk_templates) > self.assertEqual(len(storage_units), len(disk_templates)) > > > -- > Raffa Santi > 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 > -- 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
