Adam Litke has posted comments on this change. Change subject: storage: Validate generation id in volume.operation context ......................................................................
Patch Set 1: (11 comments) https://gerrit.ovirt.org/#/c/64486/1/lib/vdsm/storage/exception.py File lib/vdsm/storage/exception.py: Line 1776: Line 1777: Line 1778: ################################################# Line 1779: # SDM Errors Line 1780: # Range: 910-919 > Can you submit this separately? (we can merge this quickly) sure. Line 1781: ################################################# Line 1782: Line 1783: class DomainHasGarbage(StorageException): Line 1784: code = 910 Line 1787: def __init__(self, reason): Line 1788: self.value = reason Line 1789: Line 1790: Line 1791: class InvalidGeneration(StorageException): > GenerationMismatch? Done Line 1792: code = 911 Line 1793: message = "The provided Generation ID is not valid for this Volume" Line 1794: Line 1795: def __init__(self, requested_id, real_id): Line 1789: Line 1790: Line 1791: class InvalidGeneration(StorageException): Line 1792: code = 911 Line 1793: message = "The provided Generation ID is not valid for this Volume" > The provided generation does match the actual volume generation. Done Line 1794: Line 1795: def __init__(self, requested_id, real_id): Line 1791: class InvalidGeneration(StorageException): Line 1792: code = 911 Line 1793: message = "The provided Generation ID is not valid for this Volume" Line 1794: Line 1795: def __init__(self, requested_id, real_id): > requested_generation, real_generation Done Line 1792: code = 911 Line 1793: message = "The provided Generation ID is not valid for this Volume" Line 1794: Line 1795: def __init__(self, requested_id, real_id): Line 1796: self.value = "requested id:%i, actual_id:%i" % (requested_id, real_id) > requested=%s, actual=%s? Done https://gerrit.ovirt.org/#/c/64486/1/tests/storage_volume_test.py File tests/storage_volume_test.py: Line 134: vol_id = str(uuid.uuid4()) Line 135: generation = 100 Line 136: Line 137: with fake_env('file') as env: Line 138: env.make_volume(MB, img_id, vol_id) > Add generation parameter here? This would make lot of tests easier later. Most tests don't care about the generation. Let's leave it for now as we can always change it later. Line 139: vol = env.sd_manifest.produceVolume(img_id, vol_id) Line 140: vol.setMetaParam(sc.GENERATION, 100) Line 141: with vol.operation(generation): Line 142: pass Line 151: vol = env.sd_manifest.produceVolume(img_id, vol_id) Line 152: vol.setMetaParam(sc.GENERATION, generation) Line 153: with self.assertRaises(se.InvalidGeneration): Line 154: with vol.operation(generation + 1): Line 155: pass > Lets test also genration - 1 Done Line 156: Line 157: def test_get_info_generation_id(self): Line 158: img_id = str(uuid.uuid4()) Line 159: vol_id = str(uuid.uuid4()) https://gerrit.ovirt.org/#/c/64486/1/vdsm/storage/volume.py File vdsm/storage/volume.py: Line 498: """ Line 499: pass Line 500: Line 501: @contextmanager Line 502: def operation(self, requested_gen_id=None): > requested_generation Done Line 503: """ Line 504: Must be called with the Volume Lease held. Line 505: Line 506: In order to detect interrupted datapath operations a volume should be Line 509: interruption occurs the volume will remain in an ILLEGAL state. Line 510: Line 511: If generation is provided we check that the volume's generation matches Line 512: """ Line 513: real_gen_id = self.getMetaParam(sc.GENERATION) > real_generation actual_gen Line 514: if requested_gen_id is not None and real_gen_id != requested_gen_id: Line 515: raise se.InvalidGeneration(requested_gen_id, real_gen_id) Line 516: self.setLegality(sc.ILLEGAL_VOL) Line 517: yield Line 511: If generation is provided we check that the volume's generation matches Line 512: """ Line 513: real_gen_id = self.getMetaParam(sc.GENERATION) Line 514: if requested_gen_id is not None and real_gen_id != requested_gen_id: Line 515: raise se.InvalidGeneration(requested_gen_id, real_gen_id) > The _id suffix not only incorrect but make the code harder to read. Done Line 516: self.setLegality(sc.ILLEGAL_VOL) Line 517: yield Line 518: self.setLegality(sc.LEGAL_VOL) Line 519: Line 517: yield Line 518: self.setLegality(sc.LEGAL_VOL) Line 519: Line 520: Line 521: > Unintended I guess? Done Line 522: class Volume(object): Line 523: log = logging.getLogger('storage.Volume') Line 524: manifestClass = VolumeManifest Line 525: -- To view, visit https://gerrit.ovirt.org/64486 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77d9c6cb46053ab32c59c77599b7c1366e1c8196 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
