Change in vdsm[master]: sdm: Add create_volume job

nsoffer at redhat.com nsoffer at redhat.com
Sat Dec 12 10:10:27 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
......................................................................


Patch Set 3: Code-Review-1

(15 comments)

Partial review, I like this very much.

https://gerrit.ovirt.org/#/c/50221/3/tests/sdm_create_volume_test.py
File tests/sdm_create_volume_test.py:

Line 38: 
Line 39: class FakeDomainManifest(object):
Line 40:     def __init__(self, sd_id):
Line 41:         self.sdUUID = sd_id
Line 42: 
We can at record this an verify that a job verified the arguments.

Lets also check the case when this raises.
Line 43:     def validateCreateVolumeParams(self, *args):
Line 44:         pass
Line 45: 
Line 46:     @recorded


Line 73:         self.vol_id = vol_id
Line 74: 
Line 75:     # TODO: record these calls and verify them in the tests.
Line 76: 
Line 77:     def create(self, *args):
Lets test that we deal correctly with failures here (locks released, error reported)
Line 78:         pass
Line 79: 
Line 80:     def commit(self):
Line 81:         pass


Line 76: 
Line 77:     def create(self, *args):
Line 78:         pass
Line 79: 
Line 80:     def commit(self):
Same
Line 81:         pass
Line 82: 
Line 83: 
Line 84: class CreateVolumeTests(VdsmTestCase):


Line 82: 
Line 83: 
Line 84: class CreateVolumeTests(VdsmTestCase):
Line 85: 
Line 86:     def _get_args(self):
Better call this setUp, and set the needed test state in self.
Line 87:         job_id = str(uuid.uuid4())
Line 88:         host_id = 1
Line 89:         dom_manifest = FakeDomainManifest(str(uuid.uuid4()))
Line 90:         vol_info = dict(img_id=str(uuid.uuid4()), vol_id=str(uuid.uuid4()),


Line 101:                                 self.rm)]):
Line 102:             yield
Line 103: 
Line 104:     def test_create_volume(self):
Line 105:         args = self._get_args()
You are re-inventing setUp()
Line 106:         job = storage.sdm.api.create_volume.Job(**args)
Line 107: 
Line 108:         with self._fake_env():
Line 109:             job.run()


Line 127:     def test_create_volume_domainlock_contended(self):
Line 128:         def error(*args):
Line 129:             raise se.AcquireLockFailure('id', 'rc', 'out', 'err')
Line 130: 
Line 131:         args = self._get_args()
You are re-inventing setUp()
Line 132:         args['dom_manifest'].acquireDomainLock = error
Line 133:         job = storage.sdm.api.create_volume.Job(**args)
Line 134:         job.run()
Line 135:         wait_for_job(job)


https://gerrit.ovirt.org/#/c/50221/3/vdsm/storage/sdm/api/__init__.py
File vdsm/storage/sdm/api/__init__.py:

Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Line 21: __all__ = [
Please avoid this, we don't want code that does "from sdm import *", adding 20 names to the global namespace.
Line 22:     'create_volume',


https://gerrit.ovirt.org/#/c/50221/3/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 28: 
Line 29: rmanager = rm.ResourceManager.getInstance()
Line 30: 
Line 31: 
Line 32: class Job(sdm_job.SdmJob):
Please keep empty line after between class and __init__.
Line 33:     def __init__(self, job_id, host_id, dom_manifest, vol_info):
Line 34:         super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 35:         self.dom_manifest = dom_manifest
Line 36:         self.vol_info = _CreateVolumeInfo(vol_info)


Line 36:         self.vol_info = _CreateVolumeInfo(vol_info)
Line 37: 
Line 38:     def _run(self):
Line 39:         self.dom_manifest.validateCreateVolumeParams(
Line 40:             self.vol_info.vol_format, self.vol_info.parent_vol_id)
Since we validate vol_info in __init__, we should also validate these parameters in __init__. I would copy the logic from dom_manifest into CreateVolumeInfo, so we don't have to depend on this class method, or if you want to avoid the duplication, send the manifest to  CreateVolumeInfo.__init__.
Line 41: 
Line 42:         with self.dom_manifest.domain_lock(self.host_id):
Line 43:             image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 44:                                            IMAGE_NAMESPACE)


Line 50:                                      self.vol_info.vol_id)
Line 51:                 artifacts.create(
Line 52:                     self.vol_info.size, self.vol_info.vol_format,
Line 53:                     self.vol_info.disk_type, self.vol_info.desc,
Line 54:                     self.vol_info.parent_vol_id)
Wny not pass vol_info to artifacts.create?
Line 55:                 artifacts.commit()
Line 56: 
Line 57: # TODO: Adopt the properties framework for managing complex verb parameters
Line 58: 


Line 56: 
Line 57: # TODO: Adopt the properties framework for managing complex verb parameters
Line 58: 
Line 59: 
Line 60: class _CreateVolumeInfo(object):
We don't need to keep this private. In the future, it would be nice if the bridge creates this class (based on the type name in the schema), so vol_info would be an instance of this class.

Also, keeping same name as the in the schema makes it easier to grep  later.

Don't try to do this now :-)
Line 61:     def __init__(self, params):
Line 62:         self.img_id = _get_required(params, 'img_id')
Line 63:         self.vol_id = _get_required(params, 'vol_id')
Line 64:         self.size = _get_required(params, 'size')


Line 58: 
Line 59: 
Line 60: class _CreateVolumeInfo(object):
Line 61:     def __init__(self, params):
Line 62:         self.img_id = _get_required(params, 'img_id')
Note that this will blow up before the job exists, so the request will fail before adding the job.

If engine lost connection at this point, it will not find a job with an error later, so the error will be only in vdsm log.

I think this is ok, since this is a double failure and we are not trying to avoid that, and failing early simplify the code and prevents later errors.
Line 63:         self.vol_id = _get_required(params, 'vol_id')
Line 64:         self.size = _get_required(params, 'size')
Line 65:         vol_types = [volume.VOLUME_TYPES[vt]
Line 66:                      for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]


https://gerrit.ovirt.org/#/c/50221/3/vdsm/storage/sdm/api/sdm_job.py
File vdsm/storage/sdm/api/sdm_job.py:

Line 30: 
Line 31: class SdmJob(jobs.Job):
Line 32:     log = logging.getLogger('storage.sdmjob')
Line 33: 
Line 34:     def __init__(self, job_id, desc, host_id):
Document what subclass must do here - validate parameters?
Line 35:         super(SdmJob, self).__init__(job_id, desc)
Line 36:         self.host_id = host_id
Line 37: 
Line 38:     def run(self):


Line 33: 
Line 34:     def __init__(self, job_id, desc, host_id):
Line 35:         super(SdmJob, self).__init__(job_id, desc)
Line 36:         self.host_id = host_id
Line 37: 
Add the a _run method documenting what subclass must implement (taking domain locks, taking resource manager locks, etc.)

    def _run(self):
        raise NotImplementedError("Must be implemented by a subclass")
Line 38:     def run(self):
Line 39:         vars.job_id = self.id
Line 40:         try:
Line 41:             self._run()


Line 49:                                self.id, self.description)
Line 50:             self._error = utils.GeneralException(str(e))
Line 51:             self._status = jobs.STATUS.FAILED
Line 52:         else:
Line 53:             self._status = jobs.STATUS.DONE
Add finally to make it clear that this must be cleared


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list