Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 17:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: sdm: Use guarded.context in copy_data
..


sdm: Use guarded.context in copy_data

Take the appropriate ResourceManager locks and a Volume Lease on the
destination volume when performing a copy_data operation.

Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/61693
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
---
M tests/storage_sdm_copy_data_test.py
M vdsm/storage/sdm/api/copy_data.py
2 files changed, 86 insertions(+), 24 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 16: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 16:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 15: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 15:

(4 comments)

https://gerrit.ovirt.org/#/c/61693/14/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 39
Line 40
Line 41
Line 42
Line 43
> This tests copying divs, maybe rename to TestCopyDataDIV?
Done


Line 48
Line 49
Line 50
Line 51
Line 52
> Same
Done


https://gerrit.ovirt.org/#/c/61693/15/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 40: 
Line 41: import storage.sdm.api.copy_data
Line 42: 
Line 43: 
Line 44: class fake_guarded_context(object):
> Why use non standard class name?
context managers are supposed to be lowercase.  that's why we have 
guarded.context and not guarded.Context
Line 45: 
Line 46: def __init__(self):
Line 47: self.locks = None
Line 48: 


Line 47: self.locks = None
Line 48: 
Line 49: def __call__(self, locks=None):
Line 50: if locks is not None:
Line 51: self.locks = locks
> Why do we need this check? each time we call this, we want to save the new 
Nope.  We call it when checking the locks (to get the instance) and in that 
case we don't want to overwrite the locks that were set by copy_data.
Line 52: return self
Line 53: 
Line 54: def __enter__(self):
Line 55: return self


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 15: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/61693/15/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 40: 
Line 41: import storage.sdm.api.copy_data
Line 42: 
Line 43: 
Line 44: class fake_guarded_context(object):
Why use non standard class name?
Line 45: 
Line 46: def __init__(self):
Line 47: self.locks = None
Line 48: 


Line 47: self.locks = None
Line 48: 
Line 49: def __call__(self, locks=None):
Line 50: if locks is not None:
Line 51: self.locks = locks
Why do we need this check? each time we call this, we want to save the new 
locks - always.
Line 52: return self
Line 53: 
Line 54: def __enter__(self):
Line 55: return self


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 15:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 14:

(14 comments)

https://gerrit.ovirt.org/#/c/61693/14/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 39
Line 40
Line 41
Line 42
Line 43
> This tests copying divs, maybe rename to TestCopyDataDIV?
Done


Line 48
Line 49
Line 50
Line 51
Line 52
> Same
Done


Line 48: self._ctx = self.fake_guarded_context(locks)
Line 49: return self._ctx
Line 50: 
Line 51: def validate(self, locks):
Line 52: return sorted(locks) == sorted(self._ctx.locks)
> This create very bad failures:
Done
Line 53: 
Line 54: class fake_guarded_context(object):
Line 55: 
Line 56: def __init__(self, locks):


Line 48: self._ctx = self.fake_guarded_context(locks)
Line 49: return self._ctx
Line 50: 
Line 51: def validate(self, locks):
Line 52: return sorted(locks) == sorted(self._ctx.locks)
> This create very bad failures:
Done
Line 53: 
Line 54: class fake_guarded_context(object):
Line 55: 
Line 56: def __init__(self, locks):


Line 59: def __enter__(self):
Line 60: return self
Line 61: 
Line 62: def __exit__(self, exc_type, exc_val, exc_tb):
Line 63: pass
> We can make it much simpler:
Done
Line 64: 
Line 65: 
Line 66: @expandPermutations
Line 67: class CopyDataTests(VdsmTestCase):


Line 59: def __enter__(self):
Line 60: return self
Line 61: 
Line 62: def __exit__(self, exc_type, exc_val, exc_tb):
Line 63: pass
> We can make it much simpler:
Done
Line 64: 
Line 65: 
Line 66: @expandPermutations
Line 67: class CopyDataTests(VdsmTestCase):


Line 74: def get_vols(self, storage_type, src_fmt, dst_fmt, chain_length=1):
Line 75: with fake_env(storage_type) as env:
Line 76: rm = FakeResourceManager()
Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
> Why not mock the guarded.context? This way we don't have to create a fake m
Done
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
Line 81: (resourceManager.ResourceManager,
Line 82:  'getInstance', lambda cls: rm),


