Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

mmirecki at redhat.com mmirecki at redhat.com
Tue Nov 24 11:16:31 UTC 2015


Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
......................................................................


Patch Set 3:

(13 comments)

https://gerrit.ovirt.org/#/c/48880/3/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 145: 
Line 146:         ('hotunplug_timeout', '30',
Line 147:             'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149:         ('hotunplug_sleep_time', '1',
> Maybe hotunplug_check_interval?
Done
Line 150:             'Sleep time between consecutive checks for VM device detach'),
Line 151: 
Line 152:         ('vm_watermark_interval', '2',
Line 153:             'How often should we check drive watermark on block storage for '


Line 146:         ('hotunplug_timeout', '30',
Line 147:             'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149:         ('hotunplug_sleep_time', '1',
Line 150:             'Sleep time between consecutive checks for VM device detach'),
> Maybe "Time to wait (in seconds) between consecutive checks for device remo
Done
Line 151: 
Line 152:         ('vm_watermark_interval', '2',
Line 153:             'How often should we check drive watermark on block storage for '
Line 154:             'automatic extension of thin provisioned volumes (seconds).'),


https://gerrit.ovirt.org/#/c/48880/3/tests/vmTests.py
File tests/vmTests.py:

Line 1005: 
Line 1006:     DRIVE_XML = """
Line 1007:     <disk type='file' device='disk' snapshot='no'>
Line 1008:         <source file='test_path'/>
Line 1009:     </disk>"""
> This is used only by the wait for removal tests, should be in another class
Done
Line 1010: 
Line 1011:     @MonkeyPatch(libvirtconnection, 'get', lambda x: fake.Connection())
Line 1012:     @permutations([[define.NORMAL], [define.ERROR]])
Line 1013:     def testTimeOffsetNotPresentByDefault(self, exitCode):


Line 1257:         with fake.VM() as testvm:
Line 1258:             testvm._dom = fake.Domain(vmId='testvm')
Line 1259:             self.assertFalse(response.is_error(testvm.acpiShutdown()))
Line 1260: 
Line 1261:     @MonkeyPatch(config, 'getint', lambda x, y: 0)
> Nice hack, but too dirty :-)
Done
Line 1262:     def test_wait_for_drive_removal_timeout(self):
Line 1263:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1264:         testvm = TestingVm(fake.Domain())
Line 1265: 


Line 1262:     def test_wait_for_drive_removal_timeout(self):
Line 1263:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1264:         testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266:         testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=99)
> Why not create the TestingVm with FakeVmDom() ?
Done
Line 1267:         self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive)
Line 1268: 
Line 1269:     @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270:     def test_wait_for_drive_removal_removed_on_first_check(self):


Line 1263:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1264:         testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266:         testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=99)
Line 1267:         self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive)
> To test this, it is enough to do couple of loops. We can override the timeo
Done
Line 1268: 
Line 1269:     @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270:     def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1271:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")


Line 1271:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1272:         testvm = TestingVm(fake.Domain())
Line 1273: 
Line 1274:         testvm._dom = FakeVmDom(self.DRIVE_XML)
Line 1275:         testvm._waitForDriveRemoval(drive)
> Nice!
We can count the sleeps in FakeVmDom
Adding:
  self.assertEqual(testvm._dom._result_count, 0)
which check how many times we have waited.
Line 1276: 
Line 1277:     @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1278:     def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1279:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")


Line 1280:         testvm = TestingVm(fake.Domain())
Line 1281: 
Line 1282:         testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=2)
Line 1283:         testvm._waitForDriveRemoval(drive)
Line 1284:         self.assertEqual(testvm._dom._result_count, 2)
> Like the previous test, with timeout=0.5 (sometimes we run on overloaded sl
Increasing the timeout to 1s.
The 1s should never be reached, as it should exit after 2 matching responses from dom.
Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 


Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 
Line 1289:     def __init__(self, device_xml, times_to_return_matching=0):
> This is little too long
Done
Line 1290:         self._times_to_return_matching = times_to_return_matching
Line 1291:         self._result_count = 0
Line 1292:         self.result_xml = ET.fromstring(
Line 1293:             """


Line 1287: class FakeVmDom(object):
Line 1288: 
Line 1289:     def __init__(self, device_xml, times_to_return_matching=0):
Line 1290:         self._times_to_return_matching = times_to_return_matching
Line 1291:         self._result_count = 0
> Why not use:
Done
Line 1292:         self.result_xml = ET.fromstring(
Line 1293:             """
Line 1294:             <domain type='kvm' id='2'>
Line 1295:                 <devices/>


Line 1294:             <domain type='kvm' id='2'>
Line 1295:                 <devices/>
Line 1296:             </domain>
Line 1297:             """)
Line 1298:         self.result_xml.find("devices").append(ET.fromstring(device_xml))
> Why keep etree element and return xml, when we can prepare the xml once in 
Done
Line 1299: 
Line 1300:     def XMLDesc(self, anything):
Line 1301:         if self._result_count < self._times_to_return_matching:
Line 1302:             self._result_count += 1


Line 1299: 
Line 1300:     def XMLDesc(self, anything):
Line 1301:         if self._result_count < self._times_to_return_matching:
Line 1302:             self._result_count += 1
Line 1303:             return ET.tostring(self.result_xml)
> We can simplify a little bit:
Done
Line 1304:         return '<does_not_match/>'
Line 1305: 
Line 1306: 
Line 1307: class ChangingSchedulerDomain(object):


Line 1300:     def XMLDesc(self, anything):
Line 1301:         if self._result_count < self._times_to_return_matching:
Line 1302:             self._result_count += 1
Line 1303:             return ET.tostring(self.result_xml)
Line 1304:         return '<does_not_match/>'
> Nice hack, but it would better to return the expected xml.
Done
Line 1305: 
Line 1306: 
Line 1307: class ChangingSchedulerDomain(object):
Line 1308: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki 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