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