Line 76: rm = FakeResourceManager()
Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
> Same, we don't call any method of VolumeLease, right?
Done
Line 81: (resourceManager.ResourceManager,
Line 82:  'getInstance', lambda cls: rm),
Line 83: (blockVolume, 'rmanager', rm),
Line 84: ]):


Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
Line 81: (resourceManager.ResourceManager,
> Do we realy need to mock resourceManager when we mock the context?
Done
Line 82:  'getInstance', lambda cls: rm),
Line 83: (blockVolume, 'rmanager', rm),
Line 84: ]):
Line 85: src_vols = make_qemu_chain(env, self.SIZE, src_fmt,


Line 102: 
Line 103: def expected_locks(self, src_vol, dst_vol):
Line 104: # We expect:
Line 105: # 1) a domain lock for each volume
Line 106: # 2) an image lock in exclusive mode for each volume
> Exclusive only for the destination volume.
Done
Line 107: # 3) Destination volume lease
Line 108: src_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
src_vol.sdUUID)
Line 109: dst_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
dst_vol.sdUUID)
Line 110: ret = [


Line 106: # 2) an image lock in exclusive mode for each volume
Line 107: # 3) Destination volume lease
Line 108: src_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
src_vol.sdUUID)
Line 109: dst_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
dst_vol.sdUUID)
Line 110: ret = [
> # domain lock or each volume
Done
Line 111: resourceManager.ResourceManagerLock(
Line 112: sc.STORAGE, src_vol.sdUUID, 
resourceManager.LockType.shared),
Line 113: resourceManager.ResourceManagerLock(
Line 114: sc.STORAGE, dst_vol.sdUUID, 
resourceManager.LockType.shared),


Line 110: ret = [
Line 111: resourceManager.ResourceManagerLock(
Line 112: sc.STORAGE, src_vol.sdUUID, 
resourceManager.LockType.shared),
Line 113:  

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 14: Code-Review-1

(12 comments)

https://gerrit.ovirt.org/#/c/61693/14/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 39
Line 40
Line 41
Line 42
Line 43
This tests copying divs, maybe rename to TestCopyDataDIV?

We will probably have later TestCopyDataDIVCeph etc.


Line 48
Line 49
Line 50
Line 51
Line 52
Same


Line 48: self._ctx = self.fake_guarded_context(locks)
Line 49: return self._ctx
Line 50: 
Line 51: def validate(self, locks):
Line 52: return sorted(locks) == sorted(self._ctx.locks)
This create very bad failures:

True != False

Instead of the very useful output of assertEqual.
Line 53: 
Line 54: class fake_guarded_context(object):
Line 55: 
Line 56: def __init__(self, locks):


Line 59: def __enter__(self):
Line 60: return self
Line 61: 
Line 62: def __exit__(self, exc_type, exc_val, exc_tb):
Line 63: pass
We can make it much simpler:

class FakeContext(object):
def __init__(self):
self.locks = []
def __enter__(self, locks):
self.locks = locks
return self
def __exit__(self, *args):
pass

And do this for each test:

@MonkeyPatch(gourded, 'context', FakeContext())
def test_foo():
...
self.assertEqual(sorted(guarded.context.locks),
sorted(expected_locks))
Line 64: 
Line 65: 
Line 66: @expandPermutations
Line 67: class CopyDataTests(VdsmTestCase):


Line 74: def get_vols(self, storage_type, src_fmt, dst_fmt, chain_length=1):
Line 75: with fake_env(storage_type) as env:
Line 76: rm = FakeResourceManager()
Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
Why not mock the guarded.context? This way we don't have to create a fake 
module, only a fake context manager.
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
Line 81: (resourceManager.ResourceManager,
Line 82:  'getInstance', lambda cls: rm),


Line 76: rm = FakeResourceManager()
Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
Same, we don't call any method of VolumeLease, right?
Line 81: (resourceManager.ResourceManager,
Line 82:  'getInstance', lambda cls: rm),
Line 83: (blockVolume, 'rmanager', rm),
Line 84: ]):


