Nir Soffer has posted comments on this change.

Change subject: storage: Move detect_format to new workarounds module
......................................................................


Patch Set 1:

(4 comments)

Nice, I like this name, this makes working on vdsm more fun.

https://gerrit.ovirt.org/#/c/64229/1/tests/storage_workarounds_test.py
File tests/storage_workarounds_test.py:

Line 31: 
Line 32: 
Line 33: class DetectFormatTest(VdsmTestCase):
Line 34: 
Line 35:     def make_volumes(self, env, size, src_md_fmt, src_qemu_fmt, 
dst_md_fmt):
Are you sure we need both src and dst format? We use this workaround when we 
copy a volume to another domain, the format of the vdsm images should always be 
the same.

So we can simplify this to:

    make_volumes(self, env, size, md_format, real_format)

Also, this does not use self, so better make it a function in the module.
Line 36:         src_img_id = str(uuid.uuid4())
Line 37:         src_vol_id = str(uuid.uuid4())
Line 38:         dst_img_id = str(uuid.uuid4())
Line 39:         dst_vol_id = str(uuid.uuid4())


Line 49:         When the volume size matches the VM configuration disk size 
and the
Line 50:         source volume reports COW even though qemuimg reports RAW then 
we
Line 51:         expect the workaround to report both volumes as RAW.
Line 52:         """
Line 53:         size = workarounds.VM_CONF_SIZE_BLK * sc.BLOCK_SIZE
Having VM_CONF_SIZE (in bytes) would be much nicer, at leat for the tests.
Line 54:         with fake_file_env() as env:
Line 55:             src, dst = self.make_volumes(env, size, sc.COW_FORMAT,
Line 56:                                          qemuimg.FORMAT.RAW, 
sc.RAW_FORMAT)
Line 57:             self.assertEqual((qemuimg.FORMAT.RAW, qemuimg.FORMAT.RAW),


Line 52:         """
Line 53:         size = workarounds.VM_CONF_SIZE_BLK * sc.BLOCK_SIZE
Line 54:         with fake_file_env() as env:
Line 55:             src, dst = self.make_volumes(env, size, sc.COW_FORMAT,
Line 56:                                          qemuimg.FORMAT.RAW, 
sc.RAW_FORMAT)
This is not clear.

Also mixing both sc.XXX_FORMAT and qemuimg.FORMAT_XXX make this even more 
confusing.

It is very easy to understand this workaround when you read the code, but 
extremely hard to understand if the tests are correct.

It will look like this (based on my previous comment):

    src, dst = make_volumes(env, size, md_fmt="qcow2", real_fmt="raw")

Now, we have 3 tests which are mostly the same, the only change is the size, 
and the formats.

So we can write one test, inlining the make_volume helper, and use permutations 
to try all combinations.

    @permutaions([
        # args, expected

        # When the volume size match, the metadata format is "qcow2"
        # but the actual format is "raw", we use "raw" format.
        (dict(size=VM_CONF_SIZE, md_fmt="qcow2", real_fmt="raw"),
         dict(src_fmt="raw", dst_fmt="raw")),

        # When the volume size does not match, the workaround
        # is not activated.
        ...
    ])
    def test_detect_format(self, args, expected):
Line 57:             self.assertEqual((qemuimg.FORMAT.RAW, qemuimg.FORMAT.RAW),
Line 58:                              workarounds.detect_format(src, dst))
Line 59: 
Line 60:     def test_bad_format_other_size(self):


https://gerrit.ovirt.org/#/c/64229/1/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 39: import imageSharing
Line 40: from vdsm.exception import ActionStopped
Line 41: import task
Line 42: import resourceManager as rm
Line 43: import workarounds
:-)
Line 44: 
Line 45: log = logging.getLogger('storage.Image')
Line 46: rmanager = rm.ResourceManager.getInstance()
Line 47: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If4da9d2c16679f99b55438d7336d0cfb27429c12
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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

Reply via email to