Change in vdsm[master]: verify_untrusted_volume: Check compatibility version

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: verify_untrusted_volume: Check compatibility version
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/59411/14/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1509: 
Line 1510: if qemu_format == qemuimg.FORMAT.QCOW2:
Line 1511: # Vdsm depends on qemu-img 2.3.0 or later which always 
reports
Line 1512: # 'compat' for qcow2 volumes.
Line 1513: compat = qemu_info["compat"]
Just to make it clear and consistent with other name here, we should rename 
this to image_compat or qemu_compat.
Line 1514: if not qemuimg.supports_compat(compat):
Line 1515: raise se.ImageVerificationError(
Line 1516: "qcow2 compat %r is not supported" % compat)
Line 1517: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2d5609340ba3bbe00f81d0cde727eb75c94ebec
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: verify_untrusted_volume: Only allow compat designated by config

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: verify_untrusted_volume: Only allow compat designated by config
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/60113/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1516: "qcow2 compat %r is not supported" % compat)
Line 1517: 
Line 1518: # Although we can handle both 0.1 and 1.1 compat qcow2 
files, we
Line 1519: # currently limit support to one or the other via the 
vdsm config.
Line 1520: # Once both are supported concurrently this check can be 
removed.
The config only set the format used to create new images. When you set config 
value to 1.1, you still want to support 0.11.

So we can do:

current_compat = config.get('irs', 'qcow2_compat')
if versioncmp(image_compat, current_compat) > 0:
raise ...

versioncmp() should behave like cmp for version strings. We have similar
code here:
https://gerrit.ovirt.org/#/c/59306/16/build-aux/vercmp

But I think that amending the image to our current format is providing a much 
better user experience, and saving lot of support time.
Line 1521: required_compat = config.get('irs', 'qcow2_compat')
Line 1522: if compat != required_compat:
Line 1523: raise se.ImageVerificationError(
Line 1524: "qcow2 compat %r is not supported by this host" 
% compat)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86da01d885c3f265761fa323aea8b50524c0fcbe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: verify_untrusted_volume: Check compatibility version

[alitke]

Adam Litke has posted comments on this change.

Change subject: verify_untrusted_volume: Check compatibility version
..


Patch Set 14: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2d5609340ba3bbe00f81d0cde727eb75c94ebec
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: verify_untrusted_volume: Only allow compat designated by config

[alitke]

Adam Litke has posted comments on this change.

Change subject: verify_untrusted_volume: Only allow compat designated by config
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86da01d885c3f265761fa323aea8b50524c0fcbe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: verify_untrusted_volume: Only allow compat designated by config

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: verify_untrusted_volume: Only allow compat designated by config
..

verify_untrusted_volume: Only allow compat designated by config

Currently each host sets which qemuimg compat level is allowed.  Use
this setting also during volume verification until 0.10 and 1.1 can be
supported at the same time.

Change-Id: I86da01d885c3f265761fa323aea8b50524c0fcbe
Signed-off-by: Adam Litke 
---
M tests/storage_hsm_test.py
M vdsm/storage/hsm.py
2 files changed, 26 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/13/60113/1

diff --git a/tests/storage_hsm_test.py b/tests/storage_hsm_test.py
index c3504a0..42e8349 100644
--- a/tests/storage_hsm_test.py
+++ b/tests/storage_hsm_test.py
@@ -22,6 +22,7 @@
 from contextlib import contextmanager
 
 from monkeypatch import MonkeyPatchScope
+from testlib import make_config
 from testlib import VdsmTestCase
 from testlib import permutations, expandPermutations
 from storagetestlib import fake_file_env
@@ -85,6 +86,23 @@
   h.verify_untrusted_volume, 'sp',
   vol.sdUUID, vol.imgUUID, vol.volUUID)
 
+@permutations((
+('0.10', '1.1'),
+('1.1', '0.10'),
+))
+def test_disabled_compat_raises(self, qemu_compat, hsm_compat):
+with self.fake_volume(sc.COW_FORMAT) as vol:
+create_conf = make_config([('irs', 'qcow2_compat', qemu_compat)])
+check_conf = make_config([('irs', 'qcow2_compat', hsm_compat)])
+with MonkeyPatchScope([(qemuimg, 'config', create_conf),
+   (hsm, 'config', check_conf)]):
+qemuimg.create(vol.volumePath, size=self.SIZE,
+   format=qemuimg.FORMAT.QCOW2)
+h = FakeHSM()
+self.assertRaises(se.ImageVerificationError,
+  h.verify_untrusted_volume, 'sp',
+  vol.sdUUID, vol.imgUUID, vol.volUUID)
+
 def test_compat_not_checked_for_raw(self):
 with self.fake_volume(sc.RAW_FORMAT) as vol:
 qemu_fmt = qemuimg.FORMAT.RAW
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index e245bf2..101e7f5 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1515,6 +1515,14 @@
 raise se.ImageVerificationError(
 "qcow2 compat %r is not supported" % compat)
 
