Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

-- 
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has submitted this change and it was merged.

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


storage: Move detect_format to new workarounds module

The function Image._detect_format is a workaround for a bug where the
vdsm metadata disagrees with qemuimg about the format of a volume.
While the underlying bug has been fixed, we still need to maintain this
code in order to prevent problems with preexisting affected volumes.

For reusability and maintainability, move this function to a new module
called workarounds.py and add some basic unit tests.  This code will be
reused by sdm.copy_data in a followup patch.

Change-Id: If4da9d2c16679f99b55438d7336d0cfb27429c12
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/64229
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
Tested-by: Nir Soffer 
---
M lib/vdsm/storage/Makefile.am
A lib/vdsm/storage/workarounds.py
M tests/Makefile.am
A tests/storage_workarounds_test.py
M vdsm.spec.in
M vdsm/storage/image.py
6 files changed, 151 insertions(+), 26 deletions(-)

Approvals:
  Nir Soffer: Verified; Looks good to me, approved
  Jenkins CI: Passed CI tests



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If4da9d2c16679f99b55438d7336d0cfb27429c12
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 5: Verified+1

The new module is well tested, and code calling it looks ok, I think this is 
good enough.

-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 5: Code-Review+2

Added the missing __future__ import.

-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 4: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/64229/4/lib/vdsm/storage/workarounds.py
File lib/vdsm/storage/workarounds.py:

Line 16: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
02110-1301 USA
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Missing absolute_import in lib/vdsm/storage/workarounds.py
Line 21: import logging
Line 22: 
Line 23: from vdsm import qemuimg
Line 24: from vdsm.storage import constants as sc


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread alitke 
Adam Litke has posted comments on this change.

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


Patch Set 2:

(5 comments)

https://gerrit.ovirt.org/#/c/64229/2/vdsm/storage/Makefile.am
File vdsm/storage/Makefile.am:

Line 55:taskManager.py \
Line 56:task.py \
Line 57:threadPool.py \
Line 58:volume.py \
Line 59:workarounds.py \
> We cannot create new volumes in the vdsm tree - we are trying to move stuff
Done
Line 60:$(NULL)
Line 61: 
Line 62: dist_vdsmexec_SCRIPTS = \
Line 63:curl-img-wrap \


https://gerrit.ovirt.org/#/c/64229/2/vdsm/storage/workarounds.py
File vdsm/storage/workarounds.py:

Line 35: 
Line 36: Since commit 0b61c4851a528fd6354d9ab77a68085c41f35dc9 copy of 
internal raw
Line 37: volumes is done using 'qemu-img convert' instead of invoking 'dd'.
Line 38: 
Line 39: Consequently, exporting VM metadata images (produce during live 
snapshot)
> produced
Done
Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
Line 43: has subsequently been fixed in ovirt-engine


Line 38: 
Line 39: Consequently, exporting VM metadata images (produce during live 
snapshot)
Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
> can you use https://bugzilla.redhat.com/1282239?
Done
Line 43: has subsequently been fixed in ovirt-engine
Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we


Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
Line 43: has subsequently been fixed in ovirt-engine
Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
> Can you use https://gerrit.ovirt.org/48768?
Done
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we
Line 47: must keep using this workaround to avoid problems with copying VM 
disks.
Line 48: """


Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we
Line 47: must keep using this workaround to avoid problems with copying VM 
disks.
Line 48: """
> I don't think any workaround got such quality documentation :-)
Sadly, no.
Line 49: src_format = srcVol.getFormat()
Line 50: size_in_blk = srcVol.getSize()
Line 51: if src_format == sc.COW_FORMAT and size_in_blk == VM_CONF_SIZE_BLK:
Line 52: info = qemuimg.info(srcVol.getVolumePath())


-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 3: Code-Review-1

See comments in version 2.

-- 
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 2:

(7 comments)

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

Line 30: from vdsm.storage import constants as sc
Line 31: 
Line 32: 
Line 33: md_formats = dict(raw=sc.RAW_FORMAT, cow=sc.COW_FORMAT)
Line 34: qemu_formats = dict(raw=qemuimg.FORMAT.RAW, cow=qemuimg.FORMAT.QCOW2)
Nice!
Line 35: VM_CONF_SIZE = workarounds.VM_CONF_SIZE_BLK * sc.BLOCK_SIZE
Line 36: 
Line 37: 
Line 38: def make_volume(env, size, md_fmt, real_fmt):


Line 40: vol_id = str(uuid.uuid4())
Line 41: env.make_volume(size, img_id, vol_id, 
vol_format=md_formats[md_fmt])
Line 42: vol = env.sd_manifest.produceVolume(img_id, vol_id)
Line 43: qemuimg.create(vol.getVolumePath(), size, qemu_formats[real_fmt])
Line 44: return vol
Neat
Line 45: 
Line 46: 
Line 47: class DetectFormatTest(VdsmTestCase):
Line 48: 


https://gerrit.ovirt.org/#/c/64229/2/vdsm/storage/Makefile.am
File vdsm/storage/Makefile.am:

Line 55:taskManager.py \
Line 56:task.py \
Line 57:threadPool.py \
Line 58:volume.py \
Line 59:workarounds.py \
We cannot create new volumes in the vdsm tree - we are trying to move stuff to 
lib/vdsm. Please move this to lib/vdsm/storage/workarounds.py
Line 60:$(NULL)
Line 61: 
Line 62: dist_vdsmexec_SCRIPTS = \
Line 63:curl-img-wrap \


https://gerrit.ovirt.org/#/c/64229/2/vdsm/storage/workarounds.py
File vdsm/storage/workarounds.py:

Line 35: 
Line 36: Since commit 0b61c4851a528fd6354d9ab77a68085c41f35dc9 copy of 
internal raw
Line 37: volumes is done using 'qemu-img convert' instead of invoking 'dd'.
Line 38: 
Line 39: Consequently, exporting VM metadata images (produce during live 
snapshot)
produced
Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
Line 43: has subsequently been fixed in ovirt-engine


Line 38: 
Line 39: Consequently, exporting VM metadata images (produce during live 
snapshot)
Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
can you use https://bugzilla.redhat.com/1282239?
Line 43: has subsequently been fixed in ovirt-engine
Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we


Line 40: fails on qemu-img convert - since the images 'impersonate' to 
qcow2 (the
Line 41: format in .meta file is cow, whereas the real format is raw).  
This problem
Line 42: is documented by 
https://bugzilla.redhat.com/show_bug.cgi?id=1282239 and
Line 43: has subsequently been fixed in ovirt-engine
Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
Can you use https://gerrit.ovirt.org/48768?
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we
Line 47: must keep using this workaround to avoid problems with copying VM 
disks.
Line 48: """


Line 44: (see https://gerrit.ovirt.org/#/c/48768/).
Line 45: 
Line 46: Since VM metadata volumes with this problem may still exist in 
storage we
Line 47: must keep using this workaround to avoid problems with copying VM 
disks.
Line 48: """
I don't think any workaround got such quality documentation :-)
Line 49: src_format = srcVol.getFormat()
Line 50: size_in_blk = srcVol.getSize()
Line 51: if src_format == sc.COW_FORMAT and size_in_blk == VM_CONF_SIZE_BLK:
Line 52: info = qemuimg.info(srcVol.getVolumePath())


-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-22 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-21 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-21 Thread alitke 
Adam Litke has posted comments on this change.

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


Patch Set 1:

(5 comments)

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 w
Done
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 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):
> Looking at the next version, we should just have make_volume() here, and cr
Done
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.
Solved by just using a constant at the top of the file.
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.
Applied the suggestions about using friendly format strings.
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):


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)
> Looking in the next version, I don't know if it worth to add permutations h
ok.
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):


-- 
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-20 Thread nsoffer 
Nir Soffer has posted comments on this change.

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


Patch Set 1:

(2 comments)

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 w
Looking at the next version, we should just have make_volume() here, and create 
2 volumes in each test.
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 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.
Looking in the next version, I don't know if it worth to add permutations here, 
since we are going to drop these tests in the next version, so lest keep the 
functions.
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):


-- 
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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-20 Thread nsoffer 
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 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-20 Thread automation 
gerrit-hooks has posted comments on this change.

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


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
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 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: storage: Move detect_format to new workarounds module

