Change in vdsm[master]: Introduce VolumeArtifacts

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 30 13:04:57 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
......................................................................


Patch Set 25:

(9 comments)

Partial review

https://gerrit.ovirt.org/#/c/48097/25/debian/vdsm.install
File debian/vdsm.install:

Line 111: ./usr/share/vdsm/storage/sd.py
Line 112: ./usr/share/vdsm/storage/sdc.py
Line 113: ./usr/share/vdsm/storage/sdm/__init__.py
Line 114: ./usr/share/vdsm/storage/sdm/volume_artifacts.py
Line 115: ./usr/share/vdsm/storage/securable.py
I this related? looks like bad merge.
Line 116: ./usr/share/vdsm/storage/sp.py
Line 117: ./usr/share/vdsm/storage/spbackends.py
Line 118: ./usr/share/vdsm/storage/storageServer.py
Line 119: ./usr/share/vdsm/storage/storage_mailbox.py


https://gerrit.ovirt.org/#/c/48097/25/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1774: 
Line 1775: 
Line 1776: #################################################
Line 1777: #  SDM Errors
Line 1778: #  Range: 909-?
910
Line 1779: #################################################
Line 1780: 
Line 1781: class DomainHasGarbage(StorageException):
Line 1782:     code = 910


https://gerrit.ovirt.org/#/c/48097/25/tests/Makefile.am
File tests/Makefile.am:

Line 144: 	vmTests.py \
Line 145: 	vmTestsData.py \
Line 146: 	vmUtilsTests.py \
Line 147: 	vmXmlTests.py \
Line 148: 	volume_artifacts_test.py \
Lets start adding storage_ prefix to our tests.

When we move all storage code to lib/vdsm/ we can move all the tests to tests/storage. Until we can, having storage_ prefix make it easy to run all the storage tests using storage_*_test.py.
Line 149: 	v2vTests.py \
Line 150: 	$(NULL)
Line 151: 
Line 152: nodist_vdsmtests_PYTHON = \


https://gerrit.ovirt.org/#/c/48097/25/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 36: class ExpectedFailure(Exception):
Line 37:     pass
Line 38: 
Line 39: 
Line 40: BASE_RAW_PARAMS = (1073741824, volume.RAW_FORMAT, image.SYSTEM_DISK_TYPE, '')
Lets add a non-empty description (.e.g "raw volume") to make this more clear.
Line 41: BASE_COW_PARAMS = (1073741824, volume.COW_FORMAT, image.SYSTEM_DISK_TYPE, '')
Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):


Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):
Line 45:     artifacts_class = env.sd_manifest.get_volume_artifacts_class()
Line 46:     return artifacts_class(env.sd_manifest, img_id, vol_id)
This looks like useful method for StorageDomainManifest - why do we return the class instead of creating an instance?
Line 47: 
Line 48: 
Line 49: class VolumeArtifactsTestsMixin(object):
Line 50: 


Line 57:             artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 58:             self.assertFalse(artifacts.is_garbage())
Line 59:             self.assertFalse(artifacts.is_image())
Line 60:             self.assertRaises(ValidationError,
Line 61:                               self.validate_artifacts, artifacts)
Can we simply assert inside validate_artifacts? Is there a reason to raise special error and assert here?
Line 62: 
Line 63:     def test_state_garbage_volatile_image_dir(self):
Line 64:         with self.fake_env() as env:
Line 65:             artifacts = get_artifacts(env, self.img_id, self.vol_id)


Line 67:             self.assertTrue(artifacts.is_garbage())
Line 68:             self.assertFalse(artifacts.is_image())
Line 69:             self.validate_artifacts(artifacts)
Line 70: 
Line 71:     def test_state_garbage_create(self):
test_state_garbage_create_raises?
Line 72:         with self.fake_env() as env:
Line 73:             artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 74:             artifacts.create(*BASE_RAW_PARAMS)
Line 75:             self.assertRaises(se.DomainHasGarbage, artifacts.create,


Line 200:         self.assertTrue(
Line 201:             os.path.basename(path).startswith(sd.REMOVED_IMAGE_PREFIX))
Line 202:         self.assertTrue(os.path.exists(path))
Line 203:         self.assertFalse(os.path.exists(artifacts._image_dir))
Line 204:         self.assertEqual(nr_files, len(os.listdir(path)))
I don't think we should count files but check which files exists instead. This will make the tests more clear and prevent errors when wrong code create one file (but the wrong one) and the test pass.
Line 205: 
Line 206:     def validate_metadata_artifact(self, artifacts, dirlist):
Line 207:         metafile = os.path.basename(artifacts.meta_volatile_path)
Line 208:         self.assertIn(metafile, dirlist)


Line 204:         self.assertEqual(nr_files, len(os.listdir(path)))
Line 205: 
Line 206:     def validate_metadata_artifact(self, artifacts, dirlist):
Line 207:         metafile = os.path.basename(artifacts.meta_volatile_path)
Line 208:         self.assertIn(metafile, dirlist)
I don't like the idea of getting file list and adding helper to check if a file exists in the list. This may be better if you care about performance, but for test is better to assert that:

    self.assertTrue(os.path.exists(path))

Or if you want to have a better message, add a helper for checking if file exists:

    def assertFileExists(self, path):
        if not os.path.exists(path):
            raise AssertionError("No such file: %s" % path)

In this case - artifacts actually return the path, so we should do this in the test itself:

    self.assertFileExists(artifacts.meta_volatile_path)
Line 209: 
Line 210:     def validate_lease_artifact(self, artifacts, dirlist):
Line 211:         leasefile = os.path.basename(
Line 212:             artifacts.vol_id + fileVolume.LEASE_FILEEXT)


-- 
To view, visit https://gerrit.ovirt.org/48097
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 25
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list