+# Although we can handle both 0.1 and 1.1 compat qcow2 files, we
+# currently limit support to one or the other via the vdsm config.
+# Once both are supported concurrently this check can be removed.
+required_compat = config.get('irs', 'qcow2_compat')
+if compat != required_compat:
+raise se.ImageVerificationError(
+"qcow2 compat %r is not supported by this host" % compat)
+
 def validateImageMove(self, srcDom, dstDom, imgUUID):
 """
 Determines if the image move is legal.


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I86da01d885c3f265761fa323aea8b50524c0fcbe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: tests: Add tests for verify_untrusted_volume

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: tests: Add tests for verify_untrusted_volume

[alitke]

Adam Litke has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: tests: Refactor hsm import in storage_hsm_test.py

[alitke]

Adam Litke has abandoned this change.

Change subject: tests: Refactor hsm import in storage_hsm_test.py
..


Abandoned

squashed

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: tests: Refactor hsm import in storage_hsm_test.py

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Refactor hsm import in storage_hsm_test.py
..


Patch Set 1:

(1 comment)

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

Line 7: tests: Refactor hsm import in storage_hsm_test.py
Line 8: 
Line 9: We currently import HSM from storage.hsm but a future patch needs to
Line 10: monkeypatch the hsm module.  Change the import statement and rename 
uses
Line 11: of the name 'hsm' in the tests.
Importing the module is better even if we don't need to monkeypatch it. This 
should be default for all imports, except special cases like common decorators 
or context managers.
Line 12: 
Line 13: Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: tests: Refactor hsm import in storage_hsm_test.py

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Refactor hsm import in storage_hsm_test.py
..


Patch Set 1: Code-Review-1

(1 comment)

Why not squash this patch into the original patch?

https://gerrit.ovirt.org/#/c/60112/1/tests/storage_hsm_test.py
File tests/storage_hsm_test.py:

Line 20: 
Line 21: import uuid
Line 22: from contextlib import contextmanager
Line 23: 
Line 24: from monkeypatch import MonkeyPatchScope
pyflakes is angry:
18:44:27 ./tests/storage_hsm_test.py:24: 'MonkeyPatchScope' imported but unused
Line 25: from testlib import VdsmTestCase
Line 26: from testlib import permutations, expandPermutations
Line 27: from storagetestlib import fake_file_env
Line 28: from storagetestlib import make_file_volume


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: verify_untrusted_volume: Check compatibility version

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: verify_untrusted_volume: Check compatibility version
..


Patch Set 12: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/59411/12/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1518: # Although we can handle both 0.1 and 1.1 compat qcow2 
files, we
Line 1519: # currently limit support to one or the other via the 
vdsm config.
Line 1520: # Once both are supported concurrently this check can be 
removed.
Line 1521: required_compat = config.get('irs', 'qcow2_compat')
Line 1522: if compat != required_compat:
Please separate this check to another patch, as we don't have agreement on this 
approach.

I think we should amend the image to the current configured version instead.
Line 1523: raise se.ImageVerificationError(
Line 1524: "qcow2 compat %r is not supported by this host" 
% compat)
Line 1525: 
Line 1526: def validateImageMove(self, srcDom, dstDom, imgUUID):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2d5609340ba3bbe00f81d0cde727eb75c94ebec
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: verify_untrusted_volume: Check compatibility version

[alitke]

Adam Litke has posted comments on this change.

Change subject: verify_untrusted_volume: Check compatibility version
..


Patch Set 12: Verified+1

This version also validates the compat value against the vdsm config file as is 
requested in the bug.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2d5609340ba3bbe00f81d0cde727eb75c94ebec
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: qemuimg: Introduce supports_compat helper

[alitke]

Adam Litke has posted comments on this change.

Change subject: qemuimg: Introduce supports_compat helper
..


Patch Set 12:

Amit, I still think this patch is correct.  See the top patch in this topic for 
how we can validate against the config until both compats are supported 
together.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I341b4559d5d10709fa93b722572cf7f0a8f953ff
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: tests: Refactor hsm import in storage_hsm_test.py

[alitke]

Adam Litke has posted comments on this change.

Change subject: tests: Refactor hsm import in storage_hsm_test.py
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: tests: Refactor hsm import in storage_hsm_test.py

[alitke]

Adam Litke has uploaded a new change for review.

Change subject: tests: Refactor hsm import in storage_hsm_test.py
..

tests: Refactor hsm import in storage_hsm_test.py

We currently import HSM from storage.hsm but a future patch needs to
monkeypatch the hsm module.  Change the import statement and rename uses
of the name 'hsm' in the tests.

Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Signed-off-by: Adam Litke 
---
M tests/storage_hsm_test.py
1 file changed, 9 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/60112/1

diff --git a/tests/storage_hsm_test.py b/tests/storage_hsm_test.py
index 9a518c5..a4052be 100644
--- a/tests/storage_hsm_test.py
+++ b/tests/storage_hsm_test.py
@@ -21,6 +21,7 @@
 import uuid
 from contextlib import contextmanager
 
+from monkeypatch import MonkeyPatchScope
 from testlib import VdsmTestCase
 from testlib import permutations, expandPermutations
 from storagetestlib import fake_file_env
@@ -30,10 +31,10 @@
 from vdsm.storage import constants as sc
 from vdsm.storage import exception as se
 
-from storage.hsm import HSM
+from storage import hsm
 
 
-class FakeHSM(HSM):
+class FakeHSM(hsm.HSM):
 def __init__(self):
 pass
 
@@ -47,8 +48,8 @@
 with self.fake_volume(vol_fmt) as vol:
 qemu_fmt = sc.FMT2STR[vol_fmt]
 qemuimg.create(vol.volumePath, size=self.SIZE, format=qemu_fmt)
-hsm = FakeHSM()
-self.assertNotRaises(hsm.verify_untrusted_volume,
+h = FakeHSM()
+self.assertNotRaises(h.verify_untrusted_volume,
  'sp', vol.sdUUID, vol.imgUUID, vol.volUUID)
 
 @permutations((
@@ -58,9 +59,9 @@
 def test_wrong_format_raises(self, vol_fmt, qemu_fmt):
 with self.fake_volume(vol_fmt) as vol:
 qemuimg.create(vol.volumePath, size=self.SIZE, format=qemu_fmt)
-hsm = FakeHSM()
+h = FakeHSM()
 self.assertRaises(se.ImageVerificationError,
-  hsm.verify_untrusted_volume,
+  h.verify_untrusted_volume,
   'sp', vol.sdUUID, vol.imgUUID, vol.volUUID)
 
 def test_backingfile_raises(self):
@@ -68,9 +69,9 @@
 qemu_fmt = qemuimg.FORMAT.QCOW2
 qemuimg.create(vol.volumePath, size=self.SIZE, format=qemu_fmt,
backing='foo')
-hsm = FakeHSM()
+h = FakeHSM()
 self.assertRaises(se.ImageVerificationError,
-  hsm.verify_untrusted_volume,
+  h.verify_untrusted_volume,
   'sp', vol.sdUUID, vol.imgUUID, vol.volUUID)
 
 @contextmanager


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I36e892b0eaf882e8ad68a3c6070745a4dbc30c94
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: tests: Add tests for verify_untrusted_volume

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/60060/3/tests/storage_hsm_test.py
File tests/storage_hsm_test.py:

Line 79: img_id = str(uuid.uuid4())
Line 80: vol_id = str(uuid.uuid4())
Line 81: make_file_volume(env.sd_manifest, self.SIZE, img_id, 
vol_id,
Line 82:  vol_format=vol_fmt)
Line 83: yield env.sd_manifest.produceVolume(img_id, vol_id)
This fails with:

 17:54:31 ERROR: test_backingfile_raises 
(storage_hsm_test.VerifyUntrustedVolumeTest)
 17:54:31 --
 17:54:31 Traceback (most recent call last):
 17:54:31   File 
"/home/jenkins/workspace/vdsm_master_check-patch-el7-x86_64/vdsm/tests/storage_hsm_test.py",
 line 74, in test_backingfile_raises
 17:54:31 'sp', vol.sdUUID, vol.imgUUID, vol.volUUID)
 17:54:31   File "/usr/lib64/python2.7/unittest/case.py", line 513, in 
assertRaises
 17:54:31 callableObj(*args, **kwargs)
 17:54:31   File 
"/home/jenkins/workspace/vdsm_master_check-patch-el7-x86_64/vdsm/lib/vdsm/logUtils.py",
 line 50, in wrapper
 17:54:31 res = f(*args, **kwargs)
 17:54:31   File 
"/home/jenkins/workspace/vdsm_master_check-patch-el7-x86_64/vdsm/vdsm/storage/hsm.py",
 line 1494, in verify_untrusted_volume
 17:54:31 vol = dom.produceVolume(imgUUID, volUUID)
 17:54:31 AttributeError: 'FakeSD' object has no attribute 'produceVolume'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: tests: Add tests for verify_untrusted_volume

[alitke]

Adam Litke has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: Refactor verify_untrusted_volume

[alitke]

Adam Litke has abandoned this change.

Change subject: Refactor verify_untrusted_volume
..


Abandoned

We're keeping all the logic in the hsm verb.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I90c06286c5e0b85c3b190026882b937fdb38266e
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: tests: Add tests for verify_untrusted_volume

[alitke]

Adam Litke has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/60060/1/tests/Makefile.am
File tests/Makefile.am:

Line 155
Line 156
Line 157
Line 158
Line 159
> Need to add storage_hsm_test.py here
Done


https://gerrit.ovirt.org/#/c/60060/1/tests/storage_hsm_test.py
File tests/storage_hsm_test.py:

Line 49: qemuimg.create(vol.volumePath, size=self.SIZE, 
format=qemu_fmt)
Line 50: hsm = FakeHSM()
Line 51: self.assertNotRaises(hsm.verify_untrusted_volume,
Line 52:  'sp', vol.sdUUID, vol.imgUUID, 
vol.volUUID)
Line 53: 
> We can use permutations to test both cases. The first one is more interesti
I like this suggestion :)  Done.
Line 54: def test_wrong_format_raises(self):
Line 55: with self.fake_volume(sc.RAW_FORMAT) as vol:
Line 56: qemu_fmt = qemuimg.FORMAT.QCOW2
Line 57: qemuimg.create(vol.volumePath, size=self.SIZE, 
format=qemu_fmt)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: hsm: Use manifest in verify_untrusted_volume

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: hsm: Use manifest in verify_untrusted_volume
..


hsm: Use manifest in verify_untrusted_volume

New users of storage objects should generally use the new *Manifest
versions rather than the original objects which have unneeded
complexity.  Also, the new storagetestlib infrastructure is only
compatible with the newer *Manifest objects.

Change-Id: I6e615f0ad97149d49b13b530819d9c8e58fcdb81
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/60059
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
---
M vdsm/storage/hsm.py
1 file changed, 1 insertion(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6e615f0ad97149d49b13b530819d9c8e58fcdb81
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 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: tests: Add tests for verify_untrusted_volume

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 2: Code-Review-1

Please check my comments in version 1:
https://gerrit.ovirt.org/#/c/60060/1/tests/storage_hsm_test.py@53

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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[ovirt-3.6]: xmlrpc: Hide fenceNode password in the log

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: xmlrpc: Hide fenceNode password in the log
..


Patch Set 1:

Are you sure you want to backport these patches to 3.6?

This is risky, and the patches do not fix any critical bug.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I53318ed8ce425f042b5f783af779b540c1dfd7b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
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]: tests: Add tests for verify_untrusted_volume

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/60060/1/tests/Makefile.am
File tests/Makefile.am:

Line 155
Line 156
Line 157
Line 158
Line 159
Need to add storage_hsm_test.py here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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[ovirt-3.6]: xmlrpc: Hide fenceNode password in the log

[fromani]

Francesco Romani has posted comments on this change.

Change subject: xmlrpc: Hide fenceNode password in the log
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I53318ed8ce425f042b5f783af779b540c1dfd7b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
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]: test: Introduce mock module for testing

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: test: Introduce mock module for testing
..


Patch Set 2:

Edward, in which channel the package is available? Did you test it on rhel 
machine configured with the proper channels?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I72edd0a9388f11d905cfbd7d2fac5a95e1212cea
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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[ovirt-3.6]: rpc: Log important info from VM stats

[mzamazal]

Hello Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60101

to review the following change.

Change subject: rpc: Log important info from VM stats
..

rpc: Log important info from VM stats

Currently, we don't log results of getAllVmStats API calls.  This is not
to fill logs with a lot of uninteresting information.  But information
contained in the VM stats is useful and we'd like to have it somewhere.

A possible solution to these contradictory requirements is to log just
important information and to log it only occasionally.  In this patch,
we introduce logging VM ids and states in getAllVmStats calls from time
to time.

We implement this as a rather general functionality.  We are going to
use the same mechanism for other large API outputs in followup patches.

There may be performance concerns but please consider that we perform
data extraction only for calls that are global (not per VM) and that are
not very frequent (like every second or so).  So the processing overhead
shouldn't hurt much and shouldn't be much big in comparison to creating
the returned data inside Vdsm.

Change-Id: Ifcbac615323b62fb9a27e5c0f5a4e98990076146
Signed-off-by: Milan Zamazal 
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Backport-To: 3.6
Reviewed-on: https://gerrit.ovirt.org/58465
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/59997
Reviewed-by: Francesco Romani 
---
M tests/Makefile.am
A tests/logutils_test.py
M vdsm/API.py
M vdsm/logUtils.py
4 files changed, 53 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/01/60101/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index e4c9227..4d82bbf 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -60,6 +60,7 @@
ipwrapperTests.py \
iscsiTests.py \
libvirtconnectionTests.py \
+   logutils_test.py \
lvmTests.py \
main.py \
manifest_tests.py \
diff --git a/tests/logutils_test.py b/tests/logutils_test.py
new file mode 100644
index 000..ce88b02
--- /dev/null
+++ b/tests/logutils_test.py
@@ -0,0 +1,40 @@
+#
+# 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 logUtils import AllVmStatsValue
+
+from testlib import VdsmTestCase as TestCaseBase
+
+
+class TestAllVmStats(TestCaseBase):
+
+_STATS = [{'foo': 'bar',
+   'status': 'Up',
+   'vmId': u'43f02a2d-e563-4f11-a7bc-9ee191cfeba1'},
+  {'foo': 'bar',
+   'status': 'Powering up',
+   'vmId': u'bd0d066b-971e-42f8-8bc6-d647ab7e0e70'}]
+_SIMPLIFIED = ({u'43f02a2d-e563-4f11-a7bc-9ee191cfeba1': 'Up',
+u'bd0d066b-971e-42f8-8bc6-d647ab7e0e70': 'Powering up'})
+
+def test_allvmstats(self):
+data = AllVmStatsValue(self._STATS)
+result = str(data)
+self.assertEqual(eval(result), self._SIMPLIFIED)
diff --git a/vdsm/API.py b/vdsm/API.py
index bbca545..c5a0bc8 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -40,6 +40,7 @@
 from vdsm import constants
 from vdsm import exception
 from vdsm import response
+from vdsm import throttledlog
 import storage.misc
 import storage.clusterlock
 import storage.volume
@@ -56,7 +57,7 @@
 import hooks
 import hostdev
 from caps import PAGE_SIZE_BYTES
-from logUtils import Suppressed
+from logUtils import AllVmStatsValue, Suppressed
 import v2v
 
 import supervdsm
@@ -69,6 +70,8 @@
 
 # default message for system shutdown, will be displayed in guest
 USER_SHUTDOWN_MESSAGE = 'System going down'
+
+throttledlog.throttle('getAllVmStats', 100)
 
 
 def _after_network_setup_fail(networks, bondings, options):
@@ -1355,6 +1358,8 @@
 hooks.before_get_all_vm_stats()
 statsList = self._cif.getAllVmStats()
 statsList = hooks.after_get_all_vm_stats(statsList)
+throttledlog.info('getAllVmStats', "Current getAllVmStats: %s",
+  AllVmStatsValue(statsList))
 return {'status': doneCode, 'statsList': Suppressed(statsList)}
 
 

Change in vdsm[ovirt-3.6]: logging: Introduce throttledlog

[mzamazal]

Hello Piotr Kliczewski, Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60100

to review the following change.

Change subject: logging: Introduce throttledlog
..

logging: Introduce throttledlog

In some situations, we want to log interesting information.  A natural
place to retrieve and log the information is where it is created.
However the corresponding code may be called quite often and we don't
need logging the information that often and fill logs with it
excessively.  It's sufficient to log it just from time to time.

This patch provides means for occasional logging.  The provided
functions actually log only every given number of invocations or every
given time span.  We're going to use the functionality in followup
patches.

Change-Id: Ief96f74eb84880827ccf24c3e7ae854403090b8a
Signed-off-by: Milan Zamazal 
Bug-Url: https://bugzilla.redhat.com/1351247
Reviewed-on: https://gerrit.ovirt.org/59461
Continuous-Integration: Jenkins CI
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Francesco Romani 
Reviewed-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/59995
---
M lib/vdsm/Makefile.am
A lib/vdsm/throttledlog.py
M tests/Makefile.am
A tests/throttledlog_test.py
M vdsm.spec.in
5 files changed, 201 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/60100/1

diff --git a/lib/vdsm/Makefile.am b/lib/vdsm/Makefile.am
index 63fa4f7..f305d01 100644
--- a/lib/vdsm/Makefile.am
+++ b/lib/vdsm/Makefile.am
@@ -45,6 +45,7 @@
sslutils.py \
sysctl.py \
taskset.py \
+   throttledlog.py \
udevadm.py \
utils.py \
vdscli.py \
diff --git a/lib/vdsm/throttledlog.py b/lib/vdsm/throttledlog.py
new file mode 100644
index 000..04cc104
--- /dev/null
+++ b/lib/vdsm/throttledlog.py
@@ -0,0 +1,113 @@
+#
+# 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 logging
+
+from vdsm.utils import monotonic_time
+
+
+_DEFAULT_TIMEOUT_SEC = 3600
+_DEFAULT_LOGGING_LEVEL = logging.DEBUG
+
+_logger = logging.getLogger('throttled')
+
+_periodic = {}
+
+
+class _Periodic(object):
+
+def __init__(self, interval, timeout):
+self._interval = interval
+self._timeout = timeout
+self._counter = 0
+self._last_time = 0
+
+def tick(self):
+now = monotonic_time()
+result = self._result(now)
+self._counter = (self._counter + 1) % self._interval
+if result:
+self._last_time = now
+return result
+
+def _result(self, now):
+return (self._counter == 0 or
+(now - self._last_time) >= self._timeout)
+
+
+def throttle(name, interval, timeout=_DEFAULT_TIMEOUT_SEC):
+"""
+Throttle log messages for `name`, logging at most one message per
+`interval` calls or always after `timeout` seconds of silence.  Throttling
+applies only to logging performed via `log()` function of this module.  The
+first call of `log()` never throttles the log, following calls are
+throttled according to the given parameters.
+
+If this function has already been called for `name`, replace the throttling
+parameters for `name` with the new ones given here and start throttling
+from beginning.
+
+:param name: Arbitrary identifier to be matched in `log()` calls.
+:type name: basestring
+:param interval: The number of `log()` calls that should log at least once.
+:type interval: int
+:param timeout: The number of seconds without log emitted after which
+  `log()` should always unthrottle the next message.
+:type timeout: int
+"""
+_periodic[name] = _Periodic(interval, timeout)
+
+
+def log(name, level, message, *args):
+"""
+Log `message` and `args` if throttling settings for `name` allow it.
+See `throttle()` for information about throttling and `name`.
+`level`, `message` and `args` are passed to `logging.Logger.log()`
+unchanged.
+
+:param name: Arbitrary 

