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

mmirecki at redhat.com mmirecki at redhat.com
Mon Nov 23 15:00:30 UTC 2015


Marcin Mirecki has posted comments on this change.

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


Patch Set 1:

(8 comments)

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

Line 1256:             pass
Line 1257:         drive = Drive()
Line 1258:         drive.name = ''
Line 1259:         drive.diskType = DISK_TYPE.FILE
Line 1260:         drive.path = 'test_path'
> Why not use a real drive object? See vmStorageTests.py to see how to use re
Done
Line 1261:         with fake.VM() as testvm:
Line 1262:             testvm._dom = FakeVmDom(999)
Line 1263:             testvm._hotunplug_timeout = 1
Line 1264:             try:


Line 1257:         drive = Drive()
Line 1258:         drive.name = ''
Line 1259:         drive.diskType = DISK_TYPE.FILE
Line 1260:         drive.path = 'test_path'
Line 1261:         with fake.VM() as testvm:
> fake.VM is too complex, we cannot use such monster in the tests. I think a 
Done
Line 1262:             testvm._dom = FakeVmDom(999)
Line 1263:             testvm._hotunplug_timeout = 1
Line 1264:             try:
Line 1265:                 testvm._waitForDriveRemoval(drive)


Line 1261:         with fake.VM() as testvm:
Line 1262:             testvm._dom = FakeVmDom(999)
Line 1263:             testvm._hotunplug_timeout = 1
Line 1264:             try:
Line 1265:                 testvm._waitForDriveRemoval(drive)
> Yes, but 2 seconds is too much.
With both values changed to 0, utils.monotonic_time() >= deadline is true after about calling sleep(0) about 2 to 4 times.
Line 1266:                 raise BaseException("HotunplugTimeout should be raised")
Line 1267:             except HotunplugTimeout:
Line 1268:                 pass
Line 1269: 


Line 1264:             try:
Line 1265:                 testvm._waitForDriveRemoval(drive)
Line 1266:                 raise BaseException("HotunplugTimeout should be raised")
Line 1267:             except HotunplugTimeout:
Line 1268:                 pass
> Use self.assertRaises instead.
Done
Line 1269: 
Line 1270:             testvm._dom = FakeVmDom()
Line 1271:             testvm._hotunplug_timeout = 1
Line 1272:             testvm._wait_for_sleep = self._wait_for_sleep


Line 1265:                 testvm._waitForDriveRemoval(drive)
Line 1266:                 raise BaseException("HotunplugTimeout should be raised")
Line 1267:             except HotunplugTimeout:
Line 1268:                 pass
Line 1269: 
> The code bellow should be in another test - we want to test one scenario pe
Done
Line 1270:             testvm._dom = FakeVmDom()
Line 1271:             testvm._hotunplug_timeout = 1
Line 1272:             testvm._wait_for_sleep = self._wait_for_sleep
Line 1273:             testvm._waitForDriveRemoval(drive)


Line 1298:                 </disk>
Line 1299:               </devices>
Line 1300:              </domain>
Line 1301:             """
Line 1302:         return '<does_not_match/>'
> This is good only for drive test, but we need it also for the nic in the ne
Done
Line 1303: 
Line 1304: 
Line 1305: class ChangingSchedulerDomain(object):
Line 1306: 


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

Line 325:         self._vcpuTuneInfo = {}
Line 326:         self._numaInfo = {}
Line 327:         self._vmJobs = None
Line 328:         self._clientPort = ''
Line 329:         self._hotunplug_timeout = config.getint('vars', 'hotunplug_timeout')
> Vm is too big, we cannot add more state.
Done
Line 330: 
Line 331:     def _get_lastStatus(self):
Line 332:         # note that we don't use _statusLock here. One of the reasons is the
Line 333:         # non-obvious recursive locking in the following flow:


Line 2689:                 if utils.monotonic_time() > deadline:
Line 2690:                     raise HotunplugTimeout("Timeout detaching drive %s"
Line 2691:                                            % drive.name)
Line 2692: 
Line 2693:     def _wait_for_sleep(self):
> Yes, but again VM is too big, we cannot add such methods for testing. The b
Done
Line 2694:         time.sleep(1)
Line 2695: 
Line 2696:     def _isDriveAttached(self, drive):
Line 2697:         root = ET.fromstring(self._dom.XMLDesc(0))


-- 
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: 1
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