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

fromani at redhat.com fromani at redhat.com
Mon Mar 9 07:38:04 UTC 2015


Francesco Romani has posted comments on this change.

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


Patch Set 8:

(7 comments)

https://gerrit.ovirt.org/#/c/38009/8//COMMIT_MSG
Commit Message:

Line 11: the guest OS, pausing the VM only if disk space was exhausted.
Line 12: 
Line 13: This approach was nice and simple, but sometimes just too limiting
Line 14: and some users asked for more fine-grained drive error policy
Line 15: control.
> Currently we support only "stop" and "enospace", and we have mechanism for 
This is a very valid point, but better to continue this conversation on BZ (as you already begun to do).

Meantime, I will update this patch as you suggested.
If we settle to just export 'propagateErrors' on Engine I'll drop this patch completely.
Line 16: 
Line 17: To allow that, we add a new Drive property, 'errorPolicy', which
Line 18: will superceed over the legacy, limited 'propagateErrors' tunable,
Line 19: in the case both are given.


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

Line 272:             io="threads" name="qemu" type="raw"/>
Line 273:         """
Line 274: 
Line 275:     @permutations([['stop', 'stop'], ['report', 'report'],
Line 276:                   ['ignore', 'ignore'], ['enospace', 'enospace']])
> We can simplify the permutations, as policy == expected:
Done
Line 277:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 278:     def test_supported_policy(self, policy, expected):
Line 279:         conf = drive_config(errorPolicy=policy)
Line 280:         drive = Drive({}, self.log, **conf)


Line 275:     @permutations([['stop', 'stop'], ['report', 'report'],
Line 276:                   ['ignore', 'ignore'], ['enospace', 'enospace']])
Line 277:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 278:     def test_supported_policy(self, policy, expected):
Line 279:         conf = drive_config(errorPolicy=policy)
> Actually this test the combination of propagateErrors='off' and the new pol
will rename. Or you want it dropped?
Line 280:         drive = Drive({}, self.log, **conf)
Line 281:         xml = find_xml_element(drive.getXML().toxml(), './driver')
Line 282:         self.assertXMLEqual(xml, self.EXPECTED_XML % expected)
Line 283: 


Line 281:         xml = find_xml_element(drive.getXML().toxml(), './driver')
Line 282:         self.assertXMLEqual(xml, self.EXPECTED_XML % expected)
Line 283: 
Line 284:     @permutations([['stop', 'stop'], ['report', 'report'],
Line 285:                   ['ignore', 'ignore'], ['enospace', 'enospace']])
> Same as previous test
will rename in the next submission
Line 286:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 287:     def test_supported_policy_override_legacy(self, policy, expected):
Line 288:         conf = drive_config(errorPolicy=policy, propagateErrors='on')
Line 289:         drive = Drive({}, self.log, **conf)


Line 287:     def test_supported_policy_override_legacy(self, policy, expected):
Line 288:         conf = drive_config(errorPolicy=policy, propagateErrors='on')
Line 289:         drive = Drive({}, self.log, **conf)
Line 290:         xml = find_xml_element(drive.getXML().toxml(), './driver')
Line 291:         self.assertXMLEqual(xml, self.EXPECTED_XML % expected)
> The first two tests are identical, we can replace them with one test using 
will merge
Line 292: 
Line 293:     @permutations([[''], [None], [1], ['foobar']])
Line 294:     @MonkeyPatch(utils, 'isBlockDevice', lambda path: False)
Line 295:     def test_unsupported_policy(self, policy):


https://gerrit.ovirt.org/#/c/38009/8/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 354:                 driverAttrs['type'] = 'raw'
Line 355: 
Line 356:             driverAttrs['cache'] = self.cache
Line 357: 
Line 358:             self._setDriverAttrErrorPolicy(driverAttrs)
> This would be nicer and more testable:
indeed nicer. Will change.
Line 359: 
Line 360:             diskelem.appendChildWithArgs('driver', **driverAttrs)
Line 361: 
Line 362:         if hasattr(self, 'specParams') and 'ioTune' in self.specParams:


Line 369:         return diskelem
Line 370: 
Line 371:     def _setDriverAttrErrorPolicy(self, driverAttrs):
Line 372:         if hasattr(self, 'errorPolicy'):
Line 373:             if self.errorPolicy in _ERROR_POLICIES:
> Lets validate when setting errorPolicy. A drive raising in getXML() is not 
Indeed nicer. Will change.
Line 374:                 driverAttrs['error_policy'] = self.errorPolicy
Line 375:             else:
Line 376:                 raise ValueError("unsupported Error Policy '%s",
Line 377:                                  self.errorPolicy)


-- 
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Ravi Nori <rnori 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