Change in vdsm[master]: vm: drive: allow error policy fine tuning

nsoffer at redhat.com nsoffer at redhat.com
Tue Feb 24 20:21:56 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: vm: drive: allow error policy fine tuning
......................................................................


Patch Set 4:

(7 comments)

https://gerrit.ovirt.org/#/c/38009/4/tests/vmStorageTests.py
File tests/vmStorageTests.py:

Line 265: 
Line 266: @expandPermutations
Line 267: class ErrorPolicyTests(XMLTestCase):
Line 268: 
Line 269:     _xml_template = """
We don't have to use _private names in a test class. Nobody is using this object so xml_template is safe enough.

Since this is a constant, it should be XML_TEMPLATE
Line 270:             <disk device="disk" snapshot="no" type="file">
Line 271:                 <source file="/path/to/volume"/>
Line 272:                 <target bus="virtio" dev="vda"/>
Line 273:                 <serial>54-a672-23e5b495a9ea</serial>


Line 273:                 <serial>54-a672-23e5b495a9ea</serial>
Line 274:                 <driver cache="none" error_policy="%s"
Line 275:                         io="threads" name="qemu" type="raw"/>
Line 276:             </disk>
Line 277:             """
Using a template is fragile - if other unrelated code will change (e.g. one formatting the <source>), all these tests will fail for no good reason.

I think we should generated xml and find the relevant element (driver) and compare only that element. Some tests in vmTests are doing this, using find() which could be a testlib helper.
Line 278: 
Line 279:     @permutations([['stop', 'stop'], ['report', 'report'],
Line 280:                   ['ignore', 'ignore'], ['enospace', 'enospace']])
Line 281:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)


Line 278: 
Line 279:     @permutations([['stop', 'stop'], ['report', 'report'],
Line 280:                   ['ignore', 'ignore'], ['enospace', 'enospace']])
Line 281:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 282:     def test_set_supported_policy(self, policy, expected):
test_set_supported -> test_supported?
Line 283:         conf = drive_config(errorPolicy=policy)
Line 284:         drive = Drive({}, self.log, **conf)
Line 285:         self.assertXMLEqual(drive.getXML().toxml(),
Line 286:                             self._xml_template % expected)


Line 286:                             self._xml_template % expected)
Line 287: 
Line 288:     @permutations([[''], [None], [1], ['foobar']])
Line 289:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 290:     def test_set_unsupported_policy(self, policy):
test_set_unsupported -> test_unsupported?
Line 291:         conf = drive_config(errorPolicy=policy)
Line 292:         drive = Drive({}, self.log, **conf)
Line 293:         self.assertRaises(ValueError, drive.getXML)
Line 294: 


Line 293:         self.assertRaises(ValueError, drive.getXML)
Line 294: 
Line 295:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 296:     def test_propagate_errors(self):
Line 297:         # better explicit than implicit
Unneeded comment.
Line 298:         conf = drive_config(propagateErrors='on')
Line 299:         drive = Drive({}, self.log, **conf)
Line 300:         self.assertXMLEqual(drive.getXML().toxml(),
Line 301:                             self._xml_template % 'enospace')


Line 294: 
Line 295:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 296:     def test_propagate_errors(self):
Line 297:         # better explicit than implicit
Line 298:         conf = drive_config(propagateErrors='on')
How errorPolicy and propagateErrors='on' mix? do we need tests for conflicting options?
Line 299:         drive = Drive({}, self.log, **conf)
Line 300:         self.assertXMLEqual(drive.getXML().toxml(),
Line 301:                             self._xml_template % 'enospace')
Line 302: 


Line 300:         self.assertXMLEqual(drive.getXML().toxml(),
Line 301:                             self._xml_template % 'enospace')
Line 302: 
Line 303:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 304:     def test_not_propagate_errors(self):
not -> do_not?
Line 305:         conf = drive_config(propagateErrors='off')
Line 306:         drive = Drive({}, self.log, **conf)
Line 307:         self.assertXMLEqual(drive.getXML().toxml(),
Line 308:                             self._xml_template % 'stop')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b1234ab15fc87ad75dfeae197d058eec23accbc
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list