Change in vdsm[ovirt-3.6]: rpc: Use Suppressed class instead of logging workarounds

[mzamazal]

Hello Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60099

to review the following change.

Change subject: rpc: Use Suppressed class instead of logging workarounds
..

rpc: Use Suppressed class instead of logging workarounds

We have got logging workarounds for getAllVmStats RPC calls.
The workarounds are there to suppress logging large data regularly.
We'd like to get rid of the workarounds, it's better to handle the large
data in the API functions, where we can use specific handling of each
kind of large data.

This patch introduces a wrapper for large values, with the purpose of
suppressing writing them to the log.  The wrapper is used for
getAllVmStats so that we get rid of the logging workarounds.

There are other API calls that may produce large outputs, we will handle
them in followup patches.

Backport note: Since logUtils.py is not in lib/vdsm/ yet in 3.6, we
can't use it in yajsonrpc.  We need it there only for a single
isinstance check, so we replaced the check with a class name check.

Change-Id: Idaf00e557fccb8f08fa3aeb38d51cb4bbe0ffe53
Signed-off-by: Milan Zamazal 
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Backport-To: 3.6
Reviewed-on: https://gerrit.ovirt.org/59078
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/59994
Reviewed-by: Francesco Romani 
---
M lib/yajsonrpc/__init__.py
M vdsm/API.py
M vdsm/logUtils.py
M vdsm/rpc/Bridge.py
M vdsm/rpc/bindingxmlrpc.py
5 files changed, 31 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/99/60099/1

diff --git a/lib/yajsonrpc/__init__.py b/lib/yajsonrpc/__init__.py
index f279760..8fc0063 100644
--- a/lib/yajsonrpc/__init__.py
+++ b/lib/yajsonrpc/__init__.py
@@ -497,7 +497,6 @@
 self._attempt_log_stats()
 mangledMethod = req.method.replace(".", "_")
 logLevel = logging.DEBUG
-suppress_logging = mangledMethod in ('Host_getAllVmStats',)
 
 # VDSM should never respond to any request before all information about
 # running VMs is recovered, see https://bugzilla.redhat.com/1339291
@@ -538,9 +537,10 @@
 req.id))
 else:
 res = True if res is None else res
-log_res = "(suppressed)" if suppress_logging else res
 self.log.log(logLevel, "Return '%s' in bridge with %s",
- req.method, log_res)
+ req.method, res)
+if res.__class__.__name__ == 'Suppressed':
+res = res.value
 ctx.requestDone(JsonRpcResponse(res, None, req.id))
 
 @traceback(on=log.name)
diff --git a/vdsm/API.py b/vdsm/API.py
index 396710c..bbca545 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -56,6 +56,7 @@
 import hooks
 import hostdev
 from caps import PAGE_SIZE_BYTES
+from logUtils import Suppressed
 import v2v
 
 import supervdsm
@@ -1354,7 +1355,7 @@
 hooks.before_get_all_vm_stats()
 statsList = self._cif.getAllVmStats()
 statsList = hooks.after_get_all_vm_stats(statsList)
-return {'status': doneCode, 'statsList': statsList}
+return {'status': doneCode, 'statsList': Suppressed(statsList)}
 
 def hostdevListByCaps(self, caps=None):
 devices = hostdev.list_by_caps(caps)
diff --git a/vdsm/logUtils.py b/vdsm/logUtils.py
index 5caa167..76114f0 100644
--- a/vdsm/logUtils.py
+++ b/vdsm/logUtils.py
@@ -189,3 +189,16 @@
 raise RuntimeError(
 "Attempt to open log with incorrect credentials")
 return logging.handlers.WatchedFileHandler._open(self)
+
+
+class Suppressed(object):
+
+def __init__(self, value):
+self._value = value
+
+@property
+def value(self):
+return self._value
+
+def __repr__(self):
+return '(suppressed)'
diff --git a/vdsm/rpc/Bridge.py b/vdsm/rpc/Bridge.py
index 8815a18..a54858e 100644
--- a/vdsm/rpc/Bridge.py
+++ b/vdsm/rpc/Bridge.py
@@ -25,6 +25,7 @@
 
 from vdsm.netinfo import getDeviceByIP
 from vdsm.exception import VdsmException
+from logUtils import Suppressed
 
 
 try:
@@ -237,7 +238,10 @@
 def _fixupRet(self, className, methodName, result):
 retType = self._getRetList(className, methodName)
 if retType is not None:
-self._typeFixup('return', retType, result)
+fixup_result = result
+if isinstance(result, Suppressed):
+fixup_result = result.value
+self._typeFixup('return', retType, fixup_result)
 return result
 
 def _getRetList(self, className, methodName):
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 47e80ac..1ec9d49 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -36,6 +36,7 @@
 from 

Change in vdsm[ovirt-3.6]: rpc: Log RPC call summary on info level

[mzamazal]

Hello Piotr Kliczewski, Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60102

to review the following change.

Change subject: rpc: Log RPC call summary on info level
..

rpc: Log RPC call summary on info level

We currently log relatively detailed information about all RPC calls.

This information is logged on the debug level.  We are going to switch
to the info logging level sometimes in future and then information about
RPC calls won't be logged anymore.  But we still want to see some very
basic information about the RPC calls, such as that they were performed
and how much time they took.

This patch adds the corresponding logging.  We additionally add the
response to the context at a single place as a preparation for the
following patch that logs the response.

Change-Id: Idde2f1ba7394f16770543f5ca13411e8c2339cc6
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Signed-off-by: Milan Zamazal 
Reviewed-on: https://gerrit.ovirt.org/59080
Reviewed-by: Nir Soffer 
Reviewed-by: Piotr Kliczewski 
Continuous-Integration: Jenkins CI
Reviewed-on: https://gerrit.ovirt.org/59998
Reviewed-by: Francesco Romani 
Tested-by: Francesco Romani 
---
M lib/yajsonrpc/__init__.py
M vdsm/rpc/bindingxmlrpc.py
2 files changed, 26 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/02/60102/1

