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

nsoffer at redhat.com nsoffer at redhat.com
Wed Nov 25 22:05:58 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 5:

(8 comments)

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

Line 1261:     <disk type='file' snapshot='no'>
Line 1262:         <source file='test_path'/>
Line 1263:     </disk>"""
Line 1264:     NETWORK_DRIVE_XML = """
Line 1265:     <disk type='file' snapshot='no'>
type="network"
Line 1266:         <source name='test_path'/>
Line 1267:     </disk>"""
Line 1268:     BLOCK_DRIVE_XML = """
Line 1269:     <disk type='file'  snapshot='no'>


Line 1265:     <disk type='file' snapshot='no'>
Line 1266:         <source name='test_path'/>
Line 1267:     </disk>"""
Line 1268:     BLOCK_DRIVE_XML = """
Line 1269:     <disk type='file'  snapshot='no'>
type="block"

See vmStorageTests for example xmls.
Line 1270:         <source dev='/block_path'/>
Line 1271:     </disk>"""
Line 1272: 
Line 1273:     drive_default = Drive({}, log=logging.getLogger(''), index=0, iface="",


Line 1270:         <source dev='/block_path'/>
Line 1271:     </disk>"""
Line 1272: 
Line 1273:     drive_default = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1274:                           path='test_path', )
We can remove the default, we are not testing the Drive class here.
Line 1275:     drive_file = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1276:                        path='test_path',  diskType=DISK_TYPE.FILE)
Line 1277:     drive_network = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1278:                           path='test_path', diskType=DISK_TYPE.NETWORK)


Line 1276:                        path='test_path',  diskType=DISK_TYPE.FILE)
Line 1277:     drive_network = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1278:                           path='test_path', diskType=DISK_TYPE.NETWORK)
Line 1279:     drive_block = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1280:                         path= "/block_path", diskType=DISK_TYPE.BLOCK)
path is always /rhev/data-center/p/d/images/i/v, (p=poolid, d=domainid, i=imageid, v=volumeid) for file and block. For network the path is rbd:pool/volume, but it does not really matter. For the tests, using the same "test_path" is fine.
Line 1281: 
Line 1282:     @MonkeyPatch(vm, "config", make_config([
Line 1283:         ("vars", "hotunplug_timeout", "0.1"),
Line 1284:         ("vars", "hotunplug_check_interval", "0.03")


Line 1279:     drive_block = Drive({}, log=logging.getLogger(''), index=0, iface="",
Line 1280:                         path= "/block_path", diskType=DISK_TYPE.BLOCK)
Line 1281: 
Line 1282:     @MonkeyPatch(vm, "config", make_config([
Line 1283:         ("vars", "hotunplug_timeout", "0.1"),
> Made it still shorter.
0.03 is too short for overloaded ci slaves.

Lets use timeout=0.25, check_internal=0.1 will time out after retries, in the worst case after one retry, good enough.
Line 1284:         ("vars", "hotunplug_check_interval", "0.03")
Line 1285:     ]))
Line 1286:     @MonkeyPatch(utils, "isBlockDevice", lambda x: x == "/block_path")
Line 1287:     @permutations([[drive_default, FILE_DRIVE_XML],


Line 1282:     @MonkeyPatch(vm, "config", make_config([
Line 1283:         ("vars", "hotunplug_timeout", "0.1"),
Line 1284:         ("vars", "hotunplug_check_interval", "0.03")
Line 1285:     ]))
Line 1286:     @MonkeyPatch(utils, "isBlockDevice", lambda x: x == "/block_path")
> Drive actually goes to the file system to check if the device is a block de
Looks ok, the actual code is dirty in this case :-)
Line 1287:     @permutations([[drive_default, FILE_DRIVE_XML],
Line 1288:                    [drive_file, FILE_DRIVE_XML],
Line 1289:                    [drive_network, NETWORK_DRIVE_XML],
Line 1290:                    [drive_block, BLOCK_DRIVE_XML]])


Line 1338:                 <source file='invalid_test_path'/>
Line 1339:             </disk>
Line 1340:             <disk type='file' device='disk' snapshot='no'>
Line 1341:                 <source file='invalid_test_path'/>
Line 1342:             </disk>
Lets update the xml of the devices to look more like the real xml, see vmStorageTests.
Line 1343:             <interface>
Line 1344:                 <mac address='invalid_macAddr'/>
Line 1345:             </interface>
Line 1346:         </devices>


Line 1356: 
Line 1357:     def XMLDesc(self, flags):
Line 1358:         self.xmldesc_fetched += 1
Line 1359:         if self.xmldesc_fetched > self.times_to_match + 1:
Line 1360:             raise BaseException(
You should raise an Exception, Base Exception is not for user code (in general).

But I don't think we need it, it will make it less useful, I thin we should just count the fetches and assert about them in the tests.
Line 1361:                 "_waitForDriveRemoval called XMLDesc too many times.")
Line 1362:         if self.xmldesc_fetched > self.times_to_match:
Line 1363:             return self.DEFAULT_XML
Line 1364:         else:


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