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

nsoffer at redhat.com nsoffer at redhat.com
Fri Mar 6 23:04:02 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 8:

(6 comments)

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:

    @permuatations([["stop", "report", ...])
    def test_xxx(self, policy):
        ...
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 policy.
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
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 permutations:

    @permutaions([
        ["stop", "on"],  ["stop", "off"],
        ["enospace", "on"],  ["enospace", "off"],
        ["report", "on"],  ["report", "off"],
        ["ignore", "on"],  ["ignore", "off"],
    ])
    def test_policy(sefl, policy, propagateErrors):
        ...
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:

    driverAttrs['error_policy'] = self.errorPolicy

    @property
    def errorPolicy(self):
        if hasattr('_errorPolicy'):
            return self._errorPolicy
        else if (self.propagateErrors == 'on' or
                   utils.tobool(self.propagateErrors)):
            return "enospace"
        else:
            return "stop"
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 very useful object.
  
    @errorPolicy.setter
    def errorPolicy(self, value):
        if value not in _ERROR_POLICIES:
            raise ...
        self._errorPolicy = value
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: 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