diff --git a/lib/yajsonrpc/__init__.py b/lib/yajsonrpc/__init__.py
index 8fc0063..d02125a 100644
--- a/lib/yajsonrpc/__init__.py
+++ b/lib/yajsonrpc/__init__.py
@@ -494,6 +494,14 @@
 self._counter = 0
 
 def _serveRequest(self, ctx, req):
+start_time = monotonic_time()
+response = self._handle_request(req, ctx.address)
+self.log.info("RPC call %s finished in %.2f seconds",
+  req.method, monotonic_time() - start_time)
+if response is not None:
+ctx.requestDone(response)
+
+def _handle_request(self, req, server_address):
 self._attempt_log_stats()
 mangledMethod = req.method.replace(".", "_")
 logLevel = logging.DEBUG
@@ -504,8 +512,7 @@
 self.log.info("In recovery, ignoring '%s' in bridge with %s",
   req.method, req.params)
 # TODO: take the response from the exception instead of via errCode
-ctx.requestDone(JsonRpcResponse(None, errCode['recovery'], req.id))
-return
+return JsonRpcResponse(None, errCode['recovery'], req.id)
 
 self.log.log(logLevel, "Calling '%s' in bridge with %s",
  req.method, req.params)
@@ -513,35 +520,30 @@
 method = getattr(self._bridge, mangledMethod)
 except AttributeError:
 if req.isNotification():
-return
+return None
 
-ctx.requestDone(JsonRpcResponse(None,
-JsonRpcMethodNotFoundError(),
-req.id))
-return
+return JsonRpcResponse(None, JsonRpcMethodNotFoundError(), req.id)
 
 try:
 params = req.params
-self._bridge.register_server_address(ctx.address)
+self._bridge.register_server_address(server_address)
 if isinstance(req.params, list):
 res = method(*params)
 else:
 res = method(**params)
 self._bridge.unregister_server_address()
 except JsonRpcError as e:
-ctx.requestDone(JsonRpcResponse(None, e, req.id))
+return JsonRpcResponse(None, e, req.id)
 except Exception as e:
 self.log.exception("Internal server error")
-ctx.requestDone(JsonRpcResponse(None,
-JsonRpcInternalError(str(e)),
-req.id))
+return JsonRpcResponse(None, JsonRpcInternalError(str(e)), req.id)
 else:
 res = True if res is None else res
 self.log.log(logLevel, "Return '%s' in bridge with %s",
  req.method, res)
 if res.__class__.__name__ == 'Suppressed':
 res = res.value
-ctx.requestDone(JsonRpcResponse(res, None, req.id))
+return JsonRpcResponse(res, None, req.id)
 
 @traceback(on=log.name)
 def serve_requests(self):
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 1ec9d49..68ba1bd 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -311,6 +311,8 @@
 def _registerFunctions(self):
 def wrapIrsMethod(f):
 def wrapper(*args, **kwargs):
+start_time = utils.monotonic_time()
+
 

Change in vdsm[ovirt-3.6]: rpc: Log also error codes of RPC calls

[mzamazal]

Hello Piotr Kliczewski, Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60103

to review the following change.

Change subject: rpc: Log also error codes of RPC calls
..

rpc: Log also error codes of RPC calls

In one of the previous commits, we added logging of RPC calls on info
level.  It'd be useful to have information about response codes there,
so this patch adds that information.

Change-Id: I776e667fcfd1a20a9ef5f6c638d6c3d950672314
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Signed-off-by: Milan Zamazal 
Reviewed-on: https://gerrit.ovirt.org/59234
Reviewed-by: Nir Soffer 
Reviewed-by: Piotr Kliczewski 
Continuous-Integration: Jenkins CI
Reviewed-on: https://gerrit.ovirt.org/5
Reviewed-by: Francesco Romani 
---
M lib/yajsonrpc/__init__.py
M vdsm/rpc/bindingxmlrpc.py
2 files changed, 36 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/60103/1

diff --git a/lib/yajsonrpc/__init__.py b/lib/yajsonrpc/__init__.py
index d02125a..e3d2d98 100644
--- a/lib/yajsonrpc/__init__.py
+++ b/lib/yajsonrpc/__init__.py
@@ -496,8 +496,22 @@
 def _serveRequest(self, ctx, req):
 start_time = monotonic_time()
 response = self._handle_request(req, ctx.address)
-self.log.info("RPC call %s finished in %.2f seconds",
-  req.method, monotonic_time() - start_time)
+error = getattr(response, "error", None)
+if error is None:
+response_log = "succeeded"
+else:
+if isinstance(error, JsonRpcError):
+error_code = error.code
+elif isinstance(error, dict):
+try:
+error_code = error['status']['code']
+except:
+error_code = '?'
+else:
+error_code = '?'
+response_log = "failed (error %s)" % (error_code,)
+self.log.info("RPC call %s %s in %.2f seconds",
+  req.method, response_log, monotonic_time() - start_time)
 if response is not None:
 ctx.requestDone(response)
 
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 68ba1bd..058f5d7 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -327,15 +327,18 @@
 
 self.log.debug(fmt, *logargs)
 
+res = {}
 try:
-return f(*args, **kwargs)
+res = f(*args, **kwargs)
 except:
 self.log.error("Unexpected exception", exc_info=True)
-return errCode['unexpected']
-finally:
-self.log.info("RPC call %s finished in %.2f seconds",
-  f.__name__,
-  utils.monotonic_time() - start_time)
+res = errCode['unexpected']
+self.log.info("RPC call %s finished (code=%s) in "
+  "%.2f seconds",
+  f.__name__,
+  res.get('status', {}).get('code'),
+  utils.monotonic_time() - start_time)
+return res
 
 wrapper.__name__ = f.__name__
 wrapper.__doc__ = f.__doc__
@@ -1225,6 +1228,7 @@
 def wrapApiMethod(f):
 def wrapper(*args, **kwargs):
 start_time = utils.monotonic_time()
+res = {}
 try:
 logLevel = logging.DEBUG
 suppress_args = f.__name__ in ('fenceNode',)
@@ -1285,18 +1289,23 @@
 except libvirt.libvirtError as e:
 f.__self__.cif.log.error("libvirt error", exc_info=True)
 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
-return errCode['noVM']
+res = errCode['noVM']
 else:
-return errCode['unexpected']
+res = errCode['unexpected']
+return res
 except VdsmException as e:
 f.__self__.cif.log.error("vdsm exception occured", exc_info=True)
-return e.response()
+res = e.response()
+return res
 except:
 f.__self__.cif.log.error("unexpected error", exc_info=True)
-return errCode['unexpected']
+res = errCode['unexpected']
+return res
 finally:
-f.__self__.cif.log.info("RPC call %s finished in %.2f seconds",
+f.__self__.cif.log.info("RPC call %s finished (code=%s) in "
+"%.2f seconds",
 f.__name__,
+res.get('status', {}).get('code'),
 

Change in vdsm[ovirt-3.6]: rpc: Log calls of API methods with possibly large results

[mzamazal]

Hello Nir Soffer, Francesco Romani, Michal Skrivanek,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60098

to review the following change.

Change subject: rpc: Log calls of API methods with possibly large results
..

rpc: Log calls of API methods with possibly large results

We can prevent some API methods from logging their excessively large
results.  But this doesn't mean we shouldn't log their calls at all, we
just shouldn't log the large values.

This patch re-enables logging calls of the methods with suppressed
logging, it just disables logging their outputs.

Change-Id: I85d7f0ceee7e120081e2d9ad4ca3c8148ee70554
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Backport-To: 3.6
Signed-off-by: Milan Zamazal 
Reviewed-on: https://gerrit.ovirt.org/58464
Continuous-Integration: Jenkins CI
Reviewed-by: Francesco Romani 
Reviewed-by: Michal Skrivanek 
Reviewed-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/58709
Continuous-Integration: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/59993
---
M lib/yajsonrpc/__init__.py
M vdsm/rpc/bindingxmlrpc.py
2 files changed, 6 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/98/60098/1

diff --git a/lib/yajsonrpc/__init__.py b/lib/yajsonrpc/__init__.py
index 5aa950f..f279760 100644
--- a/lib/yajsonrpc/__init__.py
+++ b/lib/yajsonrpc/__init__.py
@@ -497,8 +497,7 @@
 self._attempt_log_stats()
 mangledMethod = req.method.replace(".", "_")
 logLevel = logging.DEBUG
-if mangledMethod in ('Host_getAllVmStats',):
-logLevel = logging.TRACE
+suppress_logging = mangledMethod in ('Host_getAllVmStats',)
 
 # VDSM should never respond to any request before all information about
 # running VMs is recovered, see https://bugzilla.redhat.com/1339291
@@ -539,8 +538,9 @@
 req.id))
 else:
 res = True if res is None else res
+log_res = "(suppressed)" if suppress_logging else res
 self.log.log(logLevel, "Return '%s' in bridge with %s",
- req.method, res)
+ req.method, log_res)
 ctx.requestDone(JsonRpcResponse(res, None, req.id))
 
 @traceback(on=log.name)
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 9c1562c..47e80ac 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -1219,8 +1219,7 @@
 def wrapper(*args, **kwargs):
 try:
 logLevel = logging.DEBUG
-if f.__name__ in ('getAllVmStats',):
-logLevel = logging.TRACE
+suppress_logging = f.__name__ in ('getAllVmStats',)
 suppress_args = f.__name__ in ('fenceNode',)
 
 # TODO: This password protection code is fragile and ugly. Password
@@ -1267,8 +1266,9 @@
 res = f(*args, **kwargs)
 else:
 res = errCode['recovery']
+log_res = "(suppressed)" if suppress_logging else res
 f.__self__.cif.log.log(logLevel, 'return %s with %s',
-   f.__name__, res)
+   f.__name__, log_res)
 return res
 except libvirt.libvirtError as e:
 f.__self__.cif.log.error("libvirt error", exc_info=True)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I85d7f0ceee7e120081e2d9ad4ca3c8148ee70554
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Nir Soffer 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-3.6]: xmlrpc: Hide fenceNode password in the log

[mzamazal]

Hello Piotr Kliczewski, Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60096

to review the following change.

Change subject: xmlrpc: Hide fenceNode password in the log
..

xmlrpc: Hide fenceNode password in the log

We enabled logging of fenceNode xmlrpc calls, however there is a
plain password in the call parameters.  This patch prevents the password
from being written to the log.

