Change in vdsm[master]: after_vm_destroy.py: migrate to jsonrpcvdscli

[igoihman]

Irit Goihman has posted comments on this change.

Change subject: after_vm_destroy.py: migrate to jsonrpcvdscli
..


Patch Set 6:

pending jsonrpc cli fix

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f3fa6479dde2c4a1298d0ae167d888d9f7e020a
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: sos: change getConnectedStoragePoolsList result key

[igoihman]

Irit Goihman has posted comments on this change.

Change subject: sos: change getConnectedStoragePoolsList result key
..


Patch Set 3: -Verified

pending jsonrpc cli fix

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I369d1392701d7b0eac2ce73613ba633a9a60d059
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: sos: replace dumpStorageTable with dump_volume_chains

[igoihman]

Irit Goihman has posted comments on this change.

Change subject: sos: replace dumpStorageTable with dump_volume_chains
..


Patch Set 6: -Verified

pending jsonrpc cli fix

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I73a85e6e720b61da1673af7161a21589ade79831
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marina Kalinin 
Gerrit-Reviewer: Oved Ourfali 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: resourceManager: Inline LockType.validate

[alitke]

Adam Litke has posted comments on this change.

Change subject: resourceManager: Inline LockType.validate
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8205fb724d52eeedcc86c0f53656b2502721e1d0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Jenkins CI
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: Introduce VolumeManifest.datapath_session

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: storage: Introduce VolumeManifest.datapath_session
..

storage: Introduce VolumeManifest.datapath_session

When performing datapath operations on a volume (eg. copying data) we
mark the volume ILLEGAL before starting the operation and only mark the
volume LEGAL again once the operation is finished.  As long as this is
all done with the volume lease held the engine can poll the volume from
any host to determine if the operation is running (lease held) and can
detect an interrupted/failed operation (lease free and volume ILLEGAL).

Later this contextmanager will be expanded to support volume generation
incrementation when exiting successfully which will also allow engine to
determine if an operation was completed successfully.

All metadata updates must be performed to a single block with one write
in order to ensure atomicity.

Change-Id: I30a3ac2971411778d24e007aac9fcb3009edb4c4
Signed-off-by: Adam Litke 
---
M tests/storage_sdm_copy_data_test.py
M tests/storage_volume_test.py
M vdsm/storage/sdm/api/copy_data.py
M vdsm/storage/volume.py
4 files changed, 104 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/64362/1

diff --git a/tests/storage_sdm_copy_data_test.py 
b/tests/storage_sdm_copy_data_test.py
index c0ec533..93f55c6 100644
--- a/tests/storage_sdm_copy_data_test.py
+++ b/tests/storage_sdm_copy_data_test.py
@@ -221,7 +221,40 @@
 # Qemu pads the file to a 1k boundary with null bytes
 self.assertTrue(f.read().startswith(vm_conf_data))
 
+@permutations((('file',), ('block',)))
+def test_datapath_session(self, env_type):
+job_id = str(uuid.uuid4())
+fmt = sc.RAW_FORMAT
+with self.get_vols(env_type, fmt, fmt) as (src_chain, dst_chain):
+src_vol = src_chain[0]
+dst_vol = dst_chain[0]
+source = dict(endpoint_type='div', sd_id=src_vol.sdUUID,
+  img_id=src_vol.imgUUID, vol_id=src_vol.volUUID)
+dest = dict(endpoint_type='div', sd_id=dst_vol.sdUUID,
+img_id=dst_vol.imgUUID, vol_id=dst_vol.volUUID)
+self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality())
+self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
+fake_convert = FakeQemuConvertChecker(src_vol, dst_vol)
+with MonkeyPatchScope([(qemuimg, 'convert', fake_convert)]):
+job = storage.sdm.api.copy_data.Job(job_id, 0, source, dest)
+job.run()
+wait_for_job(job)
+self.assertEqual(jobs.STATUS.DONE, job.status)
+self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality())
+self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
+
 # TODO: Missing tests:
 # Copy between 2 different domains
 # Abort before copy
 # Abort during copy
