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 <mpoled...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