2016-09-20 Thread alitke 
Adam Litke has uploaded a new change for review.

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

storage: Move detect_format to new workarounds module

The function Image._detect_format is a workaround for a bug where the
vdsm metadata disagrees with qemuimg about the format of a volume.
While the underlying bug has been fixed, we still need to maintain this
code in order to prevent problems with preexisting affected volumes.

For reusability and maintainability, move this function to a new module
called workarounds.py and add some basic unit tests.  This code will be
reused by sdm.copy_data in a followup patch.

Change-Id: If4da9d2c16679f99b55438d7336d0cfb27429c12
Signed-off-by: Adam Litke 
---
M tests/Makefile.am
A tests/storage_workarounds_test.py
M vdsm.spec.in
M vdsm/storage/Makefile.am
M vdsm/storage/image.py
A vdsm/storage/workarounds.py
6 files changed, 153 insertions(+), 27 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/64229/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1984655..34434d0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -132,6 +132,7 @@
storage_volume_test.py \
storage_volume_artifacts_test.py \
storage_volume_metadata_test.py \
+   storage_workarounds_test.py \
tasksetTests.py \
testlibTests.py \
toolTests.py \
@@ -234,6 +235,7 @@
storage_volume_test.py \
storage_volume_artifacts_test.py \
storage_volume_metadata_test.py \
+   storage_workarounds_test.py \
storagefakelibTests.py \
storagetestlibTests.py \
toolBondingTests.py \
diff --git a/tests/storage_workarounds_test.py 
b/tests/storage_workarounds_test.py
new file mode 100644
index 000..900b512
--- /dev/null
+++ b/tests/storage_workarounds_test.py
@@ -0,0 +1,81 @@
+#
+# Copyright 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+from __future__ import absolute_import
+import uuid
+
+from testlib import VdsmTestCase
+from storagetestlib import fake_file_env
+
+from storage import workarounds
+
+from vdsm import qemuimg
+from vdsm.storage import constants as sc
+
+
+class DetectFormatTest(VdsmTestCase):
+
+def make_volumes(self, env, size, src_md_fmt, src_qemu_fmt, dst_md_fmt):
+src_img_id = str(uuid.uuid4())
+src_vol_id = str(uuid.uuid4())
+dst_img_id = str(uuid.uuid4())
+dst_vol_id = str(uuid.uuid4())
+env.make_volume(size, src_img_id, src_vol_id, vol_format=src_md_fmt)
+env.make_volume(size, dst_img_id, dst_vol_id, vol_format=dst_md_fmt)
+src_vol = env.sd_manifest.produceVolume(src_img_id, src_vol_id)
+dst_vol = env.sd_manifest.produceVolume(dst_img_id, dst_vol_id)
+qemuimg.create(src_vol.getVolumePath(), size, src_qemu_fmt)
+return src_vol, dst_vol
+
+def test_bad_format_vm_conf_disk(self):
+"""
+When the volume size matches the VM configuration disk size and the
+source volume reports COW even though qemuimg reports RAW then we
+expect the workaround to report both volumes as RAW.
+"""
+size = workarounds.VM_CONF_SIZE_BLK * sc.BLOCK_SIZE
+with fake_file_env() as env:
+src, dst = self.make_volumes(env, size, sc.COW_FORMAT,
+ qemuimg.FORMAT.RAW, sc.RAW_FORMAT)
+self.assertEqual((qemuimg.FORMAT.RAW, qemuimg.FORMAT.RAW),
+ workarounds.detect_format(src, dst))
+
+def test_bad_format_other_size(self):
+"""
+When the volume size does not match the VM configuration disk size then
+the workaround will not be activated even when the formats don't match
+"""
+size = 2 * workarounds.VM_CONF_SIZE_BLK * sc.BLOCK_SIZE
+with fake_file_env() as env:
+src, dst = self.make_volumes(env, size, sc.COW_FORMAT,
+ qemuimg.FORMAT.RAW, sc.RAW_FORMAT)
+self.assertEqual((qemuimg.FORMAT.QCOW2, qemuimg.FORMAT.RAW),
+ workarounds.detect_format(src, dst))
+
+