Line 77: with MonkeyPatchScope([
Line 78: (storage.sdm.api.copy_data, 'guarded', self._guarded),
Line 79: (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
Line 80: (volume, 'sdCache', env.sdcache),
Line 81: (resourceManager.ResourceManager,
Do we realy need to mock resourceManager when we mock the context?
Line 82:  'getInstance', lambda cls: rm),
Line 83: (blockVolume, 'rmanager', rm),
Line 84: ]):
Line 85: src_vols = make_qemu_chain(env, self.SIZE, src_fmt,


Line 102: 
Line 103: def expected_locks(self, src_vol, dst_vol):
Line 104: # We expect:
Line 105: # 1) a domain lock for each volume
Line 106: # 2) an image lock in exclusive mode for each volume
Exclusive only for the destination volume.

Since this text kind of duplicate the lock list below, maybe integrate the text 
in the list?
Line 107: # 3) Destination volume lease
Line 108: src_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
src_vol.sdUUID)
Line 109: dst_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
dst_vol.sdUUID)
Line 110: ret = [


Line 106: # 2) an image lock in exclusive mode for each volume
Line 107: # 3) Destination volume lease
Line 108: src_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
src_vol.sdUUID)
Line 109: dst_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, 
dst_vol.sdUUID)
Line 110: ret = [
# domain lock or each volume
Line 111: resourceManager.ResourceManagerLock(
Line 112: sc.STORAGE, src_vol.sdUUID, 
resourceManager.LockType.shared),
Line 113: resourceManager.ResourceManagerLock(
Line 114: sc.STORAGE, dst_vol.sdUUID, 
resourceManager.LockType.shared),


Line 110: ret = [
Line 111: resourceManager.ResourceManagerLock(
Line 112: sc.STORAGE, src_vol.sdUUID, 
resourceManager.LockType.shared),
Line 113: 

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 14: Verified+1

Verified through unit testing and functional testing of the copy_data verb.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 14:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 13:

(4 comments)

https://gerrit.ovirt.org/#/c/61693/13/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 136
Line 137
Line 138
Line 139
Line 140
> We need to write these tests now.
Let's save this for a follow-up patch.  This patch is about adding locking, not 
adding these tests.


Line 116: 
Line 117: with checked_locks() as checker:
Line 118: job.run()
Line 119: wait_for_job(job)
Line 120: checker.validate(self.get_lock_list(src_vol, dst_vol))
> I don't think that anyone can understand from this code what is the expecte
Good idea.  This also avoids the need for the LockChecker infrastructure.
Line 121: 
Line 122: self.assertEqual(jobs.STATUS.DONE, job.status)
Line 123: self.assertEqual(100.0, job.progress)
Line 124: self.assertNotIn('error', job.info())


https://gerrit.ovirt.org/#/c/61693/13/vdsm/storage/sdm/api/copy_data.py
File vdsm/storage/sdm/api/copy_data.py:

Line 95
Line 96
Line 97
Line 98
Line 99
> We must implement this todo to finish this milestone, you can add it in the
Will do.


Line 96: self._vol = None
Line 97: 
Line 98: # @property
Line 99: # def host_id(self):
Line 100: # return self._host_id
> Please remove if not needed.
Done
Line 101: 
Line 102: @property
Line 103: def locks(self):
Line 104: img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, self.sd_id)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 13:

(4 comments)

https://gerrit.ovirt.org/#/c/61693/13/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 136
Line 137
Line 138
Line 139
Line 140
We need to write these tests now.


Line 116: 
Line 117: with checked_locks() as checker:
Line 118: job.run()
Line 119: wait_for_job(job)
Line 120: checker.validate(self.get_lock_list(src_vol, dst_vol))
I don't think that anyone can understand from this code what is the expected 
behavior of the code (e.g. what locks were taken and in which order).

I think we should use a much simpler approach - we can mock guarded.context, so 
we test only that we passed the correct locks. We already tested that 
guarded.context does the right thing with locks. this will make the test much 
simpler.

This assumes that the code is using guarded.context - theoretically someone can 
write perfect code without it, but the test will fail, but practically we want 
all code to use guarded.context for locking, so this is not a real issue.

I would test this in another test that does not do any data copying (mock 
qemu.convert?), and test only the locks passed to the context while job was 
running.
Line 121: 
Line 122: self.assertEqual(jobs.STATUS.DONE, job.status)
Line 123: self.assertEqual(100.0, job.progress)
Line 124: self.assertNotIn('error', job.info())


https://gerrit.ovirt.org/#/c/61693/13/vdsm/storage/sdm/api/copy_data.py
File vdsm/storage/sdm/api/copy_data.py:

Line 95
Line 96
Line 97
Line 98
Line 99
We must implement this todo to finish this milestone, you can add it in the 
next patch if you like.


Line 96: self._vol = None
Line 97: 
Line 98: # @property
Line 99: # def host_id(self):
Line 100: # return self._host_id
Please remove if not needed.
Line 101: 
Line 102: @property
Line 103: def locks(self):
Line 104: img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, self.sd_id)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 13:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 12:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 11:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 10:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 9:

By unit tests and the new copy_data flow.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 9:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 8:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 7:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


Patch Set 6:

* 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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: sdm: Use guarded.context in copy_data
..

sdm: Use guarded.context in copy_data

Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
Signed-off-by: Adam Litke 
---
M tests/storage_sdm_copy_data_test.py
M vdsm/storage/sdm/api/copy_data.py
2 files changed, 71 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/61693/1

