Change in vdsm[master]: virt: common handling of exceptions
Dan Kenigsberg has submitted this change and it was merged. Change subject: virt: common handling of exceptions .. virt: common handling of exceptions Vm methods should not return responses directly. Rather, they should raise proper exception, and a common layer should translate them in responses. In case of succesfull completion, this code automatically produces a generic success response. Should the method require extra fields, it can return a dictionary with the keys to be added to the success response. Using this way the method could also override the default keys (e.g. the message). This is not clean but it could be needed for backward compatibility. Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Bug-Url: https://bugzilla.redhat.com/1316128 Signed-off-by: Francesco Romani Reviewed-on: https://gerrit.ovirt.org/54664 Continuous-Integration: Jenkins CI Reviewed-by: Nir Soffer --- M debian/vdsm-python.install M lib/vdsm/virt/Makefile.am A lib/vdsm/virt/api.py M tests/Makefile.am A tests/virt_api_test.py M vdsm.spec.in 6 files changed, 173 insertions(+), 0 deletions(-) Approvals: Nir Soffer: Looks good to me, approved Jenkins CI: Passed CI tests Francesco Romani: Verified -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 15: * #1316128::Update tracker: OK * Set MODIFIED::bug 1316128#1316128FAILED, illegal change from NEW -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 14: Verified+1 Verified using the tests. -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 14: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 14: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 13:
(3 comments)
https://gerrit.ovirt.org/#/c/54664/13/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 31:
Line 32: def method(func):
Line 33: """
Line 34: Decorate an instance method, and return a response according to the
Line 35: outcome of the call.
> Can you separate paragraphs with empty line?
Done
Line 36: If the method returns None, return a plain success response.
Line 37: If the method wants to augment the success response, it could
return
Line 38: a dict. The dict items will be added to the success response.
Line 39: The method could override the success response message this way.
https://gerrit.ovirt.org/#/c/54664/13/tests/virt_api_test.py
File tests/virt_api_test.py:
Line 46: vmId = 'bar'
Line 47: res = self.vm.succeed_with_args('vmName', vmName, 'vmId', vmId)
Line 48: self.assertEquals(response.is_error(res), False)
Line 49: self.assertEquals(res['args'][0], 'vmName')
Line 50: self.assertEquals(res['args'][1], vmName)
> We can simplify this to:
Done
Line 51:
Line 52: def test_success_with_kwargs(self):
Line 53: port = 0
Line 54: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 52: def test_success_with_kwargs(self):
Line 53: port = 0
Line 54: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 55: self.assertEquals(response.is_error(res), False)
Line 56: self.assertEquals(res['kwargs']['migrationPort'], port)
> We can simplify this to:
Done
Line 57:
Line 58: def test_success_with_wrong_return(self):
Line 59: vmList = ['foobar'] # wrong type as per @api.method contract
Line 60: self.assertRaises(TypeError,
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 13: Code-Review+1
(3 comments)
Nice! some tests can be simplified if you like to submit another version.
https://gerrit.ovirt.org/#/c/54664/13/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 31:
Line 32: def method(func):
Line 33: """
Line 34: Decorate an instance method, and return a response according to the
Line 35: outcome of the call.
Can you separate paragraphs with empty line?
Line 36: If the method returns None, return a plain success response.
Line 37: If the method wants to augment the success response, it could
return
Line 38: a dict. The dict items will be added to the success response.
Line 39: The method could override the success response message this way.
https://gerrit.ovirt.org/#/c/54664/13/tests/virt_api_test.py
File tests/virt_api_test.py:
Line 46: vmId = 'bar'
Line 47: res = self.vm.succeed_with_args('vmName', vmName, 'vmId', vmId)
Line 48: self.assertEquals(response.is_error(res), False)
Line 49: self.assertEquals(res['args'][0], 'vmName')
Line 50: self.assertEquals(res['args'][1], vmName)
We can simplify this to:
args = ("foo", "bar")
res = self.vm.succeed_with_args(*args)
self.assertEqual(res['args'], args)
Line 51:
Line 52: def test_success_with_kwargs(self):
Line 53: port = 0
Line 54: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 52: def test_success_with_kwargs(self):
Line 53: port = 0
Line 54: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 55: self.assertEquals(response.is_error(res), False)
Line 56: self.assertEquals(res['kwargs']['migrationPort'], port)
We can simplify this to:
kwargs = {"foo", "bar"}
res = self.vm.succeed_with_kwargs(**kwargs)
self.assertEqual(res['kwargs'], kwargs)
Line 57:
Line 58: def test_success_with_wrong_return(self):
Line 59: vmList = ['foobar'] # wrong type as per @api.method contract
Line 60: self.assertRaises(TypeError,
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Piotr Kliczewski has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 13: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 13: Verified+1 v13 addresses comments from Nir added in v9 and missed before. Tests still pass, hence V+1 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 13: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 9:
(3 comments)
https://gerrit.ovirt.org/#/c/54664/9/tests/virt_api_test.py
File tests/virt_api_test.py:
Line 43: def test_success_with_args(self):
Line 44: status = { 'vmName': 'foo', 'vmId': 'bar' }
Line 45: self.assertRaises(TypeError,
Line 46: self.vm.succeed_with_args,
Line 47: status)
> This is a strange success
It is. It is the weird mix of test with positional args and wrong type. Let me
clean this mess.
Line 48:
Line 49: def test_success_with_kwargs(self):
Line 50: port = 0
Line 51: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 70: self.assertEquals(res, expected)
Line 71:
Line 72: def test_fail_with_plain_exception(self):
Line 73: res = self.vm.fail(ValueError)
Line 74: self.assertTrue(response.is_error(res))
> It seems that we control both the error handler and the code raising the ex
Good point, your suggestion is an improvement.
I think this is reasonnable and not too fragile, implemented.
Line 75:
Line 76:
Line 77: class FakeVM(object):
Line 78:
Line 89: return ret
Line 90:
Line 91: @api.method
Line 92: def succeed_with_args(self, *args):
Line 93: return args
> We can use single method for these tests:
let me always use a dict
Line 94:
Line 95: @api.method
Line 96: def succeed_with_kwargs(self, **kwargs):
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 12: Code-Review-1 Francesco, can you check my comments on version 9? https://gerrit.ovirt.org/#/c/54664/9/tests/virt_api_test.py -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Piotr Kliczewski has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 12: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 12: Verified+1 verified using the tests -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Dan Kenigsberg has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 12: Code-Review+1 I'm fine with the plan -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 12: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 11: (1 comment) https://gerrit.ovirt.org/#/c/54664/11/tests/virt_api_test.py File tests/virt_api_test.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: from vdsm.virt import api > I think this should come after vdsm imports (more specific), or even last - Right, will fix Line 22: from vdsm import exception Line 23: from vdsm import response Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 11: (1 comment) https://gerrit.ovirt.org/#/c/54664/11/tests/virt_api_test.py File tests/virt_api_test.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: from vdsm.virt import api I think this should come after vdsm imports (more specific), or even last - this is the module under test. Line 22: from vdsm import exception Line 23: from vdsm import response Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 11: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 10: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: Looks nice, see comments. -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 6: Heh, I see you just did that :-) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/54664/7/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 45:
Line 46: _log.debug("FINISH %s response=%s", func.__name__, ret)
Line 47:
Line 48: if isinstance(ret, dict):
Line 49: return response.success(**ret)
maybe add a debug log to document that the return value is discarded?
Line 50: return response.success()
Line 51:
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 6: Lets split this to infrastructure patch (with tests), and patch using this infrastructure, which probably should include entire vm.py. -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 9: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: (1 comment) https://gerrit.ovirt.org/#/c/54664/8/tests/Makefile.am File tests/Makefile.am: Line 51: network_modules = network/*_test.py Line 52: Line 53: test_modules = \ Line 54:alignmentScanTests.py \ Line 55:api_response_test.py \ Why not virt_api_test.py? Line 56:blocksdTests.py \ Line 57:blockVolumeTests.py \ Line 58:bridgeTests.py \ Line 59:cPopenTests.py \ -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 7: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: (3 comments) https://gerrit.ovirt.org/#/c/54664/8/tests/Makefile.am File tests/Makefile.am: Line 51: network_modules = network/*_test.py Line 52: Line 53: test_modules = \ Line 54:alignmentScanTests.py \ Line 55:api_response_test.py \ > Why not virt_api_test.py? No good reason, will rename Line 56:blocksdTests.py \ Line 57:blockVolumeTests.py \ Line 58:bridgeTests.py \ Line 59:cPopenTests.py \ https://gerrit.ovirt.org/#/c/54664/8/tests/api_response_test.py File tests/api_response_test.py: Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase Line 26: Line 27: Line 28: class ResponseTests(TestCaseBase): > Can you name the class TestResponse? Indeed, let me rename it in the next upload Line 29: Line 30: def setUp(self): Line 31: self.vm = FakeVM() Line 32: Line 72: def succeed(self): Line 73: pass Line 74: Line 75: @api.method Line 76: def succeed_with_return(self, ret): > What about testing kwargs? makes sense, will add in my next upload -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 6: Nir, added a trello card about the cookie. In the next upload I'll add test coverage for this change. -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Piotr Kliczewski has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Amit Aviram has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 6: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 7: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/54664/7/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 45:
Line 46: _log.debug("FINISH %s response=%s", func.__name__, ret)
Line 47:
Line 48: if isinstance(ret, dict):
Line 49: return response.success(**ret)
> I just noticed this, we should not do this.
OK, I used to do like this and I changed in v7. Will restore the old behaviour
like you requested.
It is easy to port existing verbs to this behaviour.
Line 50: return response.success()
Line 51:
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 8: v8 _partially_ addresses Nir's comments in v7. I just need some clarifications before to address the missing comments. -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 7: (7 comments) I like the tests, see the comments. https://gerrit.ovirt.org/#/c/54664/7/tests/api_response_test.py File tests/api_response_test.py: Line 36: Line 37: def test_success_extra_params(self): Line 38: vmList = ['foobar'] Line 39: res = self.vm.succeed_with_args(vmList=vmList) Line 40: self.assertFalse(response.is_error(res)) This double negative is confusing - do we have response.is_sucess()? Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 45: res = self.vm.succeed_with_args(message=message) I would use another "verb" (.e.g succeed_with_return) to test return values and their effect, just to make it easier to understand the tests. Line 46: self.assertFalse(response.is_error(res)) Line 47: self.assertEquals(res['status']['message'], message) Line 48: Line 49: def test_fail_with_vdsm_exception(self): Line 62: self.id = vm_id Line 63: Line 64: @api.method Line 65: def fail_with_vdsm_exception(self, exc): Line 66: raise exc() I would call this fail(exception) Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 66: raise exc() Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() This is not needed, we can use previous method (after rename). Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): Lets add another one with parameter for testing return value: def succeed_with_return(self, ret): return ret Then we can test returning dict with one key, multiple keys, dict with status keys, something else, etc. Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass Lets add another one with parameter for testing return value: def succeed_with_return(self, ret): return ret Then we can test returning dict with one key, multiple keys, dict with status keys, something else, etc. Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 73: def succeed(self): Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): I would test: succeed_with_args(self, *args): succeed_with_kwargs(self, **kwargs) succeed_with_args_and_kwargs(self, *args, **kwargs) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 9:
(3 comments)
https://gerrit.ovirt.org/#/c/54664/9/tests/virt_api_test.py
File tests/virt_api_test.py:
Line 43: def test_success_with_args(self):
Line 44: status = { 'vmName': 'foo', 'vmId': 'bar' }
Line 45: self.assertRaises(TypeError,
Line 46: self.vm.succeed_with_args,
Line 47: status)
This is a strange success
Line 48:
Line 49: def test_success_with_kwargs(self):
Line 50: port = 0
Line 51: res = self.vm.succeed_with_kwargs(migrationPort=port)
Line 70: self.assertEquals(res, expected)
Line 71:
Line 72: def test_fail_with_plain_exception(self):
Line 73: res = self.vm.fail(ValueError)
Line 74: self.assertTrue(response.is_error(res))
It seems that we control both the error handler and the code raising the
exception, so maybe we can do:
e = ValueError()
expected = exception.GeneralException(str(e)).response()
To make it less fragile, we can pass an instance of the error, and implement
fail like this:
def fail(self, e):
raise e
Or if you think this is too fragile, at least check the error code of the
response? This seems to be the contract of this wrapper, returning the response
of vdsm exceptions, or general exception with the details of the original error.
Line 75:
Line 76:
Line 77: class FakeVM(object):
Line 78:
Line 89: return ret
Line 90:
Line 91: @api.method
Line 92: def succeed_with_args(self, *args):
Line 93: return args
We can use single method for these tests:
def succeed_with_args_kw(self, *args, **kw):
return {"args": args, "kwargs": kwargs}
Or return always a dict from both:
return {"args": args}
And in the following method:
return {"kwargs": kwargs}
Line 94:
Line 95: @api.method
Line 96: def succeed_with_kwargs(self, **kwargs):
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/54664/7/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 45:
Line 46: _log.debug("FINISH %s response=%s", func.__name__, ret)
Line 47:
Line 48: if isinstance(ret, dict):
Line 49: return response.success(**ret)
> maybe add a debug log to document that the return value is discarded?
I just noticed this, we should not do this.
We should have a clear and strict api:
- if there is not return value, return None (e.g. no return in python)
- If there is a return value it must be a dict
The code should be:
if ret is None:
return response.success()
else:
return response.success(**ret)
Hopefully this works with current vdsm verbs.
Line 50: return response.success()
Line 51:
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 8:
(3 comments)
https://gerrit.ovirt.org/#/c/54664/8/tests/api_response_test.py
File tests/api_response_test.py:
Line 24:
Line 25: from testlib import VdsmTestCase as TestCaseBase
Line 26:
Line 27:
Line 28: class ResponseTests(TestCaseBase):
Can you name the class TestResponse?
This name is automatically detected by py.test, even if you don't subclass from
VdsmTestCase.
We can do these changes later, but when adding new tests I think we should use
this style to save work later.
Line 29:
Line 30: def setUp(self):
Line 31: self.vm = FakeVM()
Line 32:
Line 36:
Line 37: def test_success_with_return_dict(self):
Line 38: vmList = ['foobar']
Line 39: res = self.vm.succeed_with_return({'vmList': vmList})
Line 40: self.assertEquals(response.is_error(res), False)
This is much easier to parse like this.
Line 41: self.assertEquals(res['vmList'], vmList)
Line 42:
Line 43: def test_success_with_wrong_return(self):
Line 44: vmList = ['foobar']
Line 72: def succeed(self):
Line 73: pass
Line 74:
Line 75: @api.method
Line 76: def succeed_with_return(self, ret):
What about testing kwargs?
Not sure that we need them, I think the bridge always call with positional
args, but I did not test this.
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 7: (7 comments) https://gerrit.ovirt.org/#/c/54664/7/tests/api_response_test.py File tests/api_response_test.py: Line 36: Line 37: def test_success_extra_params(self): Line 38: vmList = ['foobar'] Line 39: res = self.vm.succeed_with_args(vmList=vmList) Line 40: self.assertFalse(response.is_error(res)) > This double negative is confusing - do we have response.is_sucess()? we don't yet :\ Let me try to improve a bit without adding (now) a new helper Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 45: res = self.vm.succeed_with_args(message=message) > I would use another "verb" (.e.g succeed_with_return) to test return values Done Line 46: self.assertFalse(response.is_error(res)) Line 47: self.assertEquals(res['status']['message'], message) Line 48: Line 49: def test_fail_with_vdsm_exception(self): Line 62: self.id = vm_id Line 63: Line 64: @api.method Line 65: def fail_with_vdsm_exception(self, exc): Line 66: raise exc() > I would call this fail(exception) Done Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 66: raise exc() Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() > This is not needed, we can use previous method (after rename). Done Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): > Lets add another one with parameter for testing return value: Done Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass > Lets add another one with parameter for testing return value: Done Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 73: def succeed(self): Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): > I would test: OK. what those method should return? -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
Nir Soffer has posted comments on this change.
Change subject: virt: common handling of exceptions
..
Patch Set 5:
(3 comments)
https://gerrit.ovirt.org/#/c/54664/5/lib/vdsm/virt/api.py
File lib/vdsm/virt/api.py:
Line 27:
Line 28:
Line 29: _log = logging.getLogger("virt.api")
Line 30:
Line 31:
> Nice idea, I'm just worried about name clash again:
Sure, we will solve this when needed. Please don't waste time on this now.
Line 32: def method(func):
Line 33: @wraps(func)
Line 34: def wrapper(*args, **kwargs):
Line 35:
Line 30:
Line 31:
Line 32: def method(func):
Line 33: @wraps(func)
Line 34: def wrapper(*args, **kwargs):
> I will check that, I'm just not sure it will work transparently with plain
It will not work with plain functions, but we don't care.
Line 35:
Line 36: # TODO: maybe add a (cheap) cookie so we can unambiguosly
correlate
Line 37: # this START and FINISH log later, so it is easy to compute
the API
Line 38: # RTT and other metrics
Line 34: def wrapper(*args, **kwargs):
Line 35:
Line 36: # TODO: maybe add a (cheap) cookie so we can unambiguosly
correlate
Line 37: # this START and FINISH log later, so it is easy to compute
the API
Line 38: # RTT and other metrics
> OK, so will remove this TODO and go ahead.
But I would consider adding a cookie, can be much easier for manually grepping
logs and avoid possible errors when processing logs.
Lets convert the todo to trello card?
Line 39: _log.debug("START %s args=%s kwargs=%s", func.__name__, args,
kwargs)
Line 40: try:
Line 41: # TODO: 'self' argument?
Line 42: ret = func(*args, **kwargs)
--
To view, visit https://gerrit.ovirt.org/54664
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Amit Aviram
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: common handling of exceptions
gerrit-hooks has posted comments on this change. Change subject: virt: common handling of exceptions .. Patch Set 6: * #1316128::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1316128::OK, public bug * Check Product::#1316128::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Amit Aviram Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