Password logging seems to be already handled with jsonrpc, so this patch
just fixes xmlrpc.

Change-Id: I53318ed8ce425f042b5f783af779b540c1dfd7b5
Backport-To: 4.0
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1351247
Signed-off-by: Milan Zamazal 
Reviewed-on: https://gerrit.ovirt.org/58833
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
Reviewed-by: Piotr Kliczewski 
Reviewed-on: https://gerrit.ovirt.org/59991
Reviewed-by: Francesco Romani 
---
M vdsm/rpc/bindingxmlrpc.py
1 file changed, 4 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/96/60096/1

diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index acb3bbd..5322935 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -1222,13 +1222,16 @@
 if f.__name__ in ('getVMList', 'getAllVmStats', 'getStats',
   'fenceNode', 'setKsmTune'):
 logLevel = logging.TRACE
+suppress_args = f.__name__ in ('fenceNode',)
 
 # TODO: This password protection code is fragile and ugly. Password
 # protection should be done in the wrapped methods, and logging
 # shold be done in the next layers, similar to storage logs.
 
 displayArgs = args
-if f.__name__ == 'vmDesktopLogin':
+if suppress_args:
+displayArgs = '(suppressed)'
+elif f.__name__ == 'vmDesktopLogin':
 if 'password' in kwargs:
 raise TypeError("Got an unexpected keyword argument: "
 "'password'")


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I53318ed8ce425f042b5f783af779b540c1dfd7b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-3.6]: rpc: Lower logging priority just for getAllVmStats

[mzamazal]

Hello Piotr Kliczewski, Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/60097

to review the following change.

Change subject: rpc: Lower logging priority just for getAllVmStats
..

rpc: Lower logging priority just for getAllVmStats

Currently, we effectively disable logging of calls for several methods.
But we should consider whether it causes more harm to log the
corresponding information or not to log it.

We should avoid logging excessively large messages.  This applies to
getAllVmStats that may report quite a lot of information for many VMs.
But there is probably no good reason not to log everything about the
other methods currently silenced.  So with this patch we suppress
logging just of getAllVmStats.

We should be informed about that method as well, we handle that in
followup patches.

Change-Id: I5eefac9f88ede84df06c0cc39b188c8ae75e456b
Bug-Url: https://bugzilla.redhat.com/1351247
Backport-To: 4.0
Backport-To: 3.6
Signed-off-by: Milan Zamazal 
Reviewed-on: https://gerrit.ovirt.org/58463
Continuous-Integration: Jenkins CI
Reviewed-by: Francesco Romani 
Reviewed-by: Piotr Kliczewski 
Reviewed-on: https://gerrit.ovirt.org/58708
Continuous-Integration: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/59992
Reviewed-by: Nir Soffer 
---
M lib/yajsonrpc/__init__.py
M vdsm/rpc/bindingxmlrpc.py
2 files changed, 2 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/97/60097/1

diff --git a/lib/yajsonrpc/__init__.py b/lib/yajsonrpc/__init__.py
index f824211..5aa950f 100644
--- a/lib/yajsonrpc/__init__.py
+++ b/lib/yajsonrpc/__init__.py
@@ -497,9 +497,7 @@
 self._attempt_log_stats()
 mangledMethod = req.method.replace(".", "_")
 logLevel = logging.DEBUG
-if mangledMethod in ('Host_getVMList', 'Host_getAllVmStats',
- 'Host_getStats', 'StorageDomain_getStats',
- 'VM_getStats', 'Host_fenceNode'):
+if mangledMethod in ('Host_getAllVmStats',):
 logLevel = logging.TRACE
 
 # VDSM should never respond to any request before all information about
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 5322935..9c1562c 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -1219,8 +1219,7 @@
 def wrapper(*args, **kwargs):
 try:
 logLevel = logging.DEBUG
-if f.__name__ in ('getVMList', 'getAllVmStats', 'getStats',
-  'fenceNode', 'setKsmTune'):
+if f.__name__ in ('getAllVmStats',):
 logLevel = logging.TRACE
 suppress_args = f.__name__ in ('fenceNode',)
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5eefac9f88ede84df06c0cc39b188c8ae75e456b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: test: Introduce mock module for testing

[edwardh]

Edward Haas has posted comments on this change.

Change subject: test: Introduce mock module for testing
..


Patch Set 2:

Nir, please check the first comment on this gerrit patch, it has all the 
references.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I72edd0a9388f11d905cfbd7d2fac5a95e1212cea
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: v2v: Log detailed output of virt-v2v

[fromani]

Francesco Romani has posted comments on this change.

Change subject: v2v: Log detailed output of virt-v2v
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/59834/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 811: 
Line 812: def _parse_progress(self, chunk):
Line 813: m = self.DISK_PROGRESS_RE.match(chunk)
Line 814: if m is None:
Line 815: return None
> I didn't add any explicit test about it, but I did change fake-virt-v2v. Th
fine for me.
Line 816: try:
Line 817: return int(m.group(1))
Line 818: except ValueError:
Line 819: raise OutputParserError('error parsing progress regex: %r'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: utils: build cert paths in single place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

OK, let me check hard coding the path. It should simplify bunch of code we have.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Yedidyah Bar David 
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]: utils: build cert paths in single place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:

Line 103
Line 104
Line 105
Line 106
Line 107
> We are already hard coding this value in tool.certificates.PKI_DIR, so this
Additionally, ovirt-imageio-daemon is using the /etc/pki/vdsm/ - we cannot 
support configurable settings here. If you change this value, you break image 
upload and download in 4.0.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Yedidyah Bar David 
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]: utils: build cert paths in single place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

(2 comments)

https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:

Line 263: 
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
> As you can see here we are using it. Do you suggest to hard code this value
It is already hard coded in certificates.py and in libvirt.py (via import). Can 
you explain how this path will work with the hard coded values?
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:

Line 103
Line 104
Line 105
Line 106
Line 107
> In this code we need it. We would need to explore whether we could hard cod
We are already hard coding this value in tool.certificates.PKI_DIR, so this 
means that the current code does not support dynamic pki dir.

Your patch is not a refactoring but adding features that we don't need. I don't 
want to maintain features that nobody asked for. When we find broken feature 
that is not needed we remove it.

If you think this feature is needed and the fact that it does not work is a 
bug, open a bug.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: v2v: Log detailed output of virt-v2v

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: v2v: Log detailed output of virt-v2v
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/59834/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 811: chunk = ''
Line 812: while True:
Line 813: c = stream.read(1)
Line 814: if not c:
Line 815: raise OutputParserError('copy-disk stream closed 
unexpectedly')
> ok, let's add a test about this.
I didn't add any explicit test about it, but I did change fake-virt-v2v. The 
existing tests will fail if the input is not handled properly.
Line 816: chunk += c
Line 817: if c == '\r':
Line 818: yield chunk
Line 819: chunk = ''


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: utils: build cert paths in single place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 157: # localtion of the certificates
Line 158: def get_cert_paths(path):
Line 159: return (os.path.join(path, 'keys', 'vdsmkey.pem'),
Line 160: os.path.join(path, 'certs', 'vdsmcert.pem'),
Line 161: os.path.join(path, 'certs', 'cacert.pem'))
> This module is for constants, not for functions.
We need it.


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:

Line 263: 
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
> We can remove this configuration, it does not work anyway - all the code us
As you can see here we are using it. Do you suggest to hard code this value?
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/tool/configurators/certificates.py
File lib/vdsm/tool/configurators/certificates.py:

Line 30
Line 31
Line 32
Line 33
Line 34
> Please move  the 4 lines above to constants, and use constants.CA_FILE, con
we need path to build the constants.


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:

Line 103
Line 104
Line 105
Line 106
Line 107
> Do we use tsPath? Do we need to support this?
In this code we need it. We would need to explore whether we could hard code 
the path. I am not really sure whether it is good idea.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: migration: wait properly for migration to begin

[tjelinek]

Tomas Jelinek has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/60073/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 766: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
Line 767: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
Line 768: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
Line 769: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
Line 770: stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
> Right, /me read uncarefully.
Done
Line 771: # available since libvirt 1.3
Line 772: stats.get('memory_dirty_rate', -1),
Line 773: # available since libvirt 1.3
Line 774: stats.get('memory_iteration', -1),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: utils: build cert paths in single place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

(3 comments)

I think that the issue is trying to support dynamic pki dir, while some of the 
code is using static one (/etc/pki/vdsm). Lets simplify and support only static 
pki dir and use simple constants in constants.py.

https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:

Line 263: 
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
We can remove this configuration, it does not work anyway - all the code using 
certificates.XXX ignores this value.
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/tool/configurators/certificates.py
File lib/vdsm/tool/configurators/certificates.py:

Line 30
Line 31
Line 32
Line 33
Line 34
Please move  the 4 lines above to constants, and use constants.CA_FILE, 
constants.CERT_FILE and constants.KEY_FILE in the code.


https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:

Line 103
Line 104
Line 105
Line 106
Line 107
Do we use tsPath? Do we need to support this?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: utils: build cert paths in single place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 157: # localtion of the certificates
Line 158: def get_cert_paths(path):
Line 159: return (os.path.join(path, 'keys', 'vdsmkey.pem'),
Line 160: os.path.join(path, 'certs', 'vdsmcert.pem'),
Line 161: os.path.join(path, 'certs', 'cacert.pem'))
This module is for constants, not for functions.

Why do we need the path argument?

If you need to access temporary certificates (e.g. for testing), you can create 
your certificates in a temporary directory and monkeypatch the constants so the 
code accessing them will see the fake certificates.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: qemuimg: Introduce supports_compat helper

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: Introduce supports_compat helper
..


Patch Set 10:

Please rebase this bellow the last patch, so we can take the patches above 
easily.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I341b4559d5d10709fa93b722572cf7f0a8f953ff
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: verify_untrusted_volume: Check compatibility version

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: verify_untrusted_volume: Check compatibility version
..


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/59411/9/tests/storage_hsm_test.py
File tests/storage_hsm_test.py:

Line 75: with self.fake_volume(sc.COW_FORMAT) as vol:
Line 76: qemu_fmt = qemuimg.FORMAT.QCOW2
Line 77: qemuimg.create(vol.volumePath, size=self.SIZE, 
format=qemu_fmt)
Line 78: info = {"format": qemu_fmt, "compat": "BAD"}
Line 79: with MonkeyPatchScope([(qemuimg, 'info', lambda unused: 
info)]):
Monkeypatching works, but we really should support compat when creating images 
- we need this for exporting images to legacy systems. Can we add this easily 
now?
Line 80: hsm = FakeHSM()
Line 81: self.assertRaises(se.ImageVerificationError,
Line 82:   hsm.verify_untrusted_volume, 'sp',
Line 83:   vol.sdUUID, vol.imgUUID, vol.volUUID)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2d5609340ba3bbe00f81d0cde727eb75c94ebec
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
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]: tests: Add tests for verify_untrusted_volume

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Add tests for verify_untrusted_volume
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/60060/1/tests/storage_hsm_test.py
File tests/storage_hsm_test.py:

Line 49: qemuimg.create(vol.volumePath, size=self.SIZE, 
format=qemu_fmt)
Line 50: hsm = FakeHSM()
Line 51: self.assertNotRaises(hsm.verify_untrusted_volume,
Line 52:  'sp', vol.sdUUID, vol.imgUUID, 
vol.volUUID)
Line 53: 
We can use permutations to test both cases. The first one is more interesting 
since it is a security issue, but the second should also fail to imporve the 
user experience.

@permutations([
# vol_format, qemu_format
(sc.RAW_FORMAT, qemuimg.FORMAT.QCOW2),
(sc.COW_FORMAT, qemuimg.FORMAT.ROW),
])
def test_wrong_format(self, vol_format, qemu_format)
Line 54: def test_wrong_format_raises(self):
Line 55: with self.fake_volume(sc.RAW_FORMAT) as vol:
Line 56: qemu_fmt = qemuimg.FORMAT.QCOW2
Line 57: qemuimg.create(vol.volumePath, size=self.SIZE, 
format=qemu_fmt)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44d3bbe33430a34e3a3b985e1485c6d46ebdc3aa
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: jsonrpcvdscli: add additional verbs

[stirabos]

Simone Tiraboschi has posted comments on this change.

Change subject: jsonrpcvdscli: add additional verbs
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I57f9c63a05d8354676e68418b08593ea830b42a7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Simone Tiraboschi 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Rafael Martins 
Gerrit-Reviewer: Simone Tiraboschi 
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]: hsm: Use manifest in verify_untrusted_volume

[alitke]

Adam Litke has posted comments on this change.

Change subject: hsm: Use manifest in verify_untrusted_volume
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e615f0ad97149d49b13b530819d9c8e58fcdb81
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: hostdev: move scsi device code to separate class

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: move scsi device code to separate class
..


Patch Set 8:

(1 comment)

With the exception of the comment below it looks OK to me.

https://gerrit.ovirt.org/#/c/57960/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 447: 
Line 448: return hostdev
Line 449: 
Line 450: @classmethod
Line 451: def update_device_info(cls, vm, device_conf, x):
The method and `x' argument names as usually. :-)
Line 452: alias = 
x.getElementsByTagName('alias')[0].getAttribute('name')
Line 453: host_address = vmxml.device_address(x)
Line 454: 
Line 455: # The routine is quite unusual because we cannot directly


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I784c158192433bba1ea4dd330f8a633024b9fec2
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
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]: yml: return type fixes for Host.hostdevListByCaps

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: yml: return type fixes for Host.hostdevListByCaps
..


Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/59703/2/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

PS2, Line 1392: 
> Missing?
I was not returned from vdsm.


PS2, Line 1410: 
> Um, where did this go?
It was not returned from vdsm.


PS2, Line 1416: 
> Missing?
It was not returned from vdsm.


PS2, Line 1385: HostDeviceAddress: 
> Can we model this as union? There are 3 possible types:
Yes, we could model it as union of three types. Will update


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ad09c310a76dde9267824a337fadf3b1c02bd45
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: hostdev: move usb device code to separate class

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: move usb device code to separate class
..


Patch Set 8:

(1 comment)

With the exception of the comment below it looks OK to me.

https://gerrit.ovirt.org/#/c/57958/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 354: 
Line 355: return hostdev
Line 356: 
Line 357: @classmethod
Line 358: def update_device_info(cls, vm, device_conf, x):
The same about the method name and `x' argument name as in the preceding patch.
Line 359: alias = 
x.getElementsByTagName('alias')[0].getAttribute('name')
Line 360: host_address = vmxml.device_address(x)
Line 361: 
Line 362: # The routine is quite unusual because we cannot directly


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I481a966776089f37971a27637e06d21eafabd05c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: hostdev: use specific device classes in HostDevice

[fromani]

Francesco Romani has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

PS8, Line 320: @classmethod
 : def update_device_info(cls, vm, device_conf):
 : for device_xml in 
vm.domain.get_device_elements('hostdev'):
 : device_type = device_xml.getAttribute('type')
 : cls._DEVICE_MAPPING[device_type].update_device_info(
 : vm, device_conf, device_xml)
> I also prefer not having special cases when creating device classes. Using 
OK, since indeed I don't have any good reason against __new__ and we're 
explored a good number of alternatives, I'm now convinced __new__ is the right 
approach here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: hostdev: move pci device code to separate class

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: move pci device code to separate class
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57956/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 264: 
Line 265: return hostdev
Line 266: 
Line 267: @classmethod
Line 268: def update_device_info(cls, vm, device_conf, x):
> I don't like having the extra `x' argument here, this very confusing wrt. o
Also, please use a better argument name than `x'.
Line 269: alias = 
x.getElementsByTagName('alias')[0].getAttribute('name')
Line 270: address = vmxml.device_address(x)
Line 271: source = x.getElementsByTagName('source')[0]
Line 272: device = pci_address_to_name(**vmxml.device_address(source))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bceb93c2434ff827406bbf4ee0af30f5726f6af
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: test: Introduce mock module for testing

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: test: Introduce mock module for testing
..


Patch Set 2:

Edward, we reverted this last time because the package was not available on 
rhel. Is it available now?

Note that we cannot use the package from epel - the package must exists in rhel 
or in rhev channels.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I72edd0a9388f11d905cfbd7d2fac5a95e1212cea
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: hostdev: use specific device classes in HostDevice

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(2 comments)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 320: @classmethod
Line 321: def update_device_info(cls, vm, device_conf):
Line 322: for device_xml in vm.domain.get_device_elements('hostdev'):
Line 323: device_type = device_xml.getAttribute('type')
Line 324: cls._DEVICE_MAPPING[device_type].update_device_info(
As I've stated in a preceding patch, the called update_device_info should be 
renamed. Let's not forget about it here.


PS8, Line 320: @classmethod
 : def update_device_info(cls, vm, device_conf):
 : for device_xml in 
vm.domain.get_device_elements('hostdev'):
 : device_type = device_xml.getAttribute('type')
 : cls._DEVICE_MAPPING[device_type].update_device_info(
 : vm, device_conf, device_xml)
> That is slightly confusing. I don't understand how we'd create the device c
I also prefer not having special cases when creating device classes. Using 
__new__ here is completely fine from my point of view.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: migration: wait properly for migration to begin

[fromani]

Francesco Romani has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/60073/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 766: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
Line 767: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
Line 768: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
Line 769: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
Line 770: stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
> well, in order to make the call on 658 work, we need this not to fail in fr
Right, /me read uncarefully.

I'd say:
1. let's extract ongoing() and make it a free function like

def ongoing(stats):
try:
job_type = stats['type']
except KeyError:
return False
else:
return job_type != libvirt.VIR_DOMAIN_JOB_NONE

Let's use in line 658 like this

job_stats = self._vm._dom.jobStats()
if not ongoing(job_stats):
# migration not yet started
continue

progress = Progress.from_job_stats(job_stats)

Now we should be good, we can drop this parsing change and keep the one on line 
704.
Would this work for you?
Line 771: # available since libvirt 1.3
Line 772: stats.get('memory_dirty_rate', -1),
Line 773: # available since libvirt 1.3
Line 774: stats.get('memory_iteration', -1),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: tox: fail make process if required tox version isn't installed.

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: tox: fail make process if required tox version isn't installed.
..


Patch Set 16:

(1 comment)

https://gerrit.ovirt.org/#/c/59306/16/build-aux/vercmp
File build-aux/vercmp:

Line 28: if args.verbose:
Line 29: logging.basicConfig(level=logging.DEBUG, format="%(name)s: 
%(message)s")
Line 30: retval = compare_versions(args.actual_version, 
args.required_version)
Line 31: sys.exit(retval)
Line 32: 
2 empty lines need here to make pep8 tool happy. You also need to add this to 
the pep8 whitelist in tox.sh.
Line 33: def compare_versions(actual_version, required_version):
Line 34: actual_version = [int(n) for n in actual_version.split('.')]
Line 35: required_version = [int(n) for n in required_version.split('.')]
Line 36: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim 
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]: yml: return type fixes for Host.hostdevListByCaps

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: yml: return type fixes for Host.hostdevListByCaps
..


Patch Set 2: Code-Review-1

(4 comments)

I have a feeling that some of the values are missing (correct me if I failed to 
understand the schema). The ones I commented are *not* always returned and 
require hostdev / SR-IOV capable host.

https://gerrit.ovirt.org/#/c/59703/2/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

PS2, Line 1392: 
Missing?


PS2, Line 1410: 
Um, where did this go?


PS2, Line 1416: 
Missing?


PS2, Line 1385: HostDeviceAddress: 
Can we model this as union? There are 3 possible types:

('domain', 'bus', 'slot', 'function') - PCI addr
('host', 'bus', 'target', 'lun') - SCSI addr
('bus', 'device') - USB addr


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ad09c310a76dde9267824a337fadf3b1c02bd45
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: hostdev: move pci device code to separate class

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: move pci device code to separate class
..


Patch Set 8: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/57956/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 264: 
Line 265: return hostdev
Line 266: 
Line 267: @classmethod
Line 268: def update_device_info(cls, vm, device_conf, x):
I don't like having the extra `x' argument here, this very confusing wrt. other 
update_device_info methods. update_device_info is probably not supposed to be 
called directly in this class, so you can make the inherited method raise an 
exception and use a different name for this method.
Line 269: alias = 
x.getElementsByTagName('alias')[0].getAttribute('name')
Line 270: address = vmxml.device_address(x)
Line 271: source = x.getElementsByTagName('source')[0]
Line 272: device = pci_address_to_name(**vmxml.device_address(source))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bceb93c2434ff827406bbf4ee0af30f5726f6af
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: hostdev: use specific device classes in HostDevice

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