diff --git a/tests/storage_sdm_copy_data_test.py 
b/tests/storage_sdm_copy_data_test.py
index def8934..45c5540 100644
--- a/tests/storage_sdm_copy_data_test.py
+++ b/tests/storage_sdm_copy_data_test.py
@@ -33,8 +33,11 @@
 from vdsm import jobs
 from vdsm import qemuimg
 from vdsm.storage import constants as sc
+from vdsm.storage import guarded
 
 from storage import blockVolume
+from storage import resourceManager as rm
+from storage import sd
 
 import storage.sdm.api.copy_data
 
@@ -49,6 +52,7 @@
 rm = FakeResourceManager()
 with MonkeyPatchScope([
 (storage.sdm.api.copy_data, 'sdCache', env.sdcache),
+(guarded, 'rmanager', rm),
 (blockVolume, 'rmanager', rm),
 ]):
 src_vols = make_qemu_chain(env, self.SIZE, src_fmt,
@@ -68,6 +72,40 @@
 qemuimg.create(vol.volumePath, size=self.SIZE,
format=qemuimg.FORMAT.QCOW2, backing=backing)
 return vol
+
+def check_locks(self, storage_type, src, dst):
+src_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, src.sdUUID)
+dst_img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, dst.sdUUID)
+locks = sorted({
+guarded.ResourceManagerLock(sc.STORAGE, src.sdUUID,
+rm.LockType.shared),
+guarded.ResourceManagerLock(sc.STORAGE, dst.sdUUID,
+rm.LockType.shared),
+guarded.ResourceManagerLock(src_img_ns, src.imgUUID,
+rm.LockType.shared),
+guarded.ResourceManagerLock(dst_img_ns, dst.imgUUID,
+rm.LockType.exclusive),
+})
+if storage_type == 'block':
+# NOTE: LVM locks are taken implicitly as part of block volume
+# prepare.  As a result they are acquired in the order that the
+# prepare is called (src -> dst).  This probably should be changed
+# but we'd have to add a bulk prepare function that uses guarded.
+locks.extend([
+guarded.ResourceManagerLock(
+sd.getNamespace(sc.LVM_ACTIVATION_NAMESPACE, src.sdUUID),
+src.volUUID, rm.LockType.shared),
+guarded.ResourceManagerLock(
+sd.getNamespace(sc.LVM_ACTIVATION_NAMESPACE, dst.sdUUID),
+dst.volUUID, rm.LockType.exclusive),
+])
+
+expected_acquires = [('acquireResource', (l.ns, l.name, l.mode), {})
+ for l in locks]
+expected_releases = [('releaseResource', (l.ns, l.name), {})
+ for l in reversed(locks)]
+calls = guarded.rmanager.__calls__
+self.assertEqual(expected_acquires + expected_releases, calls)
 
 @permutations((
 ('file', 'raw', 'raw'),
@@ -106,6 +144,7 @@
 verify_qemu_chain(dst_chain)
 self.assertEqual(sc.fmt2str(dst_fmt),
  qemuimg.info(dst_vol.volumePath)['format'])
+self.check_locks(env_type, src_vol, dst_vol)
 
 @permutations((
 ('file', 'raw', 'raw', (0, 1)),
diff --git a/vdsm/storage/sdm/api/copy_data.py 
b/vdsm/storage/sdm/api/copy_data.py
index aff0185..b9dd555 100644
--- a/vdsm/storage/sdm/api/copy_data.py
+++ b/vdsm/storage/sdm/api/copy_data.py
@@ -26,7 +26,10 @@
 from vdsm import properties
 from vdsm import qemuimg
 from vdsm.storage import constants as sc
+from vdsm.storage import guarded
 
+from storage import resourceManager as rm
+from storage import sd
 from storage import volume
 from storage.sdc import sdCache
 
@@ -42,8 +45,8 @@
 
 def __init__(self, job_id, host_id, source, destination):
 super(Job, self).__init__(job_id, 'copy_data', host_id)
-self._source = _create_endpoint(source)
-self._dest = _create_endpoint(destination)
+self._source = _create_endpoint(source, writable=False)
+self._dest = _create_endpoint(destination, writable=True)
 self._operation = None
 
 @property
@@ -55,26 +58,26 @@
 self._operation.abort()
 
 def _run(self):
-# TODO: LOCKING!
-with self._source.prepare(), self._dest.prepare(writable=True):
-# Do not start copying if we have already been aborted
-if self._status == jobs.STATUS.ABORTED:
-return
+with 

Change in vdsm[master]: sdm: Use guarded.context in copy_data

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Use guarded.context in copy_data
..


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/61693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org