+
+
+class FakeQemuConvertChecker(object):
+def __init__(self, src_vol, dst_vol):
+self.src_vol = src_vol
+self.dst_vol = dst_vol
+
+def __call__(self, *args, **kwargs):
+assert sc.LEGAL_VOL == self.src_vol.getLegality()
+assert sc.ILLEGAL_VOL == self.dst_vol.getLegality()
+return qemuimg.QemuImgOperation(['/bin/true'])
diff --git a/tests/storage_volume_test.py b/tests/storage_volume_test.py
index 79cae70..2231148 100644
--- a/tests/storage_volume_test.py
+++ b/tests/storage_volume_test.py
@@ -19,10 +19,12 @@
 #
 
 from __future__ import absolute_import
+import uuid
 
 from monkeypatch import MonkeyPatchScope
 from storagefakelib import FakeStorageDomainCache
 from storagetestlib import FakeSD
+from storagetestlib import fake_env
 from testlib import expandPermutations, permutations
 from testlib import recorded
 from testlib import VdsmTestCase
@@ -34,6 +36,7 @@
 from storage import volume
 
 HOST_ID = 1
+MB = 1048576
 
 
 class FakeSDManifest(object):
@@ -92,3 +95,45 @@
 self.assertEqual(expected[:1], manifest.__calls__)
 lock.release()
 self.assertEqual(expected, manifest.__calls__)
+
+
+@expandPermutations
+class VolumeManifestTest(VdsmTestCase):
+
+def test_datapath_session(self):
+img_id = str(uuid.uuid4())
+vol_id = str(uuid.uuid4())
+
+with fake_env('file') as env:
+env.make_volume(MB, img_id, vol_id)
+vol = env.sd_manifest.produceVolume(img_id, vol_id)
+vol.setMetadata = CountedInstanceMethod(vol.setMetadata)
+self.assertEqual(sc.LEGAL_VOL, vol.getLegality())
+with vol.datapath_session():
+self.assertEqual(sc.ILLEGAL_VOL, vol.getLegality())
+self.assertEqual(1, vol.setMetadata.nr_calls)
+self.assertEqual(sc.LEGAL_VOL, vol.getLegality())
+self.assertEqual(2, vol.setMetadata.nr_calls)
+
+def 

Change in vdsm[master]: storage: Introduce VolumeManifest.datapath_session

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Introduce VolumeManifest.datapath_session
..


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30a3ac2971411778d24e007aac9fcb3009edb4c4
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]: Live Merge: Remove volume run link after live merge

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 11:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
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]: Live Merge: Teardown volume on HSM after live merge

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Teardown volume on HSM after live merge
..


Patch Set 5:

* #1377849::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1377849::OK, public bug
* Check Product::#1377849::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
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]: Live Merge: Remove volume run link after live merge

[ahino]

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
..


