Adam Litke has uploaded a new change for review.
Change subject: storage: Introduce VolumeManifest.datapath_session ......................................................................
storage: Introduce VolumeManifest.datapath_session
When performing datapath operations on a volume (eg. copying data) we mark the volume ILLEGAL before starting the operation and only mark the volume LEGAL again once the operation is finished. As long as this is all done with the volume lease held the engine can poll the volume from any host to determine if the operation is running (lease held) and can detect an interrupted/failed operation (lease free and volume ILLEGAL).
Later this contextmanager will be expanded to support volume generation incrementation when exiting successfully which will also allow engine to determine if an operation was completed successfully.
All metadata updates must be performed to a single block with one write in order to ensure atomicity.
Change-Id: I30a3ac2971411778d24e007aac9fcb3009edb4c4 Signed-off-by: Adam Litke alitke@redhat.com --- M tests/storage_sdm_copy_data_test.py M tests/storage_volume_test.py M vdsm/storage/sdm/api/copy_data.py M vdsm/storage/volume.py 4 files changed, 104 insertions(+), 8 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/64362/1
diff --git a/tests/storage_sdm_copy_data_test.py b/tests/storage_sdm_copy_data_test.py index c0ec533..93f55c6 100644 --- a/tests/storage_sdm_copy_data_test.py +++ b/tests/storage_sdm_copy_data_test.py @@ -221,7 +221,40 @@ # Qemu pads the file to a 1k boundary with null bytes self.assertTrue(f.read().startswith(vm_conf_data))
+ @permutations((('file',), ('block',))) + def test_datapath_session(self, env_type): + job_id = str(uuid.uuid4()) + fmt = sc.RAW_FORMAT + with self.get_vols(env_type, fmt, fmt) as (src_chain, dst_chain): + src_vol = src_chain[0] + dst_vol = dst_chain[0] + source = dict(endpoint_type='div', sd_id=src_vol.sdUUID, + img_id=src_vol.imgUUID, vol_id=src_vol.volUUID) + dest = dict(endpoint_type='div', sd_id=dst_vol.sdUUID, + img_id=dst_vol.imgUUID, vol_id=dst_vol.volUUID) + self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality()) + self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality()) + fake_convert = FakeQemuConvertChecker(src_vol, dst_vol) + with MonkeyPatchScope([(qemuimg, 'convert', fake_convert)]): + job = storage.sdm.api.copy_data.Job(job_id, 0, source, dest) + job.run() + wait_for_job(job) + self.assertEqual(jobs.STATUS.DONE, job.status) + self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality()) + self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality()) + # TODO: Missing tests: # Copy between 2 different domains # Abort before copy # Abort during copy + + +class FakeQemuConvertChecker(object): + def __init__(self, src_vol, dst_vol): + self.src_vol = src_vol + self.dst_vol = dst_vol + + def __call__(self, *args, **kwargs): + assert sc.LEGAL_VOL == self.src_vol.getLegality() + assert sc.ILLEGAL_VOL == self.dst_vol.getLegality() + return qemuimg.QemuImgOperation(['/bin/true']) diff --git a/tests/storage_volume_test.py b/tests/storage_volume_test.py index 79cae70..2231148 100644 --- a/tests/storage_volume_test.py +++ b/tests/storage_volume_test.py @@ -19,10 +19,12 @@ #
from __future__ import absolute_import +import uuid
from monkeypatch import MonkeyPatchScope from storagefakelib import FakeStorageDomainCache from storagetestlib import FakeSD +from storagetestlib import fake_env from testlib import expandPermutations, permutations from testlib import recorded from testlib import VdsmTestCase @@ -34,6 +36,7 @@ from storage import volume
HOST_ID = 1 +MB = 1048576
class FakeSDManifest(object): @@ -92,3 +95,45 @@ self.assertEqual(expected[:1], manifest.__calls__) lock.release() self.assertEqual(expected, manifest.__calls__) + + +@expandPermutations +class VolumeManifestTest(VdsmTestCase): + + def test_datapath_session(self): + img_id = str(uuid.uuid4()) + vol_id = str(uuid.uuid4()) + + with fake_env('file') as env: + env.make_volume(MB, img_id, vol_id) + vol = env.sd_manifest.produceVolume(img_id, vol_id) + vol.setMetadata = CountedInstanceMethod(vol.setMetadata) + self.assertEqual(sc.LEGAL_VOL, vol.getLegality()) + with vol.datapath_session(): + self.assertEqual(sc.ILLEGAL_VOL, vol.getLegality()) + self.assertEqual(1, vol.setMetadata.nr_calls) + self.assertEqual(sc.LEGAL_VOL, vol.getLegality()) + self.assertEqual(2, vol.setMetadata.nr_calls) + + def test_datapath_session_fail_inside_context(self): + img_id = str(uuid.uuid4()) + vol_id = str(uuid.uuid4()) + + with fake_env('file') as env: + env.make_volume(MB, img_id, vol_id) + vol = env.sd_manifest.produceVolume(img_id, vol_id) + self.assertEqual(sc.LEGAL_VOL, vol.getLegality()) + with self.assertRaises(ValueError): + with vol.datapath_session(): + raise ValueError() + self.assertEqual(sc.ILLEGAL_VOL, vol.getLegality()) + + +class CountedInstanceMethod(object): + def __init__(self, method): + self._method = method + self.nr_calls = 0 + + def __call__(self, *args, **kwargs): + self.nr_calls += 1 + return self._method(*args, **kwargs) diff --git a/vdsm/storage/sdm/api/copy_data.py b/vdsm/storage/sdm/api/copy_data.py index d55e651..0304fed 100644 --- a/vdsm/storage/sdm/api/copy_data.py +++ b/vdsm/storage/sdm/api/copy_data.py @@ -73,14 +73,15 @@ src_format = self._source.qemu_format dst_format = self._dest.qemu_format
- self._operation = qemuimg.convert( - self._source.path, - self._dest.path, - srcFormat=src_format, - dstFormat=dst_format, - backing=self._dest.backing_path, - backingFormat=self._dest.backing_qemu_format) - self._operation.wait_for_completion() + with self._dest.datapath_session(): + self._operation = qemuimg.convert( + self._source.path, + self._dest.path, + srcFormat=src_format, + dstFormat=dst_format, + backing=self._dest.backing_path, + backingFormat=self._dest.backing_qemu_format) + self._operation.wait_for_completion()
def _create_endpoint(params, host_id, writable): @@ -103,6 +104,7 @@ self._host_id = host_id self._writable = writable self._vol = None + self.datapath_session = None
@property def locks(self): @@ -146,7 +148,9 @@ dom = sdCache.produce_manifest(self.sd_id) self._vol = dom.produceVolume(self.img_id, self.vol_id) self._vol.prepare(rw=self._writable, justme=True) + self.datapath_session = self._vol.datapath_session try: yield finally: + self.datapath_session = None self._vol.teardown(self.sd_id, self.vol_id, justme=True) diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py index 1339d95..0c01e1c 100644 --- a/vdsm/storage/volume.py +++ b/vdsm/storage/volume.py @@ -497,6 +497,20 @@ """ pass
+ @contextmanager + def datapath_session(self): + """ + Must be called with the Volume Lease held. + + In order to detect interrupted datapath operations a volume should be + marked ILLEGAL prior to the first modification of data and subsequently + marked LEGAL again once the operation has completed. Thus, if an + interruption occurs the volume will remain in an ILLEGAL state. + """ + self.setLegality(sc.ILLEGAL_VOL) + yield + self.setLegality(sc.LEGAL_VOL) +
class Volume(object): log = logging.getLogger('storage.Volume')