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