PS8, Line 304: class HostDevice(core.Base):
Is missing is_assigned_to.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: tests: missing dispatch in jsonrpc integration tests

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: tests: missing dispatch in jsonrpc integration tests
..


Patch Set 4:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdc3c864fa78d91fb66b8d82e230c6f3505d7f8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Francesco Romani 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: schema: restore still used types

[piotr . kliczewski]

Piotr Kliczewski has abandoned this change.

Change subject: schema: restore still used types
..


Abandoned

We do not need this patch any more.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I4a0992756e196d112ad3097c680c34516a1934f5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: stomp: simplified client

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: stomp: simplified client
..


Patch Set 4: Verified+1

Removed comment. No code changes. Verified in previous patch set.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b9b33474e60ab349885a7de18eeacc8f648011f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Francesco Romani 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: hostdev: move pci device code to separate class

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: move pci device code to separate class
..


Patch Set 8: Code-Review+1

I'm not particularly fond of the code duplication, but it's better than the 
original mess. And we've already got duplicate code snippets in device classes, 
so we'll hopefully make something about it for all the devices sooner or later. 
So I support this patch series.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bceb93c2434ff827406bbf4ee0af30f5726f6af
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: utils: build cert paths in single place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: utils: build cert paths in single place
..


Patch Set 7: Verified+1

Verified by updating vdsm and running a vm.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Francesco Romani 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: test: Introduce mock module for testing

[fromani]

Francesco Romani has posted comments on this change.

Change subject: test: Introduce mock module for testing
..


Patch Set 2: Code-Review+1

let's use modern tools

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I72edd0a9388f11d905cfbd7d2fac5a95e1212cea
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: yml: return type fixes for Host.hostdevListByCaps

[fromani]

Francesco Romani has posted comments on this change.

Change subject: yml: return type fixes for Host.hostdevListByCaps
..


Patch Set 2: Code-Review+1

looks OK, but I'd like to have Martin's ack as well

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ad09c310a76dde9267824a337fadf3b1c02bd45
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Freddy Rolland 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: v2v: Log detailed output of virt-v2v

[fromani]

Francesco Romani has posted comments on this change.

Change subject: v2v: Log detailed output of virt-v2v
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/59834/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 383: self._vmid = vmid
Line 384: self._irs = irs
Line 385: self._prepared_volumes = []
Line 386: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % vmid)
Line 387: self._base_command = [_VIRT_V2V.cmd, '-v', '-x']
> There is no reason not to. Any performance penalty caused by traces is negl
Tomas, good point. The import process is sporadic enough that we can afford to 
always have debug information. I think you are right, and the pros outweight 
the cons.
Line 388: 
Line 389: def execute(self):
Line 390: raise NotImplementedError("Subclass must implement this")
Line 391: 


Line 785: yield ImportProgress(int(current_disk), 
int(disk_count),
Line 786:  description)
Line 787: for chunk in self._iter_progress(stream):
Line 788: progress = self._parse_progress(chunk)
Line 789: if progress is not None:
> It's not unrelated. Se below.
ok
Line 790: yield DiskProgress(progress)
Line 791: if progress == 100:
Line 792: break
Line 793: 


Line 811: 
Line 812: def _parse_progress(self, chunk):
Line 813: m = self.DISK_PROGRESS_RE.match(chunk)
Line 814: if m is None:
Line 815: return None
> When verbose output is enabled there are other messages before the proggres
ok, let's add a test about this.
Line 816: try:
Line 817: return int(m.group(1))
Line 818: except ValueError:
Line 819: raise OutputParserError('error parsing progress regex: %r'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
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]: migration: wait properly for migration to begin

[tjelinek]

Tomas Jelinek has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/60073/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 766: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
Line 767: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
Line 768: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
Line 769: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
Line 770: stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
> I think it is better to raise an exception here, or signal somehow the call
well, in order to make the call on 658 work, we need this not to fail in 
from_job_stats().

An alternative approach would be to do something like this on line 658:

if self._vm._dom.jobStats()['type'] == libvirt.VIR_DOMAIN_JOB_NONE:
continue

An other option I see would be to change the ongoing() to something like:

@classmethod
def ongoing(cls, stats):
return stats['type'] != libvirt.VIR_DOMAIN_JOB_NONE

so we could avoid the parsing of all the params if we need only to find out if 
it is active or not.

What you think?
Line 771: # available since libvirt 1.3
Line 772: stats.get('memory_dirty_rate', -1),
Line 773: # available since libvirt 1.3
Line 774: stats.get('memory_iteration', -1),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: hostdev: improve robustness of libvirt SCSI access

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: improve robustness of libvirt SCSI access
..


Patch Set 7: Code-Review+1

AFAICT I can't see any obvious problem.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0e9983fd309869ebd8a4aacd8bfe42158cd26e64
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: migration: wait properly for migration to begin

[fromani]

Francesco Romani has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3: Code-Review-1

(1 comment)

-1 for visibility only, please check the inline comment

https://gerrit.ovirt.org/#/c/60073/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 766: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
Line 767: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
Line 768: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
Line 769: stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
Line 770: stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
I think it is better to raise an exception here, or signal somehow the caller 
that we have incomplete/bogus data, rather than silently use weird values.

But let's start from the beginning. Considering the change at line 658, what 
happens if we drop this and we just access the values? will it raise?
Line 771: # available since libvirt 1.3
Line 772: stats.get('memory_dirty_rate', -1),
Line 773: # available since libvirt 1.3
Line 774: stats.get('memory_iteration', -1),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: hostdev: use specific device classes in HostDevice

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 301: # way of reconstructing the udev name we use as unique id.
Line 302: 
Line 303: 
Line 304: class HostDevice(core.Base):
Line 305: __slots__ = ('_device',)
> Shouldn't this be removed?
There is no reason not to use slots for device classes, but in this case they 
can stay empty.
Line 306: 
Line 307: _DEVICE_MAPPING = {
Line 308: 'pci': PciDevice,
Line 309: 'usb_device': UsbDevice,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: hostdev: use specific device classes in HostDevice

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

PS8, Line 320: @classmethod
 : def update_device_info(cls, vm, device_conf):
 : for device_xml in 
vm.domain.get_device_elements('hostdev'):
 : device_type = device_xml.getAttribute('type')
 : cls._DEVICE_MAPPING[device_type].update_device_info(
 : vm, device_conf, device_xml)
> another alternative:
That is slightly confusing. I don't understand how we'd create the device 
classes without special case in vm.py's code again.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: hostdev: use specific device classes in HostDevice

[fromani]

Francesco Romani has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

PS8, Line 320: @classmethod
 : def update_device_info(cls, vm, device_conf):
 : for device_xml in 
vm.domain.get_device_elements('hostdev'):
 : device_type = device_xml.getAttribute('type')
 : cls._DEVICE_MAPPING[device_type].update_device_info(
 : vm, device_conf, device_xml)
another alternative:

have a simple empty base class:

  class _HostDevice(core.Base):
@classmethod
def update_device_info(cls, vm, device_conf):
for device_xml in vm.domain.get_device_elements('hostdev'):
device_type = device_xml.getAttribute('type')
cls._DEVICE_MAPPING[device_type].update_device_info(
vm, device_conf, device_xml)


have a module function like

  def new(conf, log **kwargs):
  device_params = get_device_params(kwargs['device'])
  device = cls._DEVICE_MAPPING[
  device_params['capability']](conf, log, **kwargs)
  return device


perhaps overall simpler?
again, no real strong feelings against __new__, just tryint different approaches


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: hostdev: use specific device classes in HostDevice

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: use specific device classes in HostDevice
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 301: # way of reconstructing the udev name we use as unique id.
Line 302: 
Line 303: 
Line 304: class HostDevice(core.Base):
Line 305: __slots__ = ('_device',)
Shouldn't this be removed?
Line 306: 
Line 307: _DEVICE_MAPPING = {
Line 308: 'pci': PciDevice,
Line 309: 'usb_device': UsbDevice,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: migration: wait properly for migration to begin

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: migration: wait properly for migration to begin

[tjelinek]

Tomas Jelinek has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 3:

(4 comments)

https://gerrit.ovirt.org/#/c/60073/2//COMMIT_MSG
Commit Message:

PS2, Line 13: even tho
> ... even though the ... ?
Done


PS2, Line 17: ure the
> ... present
Done


PS2, Line 18: 
> ... its
Done


https://gerrit.ovirt.org/#/c/60073/2/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 705: 
Line 706: if self._stop.isSet():
Line 707: break
Line 708: 
Line 709: self.progress = progress
> Is this condition still necessary?
right, not needed.
Line 710: self._vm.log.info('%s', progress)
Line 711: 
Line 712: def stop(self):
Line 713: self._vm.log.debug('stopping migration monitor thread')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
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]: migration: wait properly for migration to begin

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: wait properly for migration to begin
..


Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/60073/2//COMMIT_MSG
Commit Message:

PS2, Line 13: even the
... even though the ... ?


PS2, Line 17: presnet
... present


PS2, Line 18: it's
... its


https://gerrit.ovirt.org/#/c/60073/2/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 705: 
Line 706: if self._stop.isSet():
Line 707: break
Line 708: 
Line 709: if progress.ongoing:
Is this condition still necessary?
Line 710: self.progress = progress
Line 711: self._vm.log.info('%s', progress)
Line 712: 
Line 713: def stop(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-3.6]: migration: fix typo

[fromani]

Francesco Romani has submitted this change and it was merged.

Change subject: migration: fix typo
..


migration: fix typo

A typo sneaked in during the review of commit 1e7d561

Change-Id: I421a7ba0d0ffedc2a90a79aab37712fabbddebca
Label: ovirt-3.6-only
Bug-Url: https://bugzilla.redhat.com/1339521
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/60037
Continuous-Integration: Jenkins CI
Reviewed-by: Milan Zamazal 
---
M vdsm/virt/migration.py
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified; Looks good to me, approved
  Milan Zamazal: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I421a7ba0d0ffedc2a90a79aab37712fabbddebca
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-3.6]: migration: fix typo

[fromani]

Francesco Romani has posted comments on this change.

Change subject: migration: fix typo
..


Patch Set 2:

raising score

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I421a7ba0d0ffedc2a90a79aab37712fabbddebca
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-3.6]: migration: fix typo

[fromani]

Francesco Romani has posted comments on this change.

Change subject: migration: fix typo
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I421a7ba0d0ffedc2a90a79aab37712fabbddebca
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: migration: wait properly for migration to begin

[tjelinek]

Tomas Jelinek has uploaded a new change for review.

Change subject: migration: wait properly for migration to begin
..

migration: wait properly for migration to begin

If the time it takes the migration to begin is longer than the
MIGRATION_MONITOR_INTERVAL (may happen due to networking issues), the first
call to Progress.from_job_stats(self._vm._dom.jobStats()) leads to an exception
because the stats don't contain the statistics yet. This situation causes the
monitor thread to die (even the migration will continue and eventually pass).

Fixed by:
- making sure the from_job_stats() will not fail in case some of the statistics
  are not presnet
- adding a check which makes sure that the monitor thread will be executing it's
  logic only if the migration is ongoing

Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Signed-off-by: Tomas Jelinek 
---
M vdsm/virt/migration.py
1 file changed, 15 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/73/60073/1

diff --git a/vdsm/virt/migration.py b/vdsm/virt/migration.py
index 834460f..55d263b 100644
--- a/vdsm/virt/migration.py
+++ b/vdsm/virt/migration.py
@@ -653,6 +653,11 @@
 break
 
 progress = Progress.from_job_stats(self._vm._dom.jobStats())
+# It may happen that the migration did not start yet
+# so we'll keep waitingy
+if not progress.ongoing:
+continue
+
 now = time.time()
 if not self._use_conv_schedule and\
 (0 < migrationMaxTime < now - self._startTime):
@@ -754,16 +759,16 @@
 def from_job_stats(cls, stats):
 return cls(
 stats['type'],
-stats[libvirt.VIR_DOMAIN_JOB_TIME_ELAPSED],
-stats[libvirt.VIR_DOMAIN_JOB_DATA_TOTAL],
-stats[libvirt.VIR_DOMAIN_JOB_DATA_PROCESSED],
-stats[libvirt.VIR_DOMAIN_JOB_DATA_REMAINING],
-stats[libvirt.VIR_DOMAIN_JOB_MEMORY_TOTAL],
-stats[libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED],
-stats[libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING],
-stats[libvirt.VIR_DOMAIN_JOB_MEMORY_BPS],
-stats[libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT],
-stats[libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES],
+stats.get(libvirt.VIR_DOMAIN_JOB_TIME_ELAPSED, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_DATA_TOTAL, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_DATA_PROCESSED, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_DATA_REMAINING, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_TOTAL, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
+stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
 # available since libvirt 1.3
 stats.get('memory_dirty_rate', -1),
 # available since libvirt 1.3


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: test: Introduce mock module for testing

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: test: Introduce mock module for testing
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I72edd0a9388f11d905cfbd7d2fac5a95e1212cea
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: automation: Increase timeout for make check

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: automation: Increase timeout for make check
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d1ec1f001f0c6336f4bf733815012a1d3e3debc
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
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]: jsonrpcvdscli: add additional verbs

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: jsonrpcvdscli: add additional verbs
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I57f9c63a05d8354676e68418b08593ea830b42a7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Simone Tiraboschi 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Rafael Martins 
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[ovirt-4.0]: API: doc: reformat docstring for consistency

[fromani]

Francesco Romani has submitted this change and it was merged.

Change subject: API: doc: reformat docstring for consistency
..


API: doc: reformat docstring for consistency

Change-Id: I13e3b6e85ab6a935ec15e581fba9fecc02fc588b
Backport-To: 4.0
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1351077
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/59668
Reviewed-by: Dan Kenigsberg 
Continuous-Integration: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/59875
Reviewed-by: Milan Zamazal 
Continuous-Integration: Jenkins CI
---
M vdsm/API.py
1 file changed, 8 insertions(+), 6 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified; Looks good to me, approved
  Milan Zamazal: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I13e3b6e85ab6a935ec15e581fba9fecc02fc588b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
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]: vdsm: adding handling for NGN in osinfo.py

[danken]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vdsm: adding handling for NGN in osinfo.py
..


vdsm: adding handling for NGN in osinfo.py

Current osinfo.py implementation lacks handling for NGN (New Generation
Node).

This patch adds another parameter in addition to the osname, release and
version named pretty_name, that is empty by default and if
/etc/os-release file exists on the OS, will return the content of the
PRETTY_NAME value (if exists) from that file.

Change-Id: Ida119527b263302bf3f78e359bac12113718b744
Bug-Url : https://bugzilla.redhat.com/show_bug.cgi?id=1324447
Signed-off-by: emesika 
Reviewed-on: https://gerrit.ovirt.org/59431
Continuous-Integration: Jenkins CI
Reviewed-by: Piotr Kliczewski 
---
M lib/api/vdsm-api.yml
M lib/vdsm/osinfo.py
2 files changed, 31 insertions(+), 9 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, approved
  Eli Mesika: Verified
  Jenkins CI: Passed CI tests



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ida119527b263302bf3f78e359bac12113718b744
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eli Mesika 
Gerrit-Reviewer: Fabian Deutsch 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
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]: Makefile: added 'tests' target

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: Makefile: added 'tests' target
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I378dbf38bbce6cadf87fbedbc9bf6a5d1c714571
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Irit Goihman 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim 
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]: vdsm.spec: during rpm build only tests target will run

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: vdsm.spec: during rpm build only tests target will run
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I318f00266c3b0bfa198c87da508d1bfe0eb7e359
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman 
Gerrit-Reviewer: Edward Haas 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[ovirt-4.0]: API: allow setLogLevel to tune a specific logger

[fromani]

Francesco Romani has submitted this change and it was merged.

Change subject: API: allow setLogLevel to tune a specific logger
..


API: allow setLogLevel to tune a specific logger

The not so known setLogLevel VDSM verb allows to dynamically tune
the log level of the root logger, until the next restart of VDSM.

This patch extends the API to let the client tune any specific
logger.

Change-Id: I8f40488fac04031552f36b9de026a0062ab81db0
Backport-To: 4.0
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1351077
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/38426
Reviewed-by: Piotr Kliczewski 
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/59874
Reviewed-by: Milan Zamazal 
---
M client/vdsClient.py
M lib/api/vdsm-api.yml
M lib/vdsm/logUtils.py
M lib/vdsm/rpc/bindingxmlrpc.py
M vdsm/API.py
5 files changed, 29 insertions(+), 14 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified; Looks good to me, approved
  Milan Zamazal: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8f40488fac04031552f36b9de026a0062ab81db0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
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[ovirt-4.0]: API: logging: move setLogLevel code into logUtils

[fromani]

Francesco Romani has submitted this change and it was merged.

Change subject: API: logging: move setLogLevel code into logUtils
..


API: logging: move setLogLevel code into logUtils

Move the implementation of API.setLogLevel into the
logUtils module, with minimal changes and renaming,
to make it less coupled.

The rationale for this move is that we should not have
logic in API.py besides the bare minimum.
Furthermore, setLogLevel fits nicely in the logUtils domain.

Change-Id: Iacda9f82c6e0bc22a19403b99acbfffbdb7fd2bf
Backport-To: 4.0
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1351077
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/58990
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/59872
Reviewed-by: Martin Polednik 
Reviewed-by: Milan Zamazal 
---
M lib/vdsm/logUtils.py
M vdsm/API.py
2 files changed, 10 insertions(+), 7 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified; Looks good to me, approved
  Milan Zamazal: Looks good to me, but someone else must approve
  Martin Polednik: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iacda9f82c6e0bc22a19403b99acbfffbdb7fd2bf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-4.0]: API: allow setLogLevel to tune a specific logger

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: API: allow setLogLevel to tune a specific logger
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8f40488fac04031552f36b9de026a0062ab81db0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
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[ovirt-4.0]: API: streamline and make setLogLevel correct

[fromani]

Francesco Romani has submitted this change and it was merged.

Change subject: API: streamline and make setLogLevel correct
..


API: streamline and make setLogLevel correct

According to the schema, setLogLevel should accept
a log level string ('DEBUG', 'INFO',...). But the code
actually blindly casted the given value to int(), and this suggests
the code actually expected an integer value.

To make things a bit user friendlier and comformant to schema,
change the argument to actually be a log level in string format.

Along the way, this patch streamlines the implementation of
setLogLevel and makes it simpler.

Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Backport-To: 4.0
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1351077
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/38425
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Dan Kenigsberg 
Continuous-Integration: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/59873
Continuous-Integration: Jenkins CI
Reviewed-by: Milan Zamazal 
---
M lib/vdsm/logUtils.py
1 file changed, 14 insertions(+), 1 deletion(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified; Looks good to me, approved
  Milan Zamazal: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
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[ovirt-4.0]: API: streamline and make setLogLevel correct

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
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[ovirt-4.0]: API: logging: move setLogLevel code into logUtils

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: API: logging: move setLogLevel code into logUtils
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacda9f82c6e0bc22a19403b99acbfffbdb7fd2bf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-4.0]: API: logging: move setLogLevel code into logUtils

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: API: logging: move setLogLevel code into logUtils
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacda9f82c6e0bc22a19403b99acbfffbdb7fd2bf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-4.0]: API: doc: reformat docstring for consistency

[fromani]

Francesco Romani has posted comments on this change.

Change subject: API: doc: reformat docstring for consistency
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I13e3b6e85ab6a935ec15e581fba9fecc02fc588b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
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[ovirt-4.0]: API: streamline and make setLogLevel correct

[fromani]

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-4.0
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
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]: yml: schema validation tests

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: yml: schema validation tests
..


Patch Set 1: Verified+1

Tests used to verify schema changes

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2420908ebb39136339a5a52daf12f359836c5291
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Piotr Kliczewski 
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]: yml: return type fixes for StoragePool.getInfo

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: yml: return type fixes for StoragePool.getInfo
..


Patch Set 2: Verified+1

Verified by running schema verification tests

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifeadf2323d2a3535a5777d0cc16027cfb9e42f0e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Freddy Rolland 
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
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org