Patch Set 10:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/10/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821: """
Line 822: volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:   self.sdUUID, imgUUID, volUUID)
Line 824: try:
Line 825: self.log.info("Unlinking volme runtime link: %r", 
volRunLink)
> ./vdsm/storage/blockSD.py:825: undefined name 'volRunLink'
Which comment? I thought I addressed all comments
Line 826: os.unlink(volRunLink)
Line 827: except OSError as e:
Line 828: if e.error != errno.ENOENT:
Line 829: raise


Line 822: volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:   self.sdUUID, imgUUID, volUUID)
Line 824: try:
Line 825: self.log.info("Unlinking volme runtime link: %r", 
volRunLink)
Line 826: os.unlink(volRunLink)
> Same error here.
Done
Line 827: except OSError as e:
Line 828: if e.error != errno.ENOENT:
Line 829: raise
Line 830: self.log.debug("Volume run link %r does not exist", 
volRunlink)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
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: Introduce VolumeManifest.datapath_session

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Introduce VolumeManifest.datapath_session
..


Patch Set 1: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/64362/1//COMMIT_MSG
Commit Message:

Line 17: incrementation when exiting successfully which will also allow engine 
to
Line 18: determine if an operation was completed successfully.
Line 19: 
Line 20: All metadata updates must be performed to a single block with one write
Line 21: in order to ensure atomicity.
Maybe separate the new context manger and its tests from the usage of this 
infrastructure?
Line 22: 
Line 23: Change-Id: I30a3ac2971411778d24e007aac9fcb3009edb4c4


https://gerrit.ovirt.org/#/c/64362/1/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 231: source = dict(endpoint_type='div', sd_id=src_vol.sdUUID,
Line 232:   img_id=src_vol.imgUUID, 
vol_id=src_vol.volUUID)
Line 233: dest = dict(endpoint_type='div', sd_id=dst_vol.sdUUID,
Line 234: img_id=dst_vol.imgUUID, 
vol_id=dst_vol.volUUID)
Line 235: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality())
We don't need this assert, since we don't except any change in the source 
volume.
Line 236: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
Line 237: fake_convert = FakeQemuConvertChecker(src_vol, dst_vol)
Line 238: with MonkeyPatchScope([(qemuimg, 'convert', 
fake_convert)]):
Line 239: job = storage.sdm.api.copy_data.Job(job_id, 0, 
source, dest)


Line 239: job = storage.sdm.api.copy_data.Job(job_id, 0, 
source, dest)
Line 240: job.run()
Line 241: wait_for_job(job)
Line 242: self.assertEqual(jobs.STATUS.DONE, job.status)
Line 243: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality())
Same.

If you want to check the source volume, maybe add another test?
Line 244: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
Line 245: 
Line 246: # TODO: Missing tests:
Line 247: # Copy between 2 different domains


Line 240: job.run()
Line 241: wait_for_job(job)
Line 242: self.assertEqual(jobs.STATUS.DONE, job.status)
Line 243: self.assertEqual(sc.LEGAL_VOL, src_vol.getLegality())
Line 244: self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
It would be nice to test that volume remain illegal when the operation fails. 
Later we will verify also the generation.
Line 245: 
Line 246: # TODO: Missing tests:
Line 247: # Copy between 2 different domains
Line 248: # Abort before copy


Line 249: # Abort during copy
Line 250: 
Line 251: 
Line 252: class FakeQemuConvertChecker(object):
Line 253: def __init__(self, src_vol, dst_vol):
We can make this more useful by accepting the command to run. To simulate 
success, call with /bin/true, and to simulate failure, /bin/false.
Line 254: self.src_vol = src_vol
Line 255: self.dst_vol = dst_vol
Line 256: 
Line 257: def __call__(self, *args, **kwargs):


https://gerrit.ovirt.org/#/c/64362/1/tests/storage_volume_test.py
File tests/storage_volume_test.py:

Line 110: vol.setMetadata = CountedInstanceMethod(vol.setMetadata)
Line 111: self.assertEqual(sc.LEGAL_VOL, vol.getLegality())
Line 112: with vol.datapath_session():
Line 113: self.assertEqual(sc.ILLEGAL_VOL, vol.getLegality())
Line 114: self.assertEqual(1, vol.setMetadata.nr_calls)
I don't think this verification is needed, documenting how the method should be 
implemented is good enough.

Also we don't care if this method will perform several metadata calls. We care 
only about setting the volume to illegal when entering the context, and setting 
it back only if no exception was raised.
Line 115: self.assertEqual(sc.LEGAL_VOL, vol.getLegality())
Line 116: self.assertEqual(2, vol.setMetadata.nr_calls)
Line 117: 
Line 118: def test_datapath_session_fail_inside_context(self):


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

Line 139
Line 140
Line 141
Line 142
Line 143
I think a property would make this much simpler:

@property
def datapath_session(self):
return self._vol.datapath_session


https://gerrit.ovirt.org/#/c/64362/1/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 497: """
Line 498: pass
Line 499: 
Line 500: @contextmanager
Line 501: def datapath_session(self):
I'm not sure about the name.

How about:

- data_operation
- operation
- transaction

One word describing this concept would be best.

We allready using the concept of operation, (e.g. QemuOperaiton), so having to 
do operations on volumes inside a volume.operation context