Adam Litke has posted comments on this change. Change subject: storage: Add support for generation id to VolumeMetadata ......................................................................
Patch Set 1: (10 comments) https://gerrit.ovirt.org/#/c/64484/1/lib/vdsm/storage/constants.py File lib/vdsm/storage/constants.py: Line 151 Line 152 Line 153 Line 154 Line 155 > Please add the new GEN line to this table. Done Line 154 Line 155 Line 156 Line 157 Line 158 > Please update the values after this change. Done Line 124: IMAGE = "IMAGE" Line 125: DESCRIPTION = "DESCRIPTION" Line 126: LEGALITY = "LEGALITY" Line 127: MTIME = "MTIME" Line 128: GENERATION = "GENID" > GENID is not correct, id is used to identify stuff, and is usually unique, Done Line 129: POOL = MDK_POOLS # Deprecated Line 130: Line 131: # In block storage, metadata size is limited to BLOCK_SIZE (512), to Line 132: # ensure that metadata is written atomically. This is big enough for the https://gerrit.ovirt.org/#/c/64484/1/lib/vdsm/storage/volumemetadata.py File lib/vdsm/storage/volumemetadata.py: Line 33: Line 34: def __init__(self, domain, image, puuid, size, format, Line 35: type, voltype, disktype, description="", Line 36: legality=constants.ILLEGAL_VOL, ctime=None, mtime=None, Line 37: generation=None): > Why not use 0 as default value? Done Line 38: assert(isinstance(size, int)) Line 39: assert(ctime is None or isinstance(ctime, int)) Line 40: assert(mtime is None or isinstance(mtime, int)) Line 41: assert(generation is None or isinstance(generation, int)) Line 37: generation=None): Line 38: assert(isinstance(size, int)) Line 39: assert(ctime is None or isinstance(ctime, int)) Line 40: assert(mtime is None or isinstance(mtime, int)) Line 41: assert(generation is None or isinstance(generation, int)) > Using 0 as default would eliminate this Keeping the isinstance check Line 42: Line 43: # Storage domain UUID Line 44: self.domain = domain Line 45: # Image UUID Line 64: self.ctime = int(time.time()) if ctime is None else ctime Line 65: # Volume modification time (unused and should be zero) Line 66: self.mtime = 0 if mtime is None else mtime Line 67: # Generation ID increments each time certain operations complete Line 68: self.generation = 0 if generation is None else generation > And simplify this Done Line 69: Line 70: @classmethod Line 71: def from_lines(cls, lines): Line 72: md = {} Line 89: disktype=md[constants.DISKTYPE], Line 90: description=md[constants.DESCRIPTION], Line 91: legality=md[constants.LEGALITY], Line 92: ctime=int(md[constants.CTIME]), Line 93: mtime=int(md[constants.MTIME]), > Maybe comment here why generation may not be available. Done Line 94: generation=int(md.get(constants.GENERATION, 0))) Line 95: except KeyError as e: Line 96: raise exception.MetaDataKeyNotFoundError( Line 97: "Missing metadata key: %s: found: %s" % (e, md)) Line 148: constants.DESCRIPTION: self.description, Line 149: constants.PUUID: self.puuid, Line 150: constants.MTIME: str(self.mtime), Line 151: constants.LEGALITY: self.legality, Line 152: constants.GENERATION: self.generation, > This will add generation key and value to legacy storage domain - we have t Having this volume-related code change behavior based on storage domain version will add its own set of complexities. For example, we'll have to find a nice way of passing around the storage domain version of a volume. Can't we just validate the currently released and supported versions of vdsm for their ability to handle unknown keys gracefully? https://gerrit.ovirt.org/#/c/64484/1/tests/storage_volume_metadata_test.py File tests/storage_volume_metadata_test.py: Line 98: PUUID=params['puuid'], Line 99: SIZE=str(params['size']), Line 100: TYPE=params['type'], Line 101: VOLTYPE=params['voltype'], Line 102: GENID=params['generation']) > If we support this only on domain version 4, we need to have a test for not Not sure if we really want to do this. Line 103: Line 104: with MonkeyPatchScope([[time, 'time', lambda: FAKE_TIME]]): Line 105: info = volume.VolumeMetadata(**params).legacy_info() Line 106: self.assertEqual(expected, info) Line 187: self.assertEqual(int(data[sc.GENERATION]), md.generation) Line 188: Line 189: def test_generation_default(self): Line 190: data = make_md_dict() Line 191: lines = make_lines(generation=None, **data) > Do we want to support generation=None? better accept only integer value, an Done Line 192: lines.remove("{}={}".format(sc.GENERATION, 1)) Line 193: md = volume.VolumeMetadata.from_lines(lines) -- To view, visit https://gerrit.ovirt.org/64484 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icb71e1fc78f6c1e411e725b26c48411ffd04d0b6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
