Nir Soffer has posted comments on this change.
Change subject: storage: Introduce VolumeManifest.datapath_session ......................................................................
Patch Set 1: Code-Review-1
(8 comments)
https://gerrit.ovirt.org/#/c/64362/1//COMMIT_MSG Commit Message:
Line 17: incrementation when exiting successfully which will also allow engine to Line 18: determine if an operation was completed successfully. Line 19: Line 20: All metadata updates must be performed to a single block with one write Line 21: in order to ensure atomicity. Maybe separate the new context manger and its tests from the usage of this infrastructure? Line 22: Line 23: Change-Id: I30a3ac2971411778d24e007aac9fcb3009edb4c4
https://gerrit.ovirt.org/#/c/64362/1/tests/storage_sdm_copy_data_test.py File tests/storage_sdm_copy_data_test.py:
Line 231: source = dict(endpoint_type='div', sd_id=src_vol.sdUUID, Line 232: img_id=src_vol.imgUUID, vol_id=src_vol.volUUID) Line 233: dest = dict(endpoint_type='div', sd_id=dst_vol.sdUUID, Line 234: img_id=dst_vol.imgUUID, vol_id=dst_vol.volUUID) Line 235: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality()) We don't need this assert, since we don't except any change in the source volume. Line 236: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality()) Line 237: fake_convert = FakeQemuConvertChecker(src_vol, dst_vol) Line 238: with MonkeyPatchScope([(qemuimg, 'convert', fake_convert)]): Line 239: job = storage.sdm.api.copy_data.Job(job_id, 0, source, dest)
Line 239: job = storage.sdm.api.copy_data.Job(job_id, 0, source, dest) Line 240: job.run() Line 241: wait_for_job(job) Line 242: self.assertEqual(jobs.STATUS.DONE, job.status) Line 243: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality()) Same.
If you want to check the source volume, maybe add another test? Line 244: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality()) Line 245: Line 246: # TODO: Missing tests: Line 247: # Copy between 2 different domains
Line 240: job.run() Line 241: wait_for_job(job) Line 242: self.assertEqual(jobs.STATUS.DONE, job.status) Line 243: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality()) Line 244: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality()) It would be nice to test that volume remain illegal when the operation fails. Later we will verify also the generation. Line 245: Line 246: # TODO: Missing tests: Line 247: # Copy between 2 different domains Line 248: # Abort before copy
Line 249: # Abort during copy Line 250: Line 251: Line 252: class FakeQemuConvertChecker(object): Line 253: def __init__(self, src_vol, dst_vol): We can make this more useful by accepting the command to run. To simulate success, call with /bin/true, and to simulate failure, /bin/false. Line 254: self.src_vol = src_vol Line 255: self.dst_vol = dst_vol Line 256: Line 257: def __call__(self, *args, **kwargs):
https://gerrit.ovirt.org/#/c/64362/1/tests/storage_volume_test.py File tests/storage_volume_test.py:
Line 110: vol.setMetadata = CountedInstanceMethod(vol.setMetadata) Line 111: self.assertEqual(sc.LEGAL_VOL, vol.getLegality()) Line 112: with vol.datapath_session(): Line 113: self.assertEqual(sc.ILLEGAL_VOL, vol.getLegality()) Line 114: self.assertEqual(1, vol.setMetadata.nr_calls) I don't think this verification is needed, documenting how the method should be implemented is good enough.
Also we don't care if this method will perform several metadata calls. We care only about setting the volume to illegal when entering the context, and setting it back only if no exception was raised. Line 115: self.assertEqual(sc.LEGAL_VOL, vol.getLegality()) Line 116: self.assertEqual(2, vol.setMetadata.nr_calls) Line 117: Line 118: def test_datapath_session_fail_inside_context(self):
https://gerrit.ovirt.org/#/c/64362/1/vdsm/storage/sdm/api/copy_data.py File vdsm/storage/sdm/api/copy_data.py:
Line 139 Line 140 Line 141 Line 142 Line 143 I think a property would make this much simpler:
@property def datapath_session(self): return self._vol.datapath_session
https://gerrit.ovirt.org/#/c/64362/1/vdsm/storage/volume.py File vdsm/storage/volume.py:
Line 497: """ Line 498: pass Line 499: Line 500: @contextmanager Line 501: def datapath_session(self): I'm not sure about the name.
How about:
- data_operation - operation - transaction
One word describing this concept would be best.
We allready using the concept of operation, (e.g. QemuOperaiton), so having to do operations on volumes inside a volume.operation context manager makes sense. Line 502: """ Line 503: Must be called with the Volume Lease held. Line 504: Line 505: In order to detect interrupted datapath operations a volume should be