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

nsoffer at redhat.com nsoffer at redhat.com
Tue Apr 12 08:10:38 UTC 2016


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, right?
Line 421:         self.__calls__ = []
Line 422: 
Line 423:     @recorded
Line 424:     def setup(self):


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 421:         self.__calls__ = []
Not needed, the recorded decorator add the __calls__ on the first access.
Line 422: 
Line 423:     @recorded
Line 424:     def setup(self):
Line 425:         if self.fail_setup:


Line 422: 
Line 423:     @recorded
Line 424:     def setup(self):
Line 425:         if self.fail_setup:
Line 426:             raise Exception('TEST_FAIL')
I would log here:

    self.log.info("% setup", self.name)
Line 427: 
Line 428:     @recorded
Line 429:     def teardown(self):
Line 430:         if self.fail_teardown:


Line 427: 
Line 428:     @recorded
Line 429:     def teardown(self):
Line 430:         if self.fail_teardown:
Line 431:             raise Exception('TEST_FAIL')
Same, log success


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 module to have such ugliness. The real problem we have is the conf dict, duplicating the nice devices objects.

If you want to make it clear what is a device object and what is a dict, mark the dicts in conf, not the devices. The ugly stuff should be more ugly to remind us to remove it, and the good stuff should stay clean.
Line 1693:                 try:
Line 1694:                     dev_object.teardown()
Line 1695:                 except Exception:
Line 1696:                     self.log.exception("Failed to teardown device.")


Line 1692:             for dev_object in dev_objects[:]:
Line 1693:                 try:
Line 1694:                     dev_object.teardown()
Line 1695:                 except Exception:
Line 1696:                     self.log.exception("Failed to teardown device.")
Log is not a sentence and it does not need trailing period - look around in other vdsm logs.
Line 1697: 
Line 1698:     def _cleanupRecoveryFile(self):
Line 1699:         self._recovery_file.cleanup()
Line 1700: 


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:

    Failed to setup device %s, dev_object
Line 1850:                     self._revert_setup_devices(done)
Line 1851:                     raise
Line 1852:                 else:
Line 1853:                     done.append(dev_object)


Line 1847:                 except Exception:
Line 1848:                     self.log.exception("Failed to setup device %s.",
Line 1849:                                        dev_object.device)
Line 1850:                     self._revert_setup_devices(done)
Line 1851:                     raise
Not sure that this raises the correct exception or the last one raised, please verify in the tests.
Line 1852:                 else:
Line 1853:                     done.append(dev_object)
Line 1854: 
Line 1855:     def _revert_setup_devices(self, dev_objects):


Line 1851:                     raise
Line 1852:                 else:
Line 1853:                     done.append(dev_object)
Line 1854: 
Line 1855:     def _revert_setup_devices(self, dev_objects):
_teardown_setup_devices?
Line 1856:         for dev_object in dev_objects:
Line 1857:             try:
Line 1858:                 dev_object.teardown()
Line 1859:             except Exception:


Line 1854: 
Line 1855:     def _revert_setup_devices(self, dev_objects):
Line 1856:         for dev_object in dev_objects:
Line 1857:             try:
Line 1858:                 dev_object.teardown()
Same issue with the names.
Line 1859:             except Exception:
Line 1860:                 self.log.exception("Device %s in inconsistent state.",
Line 1861:                                    dev_object.device)
Line 1862:                 continue


Line 1856:         for dev_object in dev_objects:
Line 1857:             try:
Line 1858:                 dev_object.teardown()
Line 1859:             except Exception:
Line 1860:                 self.log.exception("Device %s in inconsistent state.",
Better:

    self.log.exception("Failed to tear down device %s, device in inconsistent state", device)

The exception may give no clue about the device.
Line 1861:                                    dev_object.device)
Line 1862:                 continue
Line 1863: 
Line 1864:     def _run(self):


Line 1858:                 dev_object.teardown()
Line 1859:             except Exception:
Line 1860:                 self.log.exception("Device %s in inconsistent state.",
Line 1861:                                    dev_object.device)
Line 1862:                 continue
> useless, but I don't mind
I do, please remove useless code.
Line 1863: 
Line 1864:     def _run(self):
Line 1865:         self.log.info("VM wrapper has started")
Line 1866:         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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list