Change in vdsm[master]: virt: add device setup and teardown

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: virt: add device setup and teardown
..


virt: add device setup and teardown

More and more devices require some action to be taken before VM is
started - even before the XML is generated. We add two methods, setup
and teardown, that are called before VM's start and after VM's
destruction respectively.

These methods should be used as a replacement for device-specific
methods.

Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Signed-off-by: Martin Polednik 
Reviewed-on: https://gerrit.ovirt.org/55135
Reviewed-by: Francesco Romani 
Continuous-Integration: Jenkins CI
---
M tests/vmTests.py
M tests/vmfakelib.py
M vdsm/virt/vm.py
M vdsm/virt/vmdevices/core.py
4 files changed, 200 insertions(+), 0 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, approved
  Martin Polednik: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 18:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 17: Verified+1

Tested with currently broken hostdev -> works fine, nothing else uses it yet. 
The tests pass, VMs run.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 17: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 17:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 16:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 15:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/15/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

Line 77: def setup(self):
Line 78: """
Line 79: Actions to be executed before VM is started. This method is 
therefore
Line 80: able to modify the final device XML. Not executed in the 
recovery
Line 81: flow.
Add a big warning here about leaving the device in consistent state in case of 
failures, and mention that teardown will *not* be called in if setup failed.
Line 82: """
Line 83: pass
Line 84: 
Line 85: def teardown(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> OK, Martin's proposal works for me.
OK, so we need to document that setup must not leave the device in inconsistent 
state on any failure.

And make sure that all devices that use setup do catch *all* error and perform 
proper cleanup before raising the error.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 15:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> Since it doesn't really affect any device at the moment, I believe we shoul
OK, Martin's proposal works for me.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> The console device works using (too) 'clever' tricks.
Since it doesn't really affect any device at the moment, I believe we should 
just make a choice (and I'm for not tearing down device, just for the sake of 
less changes between patchsets).

If we find the need to call teardown after setup, it can be changed without 
breaking anything.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> Console uses both setup and teardown (called prepare, cleanup) so I guess t
The console device works using (too) 'clever' tricks.
The teardown method (cleanup) leverages rmFile, which silently swallows 
exceptions, so it works even if the socket file was not created.
Furthermore, if the setup() fails to create a socket, there is nothing to 
cleanup on teardown.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> For storage we have prepareVolumePath and teardownVolumePath, but prepareVo
Console uses both setup and teardown (called prepare, cleanup) so I guess that 
is the deciding device for now. Glancing over the code, we don't very well 
handle tearing down the sockets when creation flow fails, may even improve the 
code slightly to call teardown after failed setup.

I'm fine with either, we just need to decide which is more future proof.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(2 comments)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> We have reverted tearing down host devices at all.
For storage we have prepareVolumePath and teardownVolumePath, but 
prepareVolumePath is run *before* creating the drive object. I think it will be 
hard to use setup and teardown for drives without major rewrite.

Error handling is also not consistent, sometimes teardownVolumePath is called, 
sometimes it is not.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


https://gerrit.ovirt.org/#/c/55135/14/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1867: """
Line 1868: done = []
Line 1869: 
Line 1870: for dev_objects in self._devices.values():
Line 1871: for dev_object in dev_objects[:]:
> I don't believe this adds anything but confusion unless we move the rest of
So lets wait with this - we should use the same code to iterate over devices 
everywhere.
Line 1872: try:
Line 1873: dev_object.setup()
Line 1874: except Exception:
Line 1875: self.log.exception("Failed to setup device %s",


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(3 comments)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> In abstract, I prefer to do teardown if setup fails. I think it is better t
We have reverted tearing down host devices at all.

I suppose storage/console devices could give us a bigger hint in this regard.
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


https://gerrit.ovirt.org/#/c/55135/14/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1719: for dev_object in dev_objects[:]:
Line 1720: try:
Line 1721: dev_object.teardown()
Line 1722: except Exception:
Line 1723: self.log.exception("Failed to teardown device")
> This repeats the code in _teardown_setup_devices, with different log - aren
These functions should be merged.
Line 1724: 
Line 1725: def _cleanupRecoveryFile(self):
Line 1726: self._recovery_file.cleanup()
Line 1727: 


Line 1867: """
Line 1868: done = []
Line 1869: 
Line 1870: for dev_objects in self._devices.values():
Line 1871: for dev_object in dev_objects[:]:
> This code is repeated in both setup_devices and here.
I don't believe this adds anything but confusion unless we move the rest of 
code to _iter_devices.
Line 1872: try:
Line 1873: dev_object.setup()
Line 1874: except Exception:
Line 1875: self.log.exception("Failed to setup device %s",


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(2 comments)

partial review

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
> Now we have the decide - if setup fails, do we teardown the device?
In abstract, I prefer to do teardown if setup fails. I think it is better to 
clean up after ourselves and avoid partially done stuff.
However, I'm thinking about actual real usecases trying to figure out what's 
best in practice. The most complex devices we handle are host devices, so let's 
take in account first what's best for them.

Martin, I believe that for host devices it is indeed better to do teardown if 
setup fails, right? Am I remembering right?
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


Line 1167: self.assertEqual(devices[1].state, fake.TEARDOWN)
Line 1168: self.assertEqual(devices[2].state, fake.TEARDOWN)
Line 1169: 
Line 1170: def test_device_teardown_fail_all(self):
Line 1171: devices = [fake.Device('device_{}'.format(i), 
fail_teardown=True)
> fail_teardown=UnexpectedError
Yes please do that
Line 1172:for i in range(3)]
Line 1173: 
Line 1174: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1175: testvm._devices[hwclass.GENERAL] = devices


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

(5 comments)

https://gerrit.ovirt.org/#/c/55135/14/tests/vmTests.py
File tests/vmTests.py:

Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(ExpectedError, testvm._setup_devices)
Line 1118: self.assertEqual(devices[0].state, fake.SETUP)
Now we have the decide - if setup fails, do we teardown the device?

If we don't, setup must leave the device in consistent state, even if something 
raises during setup. This is the behavior in unittest setUp(), and it makes it 
quite hard to use setup and teardown for resource management.

If we do, setup can fail, leaving the device in inconsistent state, and 
teardown must know how to handle a partly setup device.

The change in the code is simple, add the device to the setup devices list 
before calling setup instead of after, or add it in the exception handler.

Francesco, thoughts?
Line 1119: self.assertEqual(devices[1].state, fake.CREATED)
Line 1120: self.assertEqual(devices[2].state, fake.CREATED)
Line 1121: 
Line 1122: def test_device_setup_fail_second(self):


Line 1167: self.assertEqual(devices[1].state, fake.TEARDOWN)
Line 1168: self.assertEqual(devices[2].state, fake.TEARDOWN)
Line 1169: 
Line 1170: def test_device_teardown_fail_all(self):
Line 1171: devices = [fake.Device('device_{}'.format(i), 
fail_teardown=True)
fail_teardown=UnexpectedError
Line 1172:for i in range(3)]
Line 1173: 
Line 1174: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1175: testvm._devices[hwclass.GENERAL] = devices


https://gerrit.ovirt.org/#/c/55135/14/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1719: for dev_object in dev_objects[:]:
Line 1720: try:
Line 1721: dev_object.teardown()
Line 1722: except Exception:
Line 1723: self.log.exception("Failed to teardown device")
This repeats the code in _teardown_setup_devices, with different log - aren't 
these devices left in inconsistent state here as well?
Line 1724: 
Line 1725: def _cleanupRecoveryFile(self):
Line 1726: self._recovery_file.cleanup()
Line 1727: 


Line 1867: """
Line 1868: done = []
Line 1869: 
Line 1870: for dev_objects in self._devices.values():
Line 1871: for dev_object in dev_objects[:]:
This code is repeated in both setup_devices and here.

We can make it nicer if we add a method returning an iterator over all devices:

def _iter_devices(self):
for devices in self._devices.values():
for device in devices[:]:
yield device

Now we can do:

done = []
for device in self._iter_devices():
try:
device.setup()
except Exception:
log ...
self._teardown_devices(done)
raise
done.add(device)

_teardown_devices can use optional parameter to teardown only certain devices, 
and if none, teardown all.
Line 1872: try:
Line 1873: dev_object.setup()
Line 1874: except Exception:
Line 1875: self.log.exception("Failed to setup device %s",


Line 1884: try:
Line 1885: dev_object.teardown()
Line 1886: except Exception:
Line 1887: self.log.exception('Failed to tear down device %s, 
device in '
Line 1888:'inconsistent state', 
dev_object.device)
This repeats the code in _teardown_devices.
Line 1889: 
Line 1890: def _run(self):
Line 1891: self.log.info("VM wrapper has started")
Line 1892: dev_spec_map = self._devSpecMapFromConf()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 14:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/12/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 419: def __init__(self, device, fail_setup=None, fail_teardown=None):
Line 420: self.fail_setup = fail_setup
Line 421: self.fail_teardown = fail_teardown
Line 422: self.device = device
Line 423: self.is_setup = False
> True,  but since we don't use it now, I think it is better to remove it. Th
Sounds reasonable. My concern is that it is implied that teardown only follows 
setup, but I can't think of any name that would look nicer atm. I guess pros 
outweigh the cons.
Line 424: self.was_setup = False
Line 425: self.was_teardown = False
Line 426: 
Line 427: @recorded


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/12/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 419: def __init__(self, device, fail_setup=None, fail_teardown=None):
Line 420: self.fail_setup = fail_setup
Line 421: self.fail_teardown = fail_teardown
Line 422: self.device = device
Line 423: self.is_setup = False
> I was thinking about it. For current tests, it's afaik not needed. Future w
True,  but since we don't use it now, I think it is better to remove it. The 
purpose of this class is to test setup or teardown flows, which never include 
setting up a device twice - this should raise.

We can use more strict way to record the state.

CREATED = "created"
SETUP = "setup"
TEARDOWN = "teardown"

class Device():
def __init__(self):
self.state = CREATED
def setup(self):
assert self.state is CREATED
self.state = SETUP
def teardown(self):
assert self.state is SETUP
self.state = TEARDOWN

Using this we can delete about half of the assertions in the tests. Instead of 
testing:

assert device.was_setup
assert device.was_teardown

We can check:

assert device.state is TEARDOWN

The only way the test will succeed is if the setup and teardown were called 
exactly once in the correct order.

Also the test failure would be much nicer:

"setup" != "teardown"

Instead of:

True is not False

What do you think?
Line 424: self.was_setup = False
Line 425: self.was_teardown = False
Line 426: 
Line 427: @recorded


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 13:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12: Verified+1

Tested whole hostdev + hotplug/hotunplug flow VM flow.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/12/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 419: def __init__(self, device, fail_setup=None, fail_teardown=None):
Line 420: self.fail_setup = fail_setup
Line 421: self.fail_teardown = fail_teardown
Line 422: self.device = device
Line 423: self.is_setup = False
> Do we need this?
I was thinking about it. For current tests, it's afaik not needed. Future wise, 
there is still some value - imagine following scenario:


setup (is_setup = True, was_setup = True, was_teardown = False)
teardown (is_setup = False, was_setup = True, was_teardown = True)
setup (is_setup = True, was_setup = True, was_teardown = True)


Therefore, keeping is_setup leads to slightly more generic code.
Line 424: self.was_setup = False
Line 425: self.was_teardown = False
Line 426: 
Line 427: @recorded


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/12/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 419: def __init__(self, device, fail_setup=None, fail_teardown=None):
Line 420: self.fail_setup = fail_setup
Line 421: self.fail_teardown = fail_teardown
Line 422: self.device = device
Line 423: self.is_setup = False
Do we need this?

Isn't this equal to self.was_setup and not self.was_teardown?
Line 424: self.was_setup = False
Line 425: self.was_teardown = False
Line 426: 
Line 427: @recorded


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

(1 comment)

Will review later

https://gerrit.ovirt.org/#/c/55135/12/tests/vmTests.py
File tests/vmTests.py:

Line 1108: self.assertFalse(devices[0].was_teardown)
Line 1109: self.assertTrue(devices[1].was_setup)
Line 1110: self.assertFalse(devices[1].was_teardown)
Line : self.assertTrue(devices[2].was_setup)
Line 1112: self.assertFalse(devices[2].was_teardown)
Thanks, it is much easier to understand the tests now!
Line 1113: 
Line 1114: def test_device_setup_fail_first(self):
Line 1115: devices = ([fake.Device('device_0', 
fail_setup=ExpectedError)] +
Line 1116:[fake.Device('device_{}'.format(i)) for i in 
range(1, 3)])


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12: Code-Review+1

partial review

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 12:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 11:

(7 comments)

Partial review

https://gerrit.ovirt.org/#/c/55135/11/tests/vmTests.py
File tests/vmTests.py:

Line 1098: 
Line 1099: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1100: testvm._devices[hwclass.GENERAL] = devices
Line 1101: self.assertNotRaises(testvm._setup_devices)
Line 1102: self.assertDeviceCalls(devices, expected_calls)
It is hard to follow these tests, you have to look at the devices list, and 
then match the calls list.

Keeping the setup/teardown state in the device would make it easier:

devices = [fake.Device('device_{}'.format(i)) for i in range(3)]
with fake.VM(self.conf, create_device_objects=True) as testvm:
testvm._devices[hwclass.GENERAL] = devices
self.assertNotRaises(testvm._setup_devices)
self.assertTrue(all(device.was_setup for device in devices))

And if the second device failed:

self.assertRaises(ExpectedFailure, testvm._setup_devices)

self.assertTrue(devices[0].was_setup)
self.assertTrue(devices[0].was_teardown)

self.assertTrue(devices[1].was_setup)
self.assertFalse(devices[1].was_teardown)

self.assertFalse(devices[2].was_setup)
self.assertFalse(devices[2].was_teardown)

This would be even nicer if we have device objects to test instead of list of 
devices:

device1 = fake.Device("device1")
device2 = fake.Device("device2")
device3 = fake.Device("device3")

with fake.VM(self.conf, create_device_objects=True) as testvm:
testvm._devices[hwclass.GENERAL] = [device1, device2, device3]

self.assertRaises(ExpectedFailure, testvm._setup_devices)

self.assertTrue(device1.was_setup)
self.assertTrue(device1.was_teardown)

self.assertTrue(device2.was_setup)
self.assertFalse(device2.was_teardown)

self.assertFalse(device3.was_setup)
self.assertFalse(device3.was_teardown)
Line 1103: 
Line 1104: def test_device_setup_fail_first(self):
Line 1105: devices = ([fake.Device('device_0', fail_setup=KeyError)] +
Line 1106:[fake.Device('device_{}'.format(i)) for i in 
range(1, 3)])


Line 1142: devices = [fake.Device('device_0', 
fail_teardown=AttributeError),
Line 1143:fake.Device('device_1',
Line 1144:fail_setup=ValueError,  # <-- correct 
one
Line 1145:fail_teardown=NameError),
Line 1146:fake.Device('device_2', fail_setup=KeyError)]
Using standard errors for this is not a good idea (unless you are writing the 
standard library), the test may succeed because the underlying code has a bug 
that raise one of the standard errors.

I would define instead tow errors in this module:

class ExpectedError(Exception): pass
class UnexpectedError(Exception): pass

Using these errors it is very clear what is the expected error and what are the 
unexpected errors, and we don't need to add comments.

This related to the whole class.
Line 1147: expected_calls = [[('setup', (), {}), ('teardown', (), {})],
Line 1148:   [('setup', (), {})],
Line 1149:   []]
Line 1150: 


https://gerrit.ovirt.org/#/c/55135/11/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 414: 
Line 415: 
Line 416: class Device(object):
Line 417: def __init__(self, name, fail_setup=None, fail_teardown=None):
Line 418: self.log = logging.getLogger('fake.Device')
Don't create logger per instance, these logger will never be removed. Logger 
should be created only per class or module.
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown
Line 421: self.device = name
Line 422: self.is_setup = False


Line 417: def __init__(self, name, fail_setup=None, fail_teardown=None):
Line 418: self.log = logging.getLogger('fake.Device')
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown
Line 421: self.device = name
Why not self.name? If we have a device attribute, then cal the argument 
"device" instead of "name".
Line 422: self.is_setup = False
Line 423: self.__calls__ = []
Line 424: 
Line 425: @recorded


Line 418: self.log = logging.getLogger('fake.Device')
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown
Line 421: self.device = name
Line 422: self.is_setup = False
I think we should remove the recording, and use two flags to understand if the 
setup and teardown methods were called:

self.was_setup = False
self.was_teardown = False
Line 423: self.__calls__ = []
Line 424: 
Line 425: 

Change in vdsm[master]: virt: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 11:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 10:

(10 comments)

https://gerrit.ovirt.org/#/c/55135/7/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 412: self.timestamp = timestamp
Line 413: self.cpuCores = CpuCoreSample(samples)
Line 414: 
Line 415: 
Line 416: class Device(object):
> I would use a different approach:
Done incl. assertions.
Line 417: def __init__(self, name, fail_setup=None, fail_teardown=None):
Line 418: self.log = logging.getLogger('fake.Device')
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown


Line 413: self.cpuCores = CpuCoreSample(samples)
Line 414: 
Line 415: 
Line 416: class Device(object):
Line 417: def __init__(self, name, fail_setup=None, fail_teardown=None):
> Instead of accepting booleans for fail_setup and fail_teardown, you can pas
Done.
Line 418: self.log = logging.getLogger('fake.Device')
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown
Line 421: self.device = name


Line 417: def __init__(self, name, fail_setup=None, fail_teardown=None):
Line 418: self.log = logging.getLogger('fake.Device')
Line 419: self.fail_setup = fail_setup
Line 420: self.fail_teardown = fail_teardown
Line 421: self.device = name
> Not needed, the recorded decorator add the __calls__ on the first access.
Some of the devices are not accessed.
Line 422: self.is_setup = False
Line 423: self.__calls__ = []
Line 424: 
Line 425: @recorded


Line 422: self.is_setup = False
Line 423: self.__calls__ = []
Line 424: 
Line 425: @recorded
Line 426: def setup(self):
> I would log here:
(+ below) done
Line 427: assert not self.is_setup, 'setup called multiple times'
Line 428: 
Line 429: if self.fail_setup:
Line 430: raise self.fail_setup


https://gerrit.ovirt.org/#/c/55135/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1692: self._cleanupDrives()
Line 1693: self._cleanupFloppy()
Line 1694: self._cleanupGuestAgent()
Line 1695: self._teardown_devices()
Line 1696: cleanup_guest_socket(self._qemuguestSocketFile)
> Log is not a sentence and it does not need trailing period - look around in
(note to self: not done)
Line 1697: # TODO: avoid reattach when Engine can tell free VFs 
otherwise
Line 1698: self._reattachHostDevices()
Line 1699: self._cleanupStatsCache()
Line 1700: numa.invalidateNumaCache(self)


Line 1847: self._updateVcpuLimit()
Line 1848: 
Line 1849: def _setup_devices(self):
Line 1850: """
Line 1851: Runs before the underlying libvirt domain is created.
> Not sure that this raises the correct exception or the last one raised, ple
Verified now.
Line 1852: 
Line 1853: Handle setup of all devices. If some device cannot be setup,
Line 1854: go through the devices that were successfully setup and tear
Line 1855: them down, logging all exceptions we encounter. Exception is 
then


Line 1851: Runs before the underlying libvirt domain is created.
Line 1852: 
Line 1853: Handle setup of all devices. If some device cannot be setup,
Line 1854: go through the devices that were successfully setup and tear
Line 1855: them down, logging all exceptions we encounter. Exception is 
then
> _teardown_setup_devices?
Done
Line 1856: raised as we cannot continue the VM creation due to device 
failures.
Line 1857: """
Line 1858: done = []
Line 1859: 


Line 1854: go through the devices that were successfully setup and tear
Line 1855: them down, logging all exceptions we encounter. Exception is 
then
Line 1856: raised as we cannot continue the VM creation due to device 
failures.
Line 1857: """
Line 1858: done = []
> Same issue with the names.
Same answer as above.
Line 1859: 
Line 1860: for dev_objects in self._devices.values():
Line 1861: for dev_object in dev_objects[:]:
Line 1862: try:


Line 1856: raised as we cannot continue the VM creation due to device 
failures.
Line 1857: """
Line 1858: done = []
Line 1859: 
Line 1860: for dev_objects in self._devices.values():
> Better:
Done
Line 1861: for dev_object in dev_objects[:]:
Line 1862: try:
Line 1863: dev_object.setup()
Line 1864: except Exception:


Line 1858: done = []
Line 1859: 
Line 1860: for dev_objects in self._devices.values():
Line 1861: for dev_object in dev_objects[:]:
Line 1862: try:
> I do, please remove useless code.
Done
Line 1863: 

Change in vdsm[master]: virt: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 10: Code-Review-1

let's make sure Nir's comments are not lost

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 10:

Martin, please check my comments from version 7 and answer all to them.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 10:

(5 comments)

partial review

https://gerrit.ovirt.org/#/c/55135/10/tests/vmfakelib.py
File tests/vmfakelib.py:

PS10, Line 423: self.__calls__ = []
Isn't this automatically added by @recorded?


PS10, Line 427: ssert not self.is_setup, 'setup called multiple times'
this is nice, but shouldn't be tested explicitely?


PS10, Line 429: if self.fail_setup
nit: if self.fail_setup is not None: ...


PS10, Line 437: assert self.is_setup, 'teardown called before setup or multiple 
times'
same as in setup()


PS10, Line 439:  if self.fail_teardown:
same as per fail_setup


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 10:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 9:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 8: Verified+1

works w/ supplied tests, fixes hostdev, doesn't break general usage

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1688: """
Line 1689: Runs after the underlying libvirt domain was destroyed.
Line 1690: """
Line 1691: for dev_objects in self._devices.values():
Line 1692: for dev_object in dev_objects[:]:
> This is already (luckily) used throughout devices. I don't see conf['device
This will be removed sooner or later, we cannot continue to work with the 
current state of the code.
Line 1693: try:
Line 1694: dev_object.teardown()
Line 1695: except Exception:
Line 1696: self.log.exception("Failed to teardown device.")


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 8:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/7/tests/vmTests.py
File tests/vmTests.py:

Line 1136: 
Line 1137: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1138: testvm._devices[hwclass.GENERAL] = devices
Line 1139: self.assertRaises(Exception, testvm._setup_devices)
Line 1140: self.assertDeviceCalls(devices, expected_calls)
> That is how it works (except for "original error" as it's the first error?)
Yes the first error during setup should be raised as it is usually the most 
interesting.
Line 1141: 
Line 1142: def test_device_teardown_success(self):
Line 1143: devices = [fake.Device() for _ in range(3)]
Line 1144: expected_calls = [[('teardown', (), {})],


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/55135/7/tests/vmTests.py
File tests/vmTests.py:

Line 1136: 
Line 1137: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1138: testvm._devices[hwclass.GENERAL] = devices
Line 1139: self.assertRaises(Exception, testvm._setup_devices)
Line 1140: self.assertDeviceCalls(devices, expected_calls)
> What about failed teardown while tearing down device after later device set
That is how it works (except for "original error" as it's the first error?).
Line 1141: 
Line 1142: def test_device_teardown_success(self):
Line 1143: devices = [fake.Device() for _ in range(3)]
Line 1144: expected_calls = [[('teardown', (), {})],


https://gerrit.ovirt.org/#/c/55135/7/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 416: class Device(object):
Line 417: def __init__(self, fail_setup=False, fail_teardown=False):
Line 418: self.fail_setup = fail_setup
Line 419: self.fail_teardown = fail_teardown
Line 420: self.device = 'test_device'
> Better accept this parameter so you can have better logging in the tests (w
They don't.
Line 421: self.__calls__ = []
Line 422: 
Line 423: @recorded
Line 424: def setup(self):


https://gerrit.ovirt.org/#/c/55135/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1688: """
Line 1689: Runs after the underlying libvirt domain was destroyed.
Line 1690: """
Line 1691: for dev_objects in self._devices.values():
Line 1692: for dev_object in dev_objects[:]:
> I don't like the term dev_objects and dev_object and don't want the vm modu
This is already (luckily) used throughout devices. I don't see conf['devices'] 
being removed anytime soon.
Line 1693: try:
Line 1694: dev_object.teardown()
Line 1695: except Exception:
Line 1696: self.log.exception("Failed to teardown device.")


Line 1845: try:
Line 1846: dev_object.setup()
Line 1847: except Exception:
Line 1848: self.log.exception("Failed to setup device %s.",
Line 1849:dev_object.device)
> Do we have __repr__ for devices? if we do, better use:
We don't afaik.
Line 1850: self._revert_setup_devices(done)
Line 1851: raise
Line 1852: else:
Line 1853: done.append(dev_object)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7: Code-Review-1

(18 comments)

Looks good and nicely tested, see the comments

https://gerrit.ovirt.org/#/c/55135/7/tests/vmTests.py
File tests/vmTests.py:

Line 1087: 'maxVCpus': '160',
Line 1088: 'memSize': '1024',
Line 1089: 'memGuaranteedSize': '512',
Line 1090: 'devices': [],
Line 1091: }
> if you resubmit, please reformat as
And sort by key
Line 1092: 
Line 1093: def assertDeviceCalls(self, devices, expected_calls):
Line 1094: for index, device in enumerate(devices):
Line 1095: self.assertEquals(device.__calls__, 
expected_calls[index])


Line 1091: }
Line 1092: 
Line 1093: def assertDeviceCalls(self, devices, expected_calls):
Line 1094: for index, device in enumerate(devices):
Line 1095: self.assertEquals(device.__calls__, 
expected_calls[index])
> if you resubmit, I'd move this at the end of the class, below the actual te
We make this little nicer:

for device, calls in zip(devices, expected_calls):
self.assertEquals(device.__calls__, calls)
Line 1096: 
Line 1097: def test_device_setup_success(self):
Line 1098: devices = [fake.Device() for _ in range(3)]
Line 1099: expected_calls = [[('setup', (), {})],


Line 1113:   []]
Line 1114: 
Line 1115: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1116: testvm._devices[hwclass.GENERAL] = devices
Line 1117: self.assertRaises(Exception, testvm._setup_devices)
Exception is not specific enough. You should test the actual exception raised 
from the first failing setup, and make sure we raise the original exception and 
log the rest of the exceptions on the way out.
Line 1118: self.assertDeviceCalls(devices, expected_calls)
Line 1119: 
Line 1120: def test_device_setup_fail_second(self):
Line 1121: devices = [fake.Device(), fake.Device(fail_setup=True), 
fake.Device()]


Line 1136: 
Line 1137: with fake.VM(self.conf, create_device_objects=True) as 
testvm:
Line 1138: testvm._devices[hwclass.GENERAL] = devices
Line 1139: self.assertRaises(Exception, testvm._setup_devices)
Line 1140: self.assertDeviceCalls(devices, expected_calls)
What about failed teardown while tearing down device after later device setup 
failed?

I would expect to see the teardown failure logged and the rest of the devices 
teardown normally, raising the original error.
Line 1141: 
Line 1142: def test_device_teardown_success(self):
Line 1143: devices = [fake.Device() for _ in range(3)]
Line 1144: expected_calls = [[('teardown', (), {})],


https://gerrit.ovirt.org/#/c/55135/7/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 412: self.timestamp = timestamp
Line 413: self.cpuCores = CpuCoreSample(samples)
Line 414: 
Line 415: 
Line 416: class Device(object):
I would use a different approach:

class Device(object):

def __init__(self, name, fail_setup=None, fail_teardown=None):
self.name = name
self.is_setup = False
...

def setup(self):
fail if needed...
assert not self.is_setup, "setup called twice"
self.is_setup = True

def teardown(self):
fail if needed...
assert self.is_setup, "teardown called before setup or twice"
self.is_setup = False

Now the tests would be much nicer:

dev1 = fake.Device("dev1")
...
self.assertTrue(dev1.is_setup)

And the device will assert for us if the code is using it incorrectly.
Line 417: def __init__(self, fail_setup=False, fail_teardown=False):
Line 418: self.fail_setup = fail_setup
Line 419: self.fail_teardown = fail_teardown
Line 420: self.device = 'test_device'


Line 413: self.cpuCores = CpuCoreSample(samples)
Line 414: 
Line 415: 
Line 416: class Device(object):
Line 417: def __init__(self, fail_setup=False, fail_teardown=False):
Instead of accepting booleans for fail_setup and fail_teardown, you can pass an 
exception instance that should be raised, making it easier to assert about 
errors.
Line 418: self.fail_setup = fail_setup
Line 419: self.fail_teardown = fail_teardown
Line 420: self.device = 'test_device'
Line 421: self.__calls__ = []


Line 416: class Device(object):
Line 417: def __init__(self, fail_setup=False, fail_teardown=False):
Line 418: self.fail_setup = fail_setup
Line 419: self.fail_teardown = fail_teardown
Line 420: self.device = 'test_device'
Better accept this parameter so you can have better logging in the tests (which 
device failed). And why not use "name" - devices have names, 

Change in vdsm[master]: virt: add device setup and teardown

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7: Code-Review+1

Looks basically OK to me.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7: Code-Review+1

(3 comments)

I've minor nits, but not worthy a resubmit. Full ACK on hold to avoid hiding 
other reviews.

https://gerrit.ovirt.org/#/c/55135/7/tests/vmTests.py
File tests/vmTests.py:

PS7, Line 1084: conf = {'vmName': 'testVm',
  : 'vmId': '9ffe28b6-6134-4b1e-beef-1185f49c436f',
  : 'smp': '8',
  : 'maxVCpus': '160',
  : 'memSize': '1024',
  : 'memGuaranteedSize': '512',
  : 'devices': [],
  : }
if you resubmit, please reformat as

  conf = {
'vmName': 'testVm',
...


PS7, Line 1093: def assertDeviceCalls(self, devices, expected_calls):
  : for index, device in enumerate(devices):
  : self.assertEquals(device.__calls__, 
expected_calls[index])
if you resubmit, I'd move this at the end of the class, below the actual tests.


https://gerrit.ovirt.org/#/c/55135/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS7, Line 1862: continue
useless, but I don't mind


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 7:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
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: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/6/tests/vmTests.py
File tests/vmTests.py:

PS6, Line 1119: [('setup', (), {}), ('teardown', (), {})
> I don't really understand. The devices are linear, therefore the one that f
You are right, it is just me being (too) paranoid.
I don't really like this constraint being implicit, 
but I think we can move on and drop this, because I don't see any actual flaw 
here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/6/tests/vmTests.py
File tests/vmTests.py:

PS6, Line 1119: [('setup', (), {}), ('teardown', (), {})
> it would be nicer if we could check that the device tore down was the same 
I don't really understand. The devices are linear, therefore the one that 
failed setup will only have called setup, and all previous will have called 
setup


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 6: Code-Review+1

(3 comments)

partial review looks good so far.
Few questions/suggestions about the tests while I review the rest

https://gerrit.ovirt.org/#/c/55135/6/tests/vmTests.py
File tests/vmTests.py:

PS6, Line 1084: conf = {'vmName': 'testVm', 'vmId': 
'9ffe28b6-6134-4b1e-beef-1185f49c436f',
  : 'smp': '8', 'maxVCpus': '160', 'memSize': '1024',
  : 'memGuaranteedSize': '512', 'devices': []}
nit: in _new_ code I'd really prefer to see

  conf = {
'vmName': 'testVm',
...
'devices': [],
  }


PS6, Line 1100: with self.assertNotRaises():
  : testvm._setup_devices()
this works, but could be better:

  self.assertNotRaises(testvm._setup_devices)

(here and below in a few other places)


PS6, Line 1119: [('setup', (), {}), ('teardown', (), {})
it would be nicer if we could check that the device tore down was the same one 
that failed setup.
Maybe add some identification to the device objects? :\
(I'm fine improving in a later patch)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/55135/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1691: Runs after the underlying libvirt domain was destroyed.
Line 1692: """
Line 1693: for dev_objects in self._devices.values():
Line 1694: for dev_object in dev_objects[:]:
Line 1695: dev_object.teardown()
> These naming style is problematic. I don't want to start using such names e
For the naming - I want to emphasize self.conf['devices'] and self._devices 
heavily. It's designed to make sense. (that should be virt's scope :) )

On to the error handling: typically, it is device's responsibility to 
initialize/teardown cleanly, that being said having generic Exception block 
here makes sense.
Line 1696: 
Line 1697: def _cleanupRecoveryFile(self):
Line 1698: self._recovery_file.cleanup()
Line 1699: 


Line 1869: 
Line 1870: # Runs before the underlying libvirt domain is created.
Line 1871: for dev_objects in self._devices.values():
Line 1872: for dev_object in dev_objects[:]:
Line 1873: dev_object.setup()
> Same comments as teardown - but here probably want to fail on the first exc
Had similar situation with hostdev lately. What do we do if setup at some point 
fails, we start tearing the devices down and the teardown fails? We'd have to 
guarantee no-raise teardown (in that case, I wouldn't change the block above)
Line 1874: 
Line 1875: if self.recovering:
Line 1876: self._dom = virdomain.Notifying(
Line 1877: self._connection.lookupByUUIDString(self.id),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 5: Code-Review+1

good point about error handling: let's set rules about that. Temporary lowering 
score until we settle.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 5: Code-Review-1

(2 comments)

Nice, but we need to think more about error handling.

https://gerrit.ovirt.org/#/c/55135/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1691: Runs after the underlying libvirt domain was destroyed.
Line 1692: """
Line 1693: for dev_objects in self._devices.values():
Line 1694: for dev_object in dev_objects[:]:
Line 1695: dev_object.teardown()
These naming style is problematic. I don't want to start using such names 
everywhere.

Why not:

for devices in self._devices.values():
for device in devices[:]:
device.teardown()

Another issue - what is teardown method raises? Are we going to fail the entire 
flow, leaving the system in inconsistent state?

Typically during teardown and cleanup, you log any exception and continue.
Line 1696: 
Line 1697: def _cleanupRecoveryFile(self):
Line 1698: self._recovery_file.cleanup()
Line 1699: 


Line 1869: 
Line 1870: # Runs before the underlying libvirt domain is created.
Line 1871: for dev_objects in self._devices.values():
Line 1872: for dev_object in dev_objects[:]:
Line 1873: dev_object.setup()
Same comments as teardown - but here probably want to fail on the first 
exception.

However, we don't want to leave some devices in setup state and some not setup. 
We may need to keep a list of devices finished setup and tear them down if some 
devices failed.
Line 1874: 
Line 1875: if self.recovering:
Line 1876: self._dom = virdomain.Notifying(
Line 1877: self._connection.lookupByUUIDString(self.id),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS4, Line 1690: for dev_object in dev_objects[:]
> In this and setup case, the underlying libvirt domain is not even started -
OK for setup, and right for teardown: I misread the code, apologies for that. 
Indeed we can't get here if dom.destroy() hasn't run, so the underlying libvirt 
domain should be in safe state, and no concurrent hot(un)plug is possible.

Could you please add one-line docstring/comment here and in the setup code, like
"Runs when the libvirt domain is destroyed"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4:

Verified w/ the followup patches, standard VM run

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS4, Line 1690: for dev_object in dev_objects[:]
> correct again, but what are we guarding against here?
In this and setup case, the underlying libvirt domain is not even started - 
hotplug and hotunplug shouldn't be allowed at all. It is really a safety 
measure in case something touched the devices anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4: Code-Review+1

(3 comments)

conceptually fine, we may want to tune some details of the implementation. 
Partial ACK to have further discussion.

https://gerrit.ovirt.org/#/c/55135/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS4, Line 1689: for dev_objects in self._devices.values():
> This is safe in both 2 and 3 as it iterates over device types (those are st
correct


PS4, Line 1690: for dev_object in dev_objects[:]
> This is also safe in both 2 and 3 as slice is still a copy
correct again, but what are we guarding against here?
It seems to me that the most likely issue is a concurrent hotplug/hotunplung, 
so the correct fix is a device lock.

We can't solve this nicely in the current architecture, so I believe this is 
good enough.


PS4, Line 1866: for dev_objects in self._devices.values():
  : for dev_object in dev_objects[:]:
For a future patch:
I wonder if an utility method that returns all the existing device object in a 
safe way could be helpful here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/55135/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS4, Line 1689: for dev_objects in self._devices.values():
This is safe in both 2 and 3 as it iterates over device types (those are static 
for us)


PS4, Line 1690: for dev_object in dev_objects[:]
This is also safe in both 2 and 3 as slice is still a copy


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[vfeenstr]

Vinzenz Feenstra has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1685: for con in self._devices[hwclass.CONSOLE]:
Line 1686: con.cleanup()
Line 1687: 
Line 1688: def _teardownDevices(self):
Line 1689: for dev_objects in self._devices.values():
> This is not safe on Python 3 - check if six has a helper that does nothing 
This is not really a problem because the keys aren't changing on runtime, 
however the bigger issue would be the iteration below
Line 1690: for dev_object in dev_objects:
Line 1691: dev_object.teardown()
Line 1692: 
Line 1693: def _cleanupRecoveryFile(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 3:

(2 comments)

https://gerrit.ovirt.org/#/c/55135/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1685: for con in self._devices[hwclass.CONSOLE]:
Line 1686: con.cleanup()
Line 1687: 
Line 1688: def _teardownDevices(self):
Line 1689: for dev_objects in self._devices.values():
This is not safe on Python 3 - check if six has a helper that does nothing on 2 
and create a list on 3.
Line 1690: for dev_object in dev_objects:
Line 1691: dev_object.teardown()
Line 1692: 
Line 1693: def _cleanupRecoveryFile(self):


Line 1862: for dev_name, dev in self._host_devices():
Line 1863: self.log.debug('Detaching device %s from the host.' 
% dev_name)
Line 1864: dev.detach()
Line 1865: 
Line 1866: for dev_objects in self._devices.values():
Same issue, same fix (six?)
Line 1867: for dev_object in dev_objects:
Line 1868: dev_object.setup()
Line 1869: 
Line 1870: if self.recovering:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 2:

I'm ok with the other answers you gave to my previous comments.
We can work out later the asymmetry between (lack of) setup and teardown.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 2: Code-Review-1

(2 comments)

one minor comment inside

https://gerrit.ovirt.org/#/c/55135/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS2, Line 1689: dev_type
minor, but will ever need to use dev_type here?

maybe just

  for dev_object in self._devices.values():
 # ... do something


let's keep in mind that in py3 values() will return a view-like object, hence 
we must ensure the safety of the iteration.


PS2, Line 1866: dev_type
same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

Line 82: pass
Line 83: 
Line 84: def teardown(self):
Line 85: """
Line 86: Actions to be executed before VM is destroyed.
> Before? I think this is after a vm is destroyed - you cannot change the per
Yes! Mixed them few times, will fix in next PS.
Line 87: """
Line 88: pass
Line 89: 
Line 90: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

Line 82: pass
Line 83: 
Line 84: def teardown(self):
Line 85: """
Line 86: Actions to be executed before VM is destroyed.
Before? I think this is after a vm is destroyed - you cannot change the 
permissions of a device before destroying the vm, right?
Line 87: """
Line 88: pass
Line 89: 
Line 90: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

PS1, Line 80: in the recovery flow.
> the information is correct, but we actually mean "when the device object is
Thinking of it further, not really. That could've been confused with 
constructor. It is something that happens after __init__ and before getXML.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Nir Soffer 
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: add device setup and teardown

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS1, Line 1866: for dev_type, dev_objects in self._devices.items():
  : for dev_object in dev_objects:
  : dev_object.setup()
> (minor) I'd have setupDevices for symmetry.
Sure for the setupDevices.

Although I hate common modules, if you feel it is way better then I'm fine with 
it. I'm afraid of hiding the fact that it'll operate on _devices objects.


https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

PS1, Line 79: right before VM's XML is generated.
> not sure this is the best place
It is *the* place for this in my opinion. I think that the docstring is 
awkwardly worded (also applies for comment below).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
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: add device setup and teardown

[fromani]

Francesco Romani has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1: Code-Review-1

(3 comments)

I like the idea, but we need to tune some details. -1 for visibility while we 
discuss.

https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS1, Line 1866: for dev_type, dev_objects in self._devices.items():
  : for dev_object in dev_objects:
  : dev_object.setup()
(minor) I'd have setupDevices for symmetry.

Perhaps: make both plain functions and move into vmdevices.common ?

It *is* debatable to have this as plain functions, but same applies for 
methods: we don't use 'self' much after all.


https://gerrit.ovirt.org/#/c/55135/1/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

PS1, Line 79: right before VM's XML is generated.
not sure this is the best place


PS1, Line 80: in the recovery flow.
the information is correct, but we actually mean "when the device object is 
created", don't we?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
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: add device setup and teardown

[automation]

gerrit-hooks has posted comments on this change.

Change subject: virt: add device setup and teardown
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
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: add device setup and teardown

[mpolednik]

Martin Polednik has uploaded a new change for review.

Change subject: virt: add device setup and teardown
..

virt: add device setup and teardown

More and more devices require some action to be taken before VM is
started - even before the XML is generated. We add two methods, setup
and teardown, that are called before VM's start and after VM's
destruction respectively.

These methods should be used as a replacement for device-specific
methods.

Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Signed-off-by: Martin Polednik 
---
M vdsm/virt/vm.py
M vdsm/virt/vmdevices/core.py
2 files changed, 23 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/55135/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index a133a9b..dde665c 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1678,11 +1678,17 @@
 self._cleanupDrives()
 self._cleanupFloppy()
 self._cleanupGuestAgent()
+self._teardownDevices()
 cleanup_guest_socket(self._qemuguestSocketFile)
 self._cleanupStatsCache()
 numaUtils.invalidateNumaCache(self)
 for con in self._devices[hwclass.CONSOLE]:
 con.cleanup()
+
+def _teardownDevices(self):
+for dev_type, dev_objects in self._devices.items():
+for dev_object in dev_objects:
+dev_object.teardown()
 
 def _cleanupRecoveryFile(self):
 self._recovery_file.cleanup()
@@ -1857,6 +1863,10 @@
 self.log.debug('Detaching device %s from the host.' % dev_name)
 dev.detach()
 
+for dev_type, dev_objects in self._devices.items():
+for dev_object in dev_objects:
+dev_object.setup()
+
 if self.recovering:
 self._dom = virdomain.Notifying(
 self._connection.lookupByUUIDString(self.id),
diff --git a/vdsm/virt/vmdevices/core.py b/vdsm/virt/vmdevices/core.py
index 5951f7d..c5f0d95 100644
--- a/vdsm/virt/vmdevices/core.py
+++ b/vdsm/virt/vmdevices/core.py
@@ -74,6 +74,19 @@
 """
 raise NotImplementedError()
 
+def setup(self):
+"""
+Actions to be executed right before VM's XML is generated. Not executed
+in the recovery flow.
+"""
+pass
+
+def teardown(self):
+"""
+Actions to be executed before VM is destroyed.
+"""
+pass
+
 
 class Generic(Base):
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches