Nir Soffer has uploaded a new change for review. Change subject: sdm: Remove circular dependency ......................................................................
sdm: Remove circular dependency StorageDomain.get_volume_artifacts() was creating a circular dependency; BlockSD and fileSD depend on volume_artifacts, volume_articats depend on blockVolume, and blockVolume depend on blockSD. The results of this circular dependency is not being to storageServerTests without importing first blockSD, although this module is never used. This patch removes the circular dependency by removing StorageDomain.get_volume_class. Since volume_artifacts must depened on old classes (mainly for using class methods), we cannot have old class depend on it. New factory method volume_artifacts.create() is used to create the correct class for the current storage domain. Change-Id: Ib5f005ed6111b8dd5f90d867aa6693aafe052c6a Signed-off-by: Nir Soffer <[email protected]> --- M tests/storage_sdm_create_volume_test.py M tests/storage_volume_artifacts_test.py M tests/storagetestlib.py M vdsm/storage/blockSD.py M vdsm/storage/fileSD.py M vdsm/storage/sdm/api/create_volume.py M vdsm/storage/sdm/volume_artifacts.py 7 files changed, 69 insertions(+), 85 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/83/57483/1 diff --git a/tests/storage_sdm_create_volume_test.py b/tests/storage_sdm_create_volume_test.py index c888158..11cad05 100644 --- a/tests/storage_sdm_create_volume_test.py +++ b/tests/storage_sdm_create_volume_test.py @@ -34,6 +34,7 @@ from storage import fileVolume, sd from storage import resourceManager as rm from storage.resourceFactories import IMAGE_NAMESPACE +from storage.sdm import volume_artifacts import storage.sdm.api.create_volume @@ -62,10 +63,7 @@ self.releaseDomainLock() def getVolumeClass(self): - return fileVolume.FileVolumeMetadata - - def get_volume_artifacts(self, img_id, vol_id): - return FakeVolumeArtifacts(self, img_id, vol_id) + return fileVolume.FileVolumeManifest class FakeVolumeArtifacts(object): @@ -103,8 +101,10 @@ @contextmanager def _fake_env(self): self.rm = FakeResourceManager() - with MonkeyPatchScope([(storage.sdm.api.create_volume, 'rmanager', - self.rm)]): + with MonkeyPatchScope([ + (storage.sdm.api.create_volume, 'rmanager', self.rm), + (volume_artifacts, 'create', FakeVolumeArtifacts), + ]): yield def test_create_volume(self): diff --git a/tests/storage_volume_artifacts_test.py b/tests/storage_volume_artifacts_test.py index e0fe872..2487464 100644 --- a/tests/storage_volume_artifacts_test.py +++ b/tests/storage_volume_artifacts_test.py @@ -57,8 +57,7 @@ def test_state_missing(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) self.assertFalse(artifacts.is_garbage()) self.assertFalse(artifacts.is_image()) self.assertRaises(AssertionError, @@ -66,8 +65,7 @@ def test_state_garbage_volatile_image_dir(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) self.assertTrue(artifacts.is_garbage()) self.assertFalse(artifacts.is_image()) @@ -75,16 +73,14 @@ def test_state_garbage_create_raises(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) self.assertRaises(se.DomainHasGarbage, artifacts.create, *BASE_RAW_PARAMS) def test_state_image(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) artifacts.commit() self.assertFalse(artifacts.is_garbage()) @@ -92,35 +88,29 @@ def test_create_additional_vol_missing_parent_id(self): with self.fake_env() as env: - first = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + first = env.get_volume_artifacts(self.img_id, self.vol_id) first.create(*BASE_RAW_PARAMS) first.commit() - second = env.sd_manifest.get_volume_artifacts( - self.img_id, str(uuid.uuid4())) + second = env.get_volume_artifacts(self.img_id, str(uuid.uuid4())) self.assertRaises(NotImplementedError, second.create, *BASE_COW_PARAMS) def test_create_additional_raw_vol(self): with self.fake_env() as env: - first = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + first = env.get_volume_artifacts(self.img_id, self.vol_id) first.create(*BASE_RAW_PARAMS) first.commit() - second = env.sd_manifest.get_volume_artifacts( - self.img_id, str(uuid.uuid4())) + second = env.get_volume_artifacts(self.img_id, str(uuid.uuid4())) self.assertRaises(se.InvalidParameterException, second.create, *BASE_RAW_PARAMS) @brokentest("Broken until COW volume support is added") def test_create_same_volume_in_image(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) artifacts.commit() - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) parent = create_volume.ParentVolumeInfo( dict(img_id=self.img_id, vol_id=self.vol_id)) params = BASE_COW_PARAMS + (parent,) @@ -132,8 +122,7 @@ def test_new_image_create_and_commit(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) size, vol_format, disk_type, desc = BASE_RAW_PARAMS artifacts.create(size, vol_format, disk_type, desc) artifacts.commit() @@ -153,18 +142,16 @@ # Artifacts must not be recognized as volumes until commit is called. with self.fake_env() as env: self.assertEqual({}, env.sd_manifest.getAllVolumes()) - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) self.assertEqual({}, env.sd_manifest.getAllVolumes()) artifacts.commit() self.assertIn(self.vol_id, env.sd_manifest.getAllVolumes()) - def validate_domain_has_garbage(self, sd_manifest): + def validate_domain_has_garbage(self, env): # Checks that existing garbage on the storage domain prevents creation # of these artifacts again. - artifacts = sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) self.assertRaises(se.DomainHasGarbage, artifacts.create, *BASE_RAW_PARAMS) @@ -176,8 +163,7 @@ def test_raw_volume_preallocation(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) size, vol_format, disk_type, desc = BASE_RAW_PARAMS artifacts.create(size, vol_format, disk_type, desc) artifacts.commit() @@ -188,62 +174,56 @@ # If we fail before the metadata is created we will have an empty # image directory with a garbage collection prefix left behind with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts._create_metadata_artifact = failure self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_new_image_path(artifacts) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) def test_new_image_create_lease_failure(self): # If we fail before the lease is created we will have a garbage image # directory containing a metadata file with the .artifact extension with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts._create_lease_file = failure self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_new_image_path(artifacts, has_md=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) def test_new_image_create_container_failure(self): # If we fail before the container is created we will have a garbage # image directory containing artifact metadata and a lease file. with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts._create_volume_file = failure self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_new_image_path(artifacts, has_md=True, has_lease=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) def test_garbage_image_dir(self): # Creating the an artifact using an existing garbage image directory is # not allowed. with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts._create_metadata_artifact = failure self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) # Invalid use of artifacts def test_new_image_commit_without_create(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) self.assertRaises(OSError, artifacts.commit) def test_new_image_commit_twice(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) artifacts.commit() self.assertRaises(OSError, artifacts.commit) @@ -288,8 +268,7 @@ with fake_file_env() as env: garbage_img_id = sd.REMOVED_IMAGE_PREFIX + self.img_id self.assertEqual(set(), env.sd_manifest.getAllImages()) - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) self.assertEqual({garbage_img_id}, env.sd_manifest.getAllImages()) artifacts.commit() @@ -303,8 +282,7 @@ def test_raw_volume_preallocation(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) size, vol_format, disk_type, desc = BASE_RAW_PARAMS artifacts.create(size, vol_format, disk_type, desc) artifacts.commit() @@ -318,8 +296,7 @@ vg = env.lvm.getVG(sd_id) expected_size = int(vg.extent_size) requested_size = expected_size - MB - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(requested_size, volume.RAW_FORMAT, image.SYSTEM_DISK_TYPE, 'raw_volume') artifacts.commit() @@ -331,51 +308,48 @@ def test_create_fail_creating_lv(self): # If we fail to create the LV then storage is clean and we can retry with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts(self.img_id, - self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) with MonkeyPatchScope([[env.lvm, 'createLV', failure]]): self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_invisibility(env, artifacts, is_garbage=False) # Storage is clean so we should be able to retry - artifacts = env.sd_manifest.get_volume_artifacts(self.img_id, - self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) def test_create_fail_acquiring_meta_slot(self): # If we fail to acquire the meta_slot we have just a garbage LV with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts(self.img_id, - self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) with MonkeyPatchScope([ [env.sd_manifest, 'acquireVolumeMetadataSlot', failure] ]): self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_invisibility(env, artifacts, is_garbage=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) def test_create_fail_setting_metadata_lvtag(self): # If we fail to set the meta_slot in the LV tags that slot remains # available for allocation (even without garbage collection) with self.fake_env() as env: slot_before = self.get_next_free_slot(env) - artifacts = env.sd_manifest.get_volume_artifacts( + artifacts = env.get_volume_artifacts( self.img_id, self.vol_id) with MonkeyPatchScope([[env.lvm, 'changeLVTags', failure]]): self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.assertEqual(slot_before, self.get_next_free_slot(env)) self.validate_invisibility(env, artifacts, is_garbage=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) def test_create_fail_writing_metadata(self): # If we fail to write metadata we will be left with a garbage LV and an # allocated metadata slot which is not freed until the LV is removed. with self.fake_env() as env: slot_before = self.get_next_free_slot(env) - artifacts = env.sd_manifest.get_volume_artifacts( + artifacts = env.get_volume_artifacts( self.img_id, self.vol_id) with MonkeyPatchScope([ [blockVolume.BlockVolumeManifest, 'newMetadata', failure] @@ -383,13 +357,13 @@ self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_invisibility(env, artifacts, is_garbage=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) self.assertNotEqual(slot_before, self.get_next_free_slot(env)) def test_create_fail_creating_lease(self): # We leave behind a garbage LV and metadata area with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( + artifacts = env.get_volume_artifacts( self.img_id, self.vol_id) with MonkeyPatchScope([ [blockVolume.BlockVolumeManifest, 'newVolumeLease', failure] @@ -397,20 +371,20 @@ self.assertRaises(ExpectedFailure, artifacts.create, *BASE_RAW_PARAMS) self.validate_invisibility(env, artifacts, is_garbage=True) - self.validate_domain_has_garbage(env.sd_manifest) + self.validate_domain_has_garbage(env) # Invalid use of artifacts def test_commit_without_create(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( + artifacts = env.get_volume_artifacts( self.img_id, self.vol_id) self.assertRaises(se.LogicalVolumeDoesNotExistError, artifacts.commit) def test_commit_twice(self): with self.fake_env() as env: - artifacts = env.sd_manifest.get_volume_artifacts( + artifacts = env.get_volume_artifacts( self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) artifacts.commit() @@ -466,8 +440,7 @@ # file implementation. with fake_block_env() as env: self.assertEqual(set(), env.sd_manifest.getAllImages()) - artifacts = env.sd_manifest.get_volume_artifacts( - self.img_id, self.vol_id) + artifacts = env.get_volume_artifacts(self.img_id, self.vol_id) artifacts.create(*BASE_RAW_PARAMS) self.assertEqual(set(), env.sd_manifest.getAllImages()) artifacts.commit() diff --git a/tests/storagetestlib.py b/tests/storagetestlib.py index 1a3dfce..95ef53d 100644 --- a/tests/storagetestlib.py +++ b/tests/storagetestlib.py @@ -41,6 +41,9 @@ if lvm: self.lvm = lvm + def get_volume_artifacts(self, img_id, vol_id): + return volume_artifacts.create(self.sd_manifest, img_id, vol_id) + @contextmanager def fake_file_env(obj=None): diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py index fb56e9f..c3b6eb1 100644 --- a/vdsm/storage/blockSD.py +++ b/vdsm/storage/blockSD.py @@ -45,7 +45,6 @@ import vdsm.supervdsm as svdsm import sd -from sdm import volume_artifacts import lvm import clusterlock import blockVolume @@ -589,9 +588,6 @@ Return a type specific volume generator object """ return blockVolume.BlockVolumeManifest - - def get_volume_artifacts(self, img_id, vol_id): - return volume_artifacts.BlockVolumeArtifacts(self, img_id, vol_id) def _getImgExclusiveVols(self, imgUUID, volsImgs): """Filter vols belonging to imgUUID only.""" diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 86a9c90..300a340 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -33,7 +33,6 @@ from vdsm.storage.persistent import PersistentDict, DictValidator import sd -import sdm.volume_artifacts import fileVolume import outOfProcess as oop from vdsm import constants @@ -202,9 +201,6 @@ Return a type specific volume generator object """ return fileVolume.FileVolumeManifest - - def get_volume_artifacts(self, img_id, vol_id): - return sdm.volume_artifacts.FileVolumeArtifacts(self, img_id, vol_id) def getDeletedImagePath(self, imgUUID): currImgDir = self.getImagePath(imgUUID) diff --git a/vdsm/storage/sdm/api/create_volume.py b/vdsm/storage/sdm/api/create_volume.py index f2f0796..af0df0d 100644 --- a/vdsm/storage/sdm/api/create_volume.py +++ b/vdsm/storage/sdm/api/create_volume.py @@ -27,6 +27,7 @@ from storage import image, sd, volume from storage.resourceFactories import IMAGE_NAMESPACE +from .. import volume_artifacts from . import base rmanager = rm.ResourceManager.getInstance() @@ -46,8 +47,9 @@ IMAGE_NAMESPACE) with rmanager.acquireResource(image_res_ns, self.vol_info.img_id, rm.LockType.exclusive): - artifacts = self.sd_manifest.get_volume_artifacts( - self.vol_info.img_id, self.vol_info.vol_id) + artifacts = volume_artifacts.create(self.sd_manifest, + self.vol_info.img_id, + self.vol_info.vol_id) artifacts.create( self.vol_info.virtual_size, vol_format, self.vol_info.disk_type, self.vol_info.description, diff --git a/vdsm/storage/sdm/volume_artifacts.py b/vdsm/storage/sdm/volume_artifacts.py index e1a1997..f11fb2f 100644 --- a/vdsm/storage/sdm/volume_artifacts.py +++ b/vdsm/storage/sdm/volume_artifacts.py @@ -59,7 +59,21 @@ TEMP_VOL_LVTAG ) -from storage import blockVolume, lvm, volume +from storage import blockVolume, fileVolume, lvm, volume + + +def create(sd_manifest, img_id, vol_id): + """ + Create VolumeArtifacts for given sd_manifest. + """ + vol_class = sd_manifest.getVolumeClass() + + if issubclass(vol_class, blockVolume.BlockVolumeManifest): + return BlockVolumeArtifacts(sd_manifest, img_id, vol_id) + elif issubclass(vol_class, fileVolume.FileVolumeManifest): + return FileVolumeArtifacts(sd_manifest, img_id, vol_id) + + raise RuntimeError("Unsupported volume class: %s", vol_class) class VolumeArtifacts(object): -- To view, visit https://gerrit.ovirt.org/57483 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib5f005ed6111b8dd5f90d867aa6693aafe052c6a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
