Change in vdsm[master]: sdm: Add create_volume_container job
nsoffer at redhat.com
nsoffer at redhat.com
Wed Dec 9 21:13:16 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: sdm: Add create_volume_container job
......................................................................
Patch Set 1: Code-Review-1
(25 comments)
I like it
https://gerrit.ovirt.org/#/c/50221/1/tests/sdm_verbs_test.py
File tests/sdm_verbs_test.py:
Line 38: self.sdUUID = sd_id
Line 39:
Line 40: def validateCreateVolumeParams(self, *args):
Line 41: pass
Line 42:
Use @recorded and validate that the job acquired and released the lock?
Line 43: def acquireDomainLock(self, *args):
Line 44: pass
Line 45:
Line 46: def releaseDomainLock(self, *args):
Line 53: class FakeVolumeArtifacts(object):
Line 54: def __init__(self, img_id, vol_id):
Line 55: self.img_id = img_id
Line 56: self.vol_id = vol_id
Line 57:
Use @recorded and validate the the job created and committed?
Line 58: def create(self, *args):
Line 59: pass
Line 60:
Line 61: def commit(self):
Line 73: break
Line 74:
Line 75:
Line 76: @expandPermutations
Line 77: class CreateVolumeContainerTests(SdmVerbTests):
Lets have separate tests for each verb, and put common helpers in sdmtestlib.py
Line 78:
Line 79: def _get_args(self):
Line 80: dom_manifest = FakeDomainManifest(str(uuid.uuid4()))
Line 81: return dict(dom_manifest=dom_manifest, job_id=str(uuid.uuid4()),
https://gerrit.ovirt.org/#/c/50221/1/tests/storagefakelib.py
File tests/storagefakelib.py:
Line 218: return ''.join(random.choice(chars) for _ in range(size))
Line 219: return '-'.join(part(size) for size in [6, 4, 4, 4, 4, 6])
Line 220:
Line 221:
Line 222: class FakeResourceManager(object):
Nice! - lets also @record operation so we can validate the locking?
Not sure it works with @contextmanager, maybe we need to validate locking using custome recording code.
Line 223: @contextmanager
Line 224: def acquireResource(self, namespace, name, lockType, timeout=None):
Line 225: yield
Line 226:
https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/Makefile.am
File vdsm/storage/sdm/Makefile.am:
Line 22
Line 23
Line 24
Line 25
Line 26
Maybe volume_artifacts.py?
It is very hard to read suchlongnames.
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20:
Line 21: SUBDIRS = jobs
Do we really need the jobs tree?
I think we should start with flat list of verb modules inside sdm.
Line 22:
Line 23: include $(top_srcdir)/build-aux/Makefile.subs
Line 24:
Line 25: vdsmsdmdir = $(vdsmdir)/storage/sdm
https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/jobs/Makefile.am
File vdsm/storage/sdm/jobs/Makefile.am:
Line 19: #
Line 20:
Line 21: include $(top_srcdir)/build-aux/Makefile.subs
Line 22:
Line 23: vdsmsdmjobsdir = $(vdsmdir)/storage/sdm/jobs
I don't think we need this.
Line 24:
Line 25: dist_vdsmsdmjobs_PYTHON = \
Line 26: createVolumeContainer.py \
Line 27: $(NULL)
Line 21: include $(top_srcdir)/build-aux/Makefile.subs
Line 22:
Line 23: vdsmsdmjobsdir = $(vdsmdir)/storage/sdm/jobs
Line 24:
Line 25: dist_vdsmsdmjobs_PYTHON = \
Do we need these long names?
I don't see any reason why the list of modules would not be:
modules = ...
Maybe I'm missing something about the current build system?
Line 26: createVolumeContainer.py \
Line 27: $(NULL)
Line 22:
Line 23: vdsmsdmjobsdir = $(vdsmdir)/storage/sdm/jobs
Line 24:
Line 25: dist_vdsmsdmjobs_PYTHON = \
Line 26: createVolumeContainer.py \
create_volume_container.py
Line 27: $(NULL)
https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/jobs/createVolumeContainer.py
File vdsm/storage/sdm/jobs/createVolumeContainer.py:
Line 29: rmanager = rm.ResourceManager.getInstance()
Line 30:
Line 31:
Line 32: class CreateVolumeContainer(sdmJob.SdmJob):
Line 33: def __init__(self, dom_manifest, job_id, img_id, vol_id, size, vol_format,
job_id should be first, it is the general job interface.
Line 34: disk_type, desc, parent_img_id, parent_vol_id, initial_size):
Line 35: super(CreateVolumeContainer, self).__init__(job_id,
Line 36: 'CreateVolumeContainer')
Line 37: self.dom_manifest = dom_manifest
Line 30:
Line 31:
Line 32: class CreateVolumeContainer(sdmJob.SdmJob):
Line 33: def __init__(self, dom_manifest, job_id, img_id, vol_id, size, vol_format,
Line 34: disk_type, desc, parent_img_id, parent_vol_id, initial_size):
To many arguments. We should pack the job arguments into a dict or into couple of complex argument, like:
- job_id
- dom_manifest
- vol_info (img_id, vol_id, size, format, disk_type, desc, initial_size)
- parent_vol (img_id, vol_id)
Same for the parameters of the createVolumeContainer verb - we should not duplicate the current messy api.
Line 35: super(CreateVolumeContainer, self).__init__(job_id,
Line 36: 'CreateVolumeContainer')
Line 37: self.dom_manifest = dom_manifest
Line 38: self.img_id = img_id
Line 47: self._progress = 0
Line 48:
Line 49: @property
Line 50: def progress(self):
Line 51: return self._progress
This should be in SdmJob - we want progress for all sdm jobs, right?
Line 52:
Line 53: def _run(self):
Line 54: self.dom_manifest.validateCreateVolumeParams(self.vol_format,
Line 55: self.parent_vol_id)
Line 53: def _run(self):
Line 54: self.dom_manifest.validateCreateVolumeParams(self.vol_format,
Line 55: self.parent_vol_id)
Line 56: host_id = self.get_domain_host_id(self.dom_manifest.sdUUID)
Line 57: self.dom_manifest.acquireDomainLock(host_id)
We need context manager for this, we are going to repeat this for every sdm verb.
Line 58:
Line 59: try:
Line 60: image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 61: IMAGE_NAMESPACE)
Line 59: try:
Line 60: image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 61: IMAGE_NAMESPACE)
Line 62: with rmanager.acquireResource(image_res_ns, self.img_id,
Line 63: rm.LockType.exclusive):
Are we going to have all locking inside SdmJob._run, instead of hsm?
Line 64:
Line 65: artifacts = self._get_artifacts_instance()
Line 66: artifacts.create(self.size, self.vol_format, self.disk_type,
Line 67: self.desc, self.parent_vol_id)
Line 65: artifacts = self._get_artifacts_instance()
Line 66: artifacts.create(self.size, self.vol_format, self.disk_type,
Line 67: self.desc, self.parent_vol_id)
Line 68: self._progress = 50
Line 69: artifacts.commit()
I wonder why we need seprate create and commit methods. Do you plan to use it in future verbs?
Line 70: self._progress = 100
Line 71: finally:
https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/jobs/sdmJob.py
File vdsm/storage/sdm/jobs/sdmJob.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):
We need a host_id parameter to so we can take locks on the domain - probably in the concrete jobs since some jobs will need both source and destination host ids (host id may be different on each domain in the future).
Line 35: self._default_error = se.StorageException
Line 36: self._error = None
Line 37: super(SdmJob, self).__init__(job_id, desc)
Line 38:
Line 31: class SdmJob(jobs.Job):
Line 32: log = logging.getLogger('Storage.SdmJob')
Line 33:
Line 34: def __init__(self, job_id, desc):
Line 35: self._default_error = se.StorageException
We don't need default error. Code that want to fail with specific error should raise it, otherwise you will fail with GeneralError.
Line 36: self._error = None
Line 37: super(SdmJob, self).__init__(job_id, desc)
Line 38:
Line 39: def info(self):
Line 32: log = logging.getLogger('Storage.SdmJob')
Line 33:
Line 34: def __init__(self, job_id, desc):
Line 35: self._default_error = se.StorageException
Line 36: self._error = None
Move up to Job.
Finally, remove __init__, because it is empty now :-)
Line 37: super(SdmJob, self).__init__(job_id, desc)
Line 38:
Line 39: def info(self):
Line 40: sdm_info = super(SdmJob, self).info()
Line 37: super(SdmJob, self).__init__(job_id, desc)
Line 38:
Line 39: def info(self):
Line 40: sdm_info = super(SdmJob, self).info()
Line 41: sdm_info['extra_info'] = {'error': self._error}
Better add optional "error" to jobs.Job class, there is nothing related to storage or sdm about it.
So we also do not need info() :-)
Line 42: return sdm_info
Line 43:
Line 44: def run(self):
Line 45: try:
Line 40: sdm_info = super(SdmJob, self).info()
Line 41: sdm_info['extra_info'] = {'error': self._error}
Line 42: return sdm_info
Line 43:
Line 44: def run(self):
Nice! - this replace the dispatcher protection in hsm in a much nicer way.
This is a good place to set vars.job_id, so all the lower layers can access it. This can be useful for logs.
Line 45: try:
Line 46: self._run()
Line 47: except se.StorageException as e:
Line 48: self.log.exception("Job failed")
Line 44: def run(self):
Line 45: try:
Line 46: self._run()
Line 47: except se.StorageException as e:
Line 48: self.log.exception("Job failed")
We need here the job_id and maybe other details about the current job.
Line 49: self._status = jobs.STATUS.FAILED
Line 50: self._error = e
Line 51: except:
Line 52: self.log.exception("Unhandled exception")
Line 50: self._error = e
Line 51: except:
Line 52: self.log.exception("Unhandled exception")
Line 53: self._status = jobs.STATUS.FAILED
Line 54: self._error = self._default_error
I would simplify this to:
except Exception as e:
self._error = GeneralError(str(e))
self._status = jobs.STATUS.FAILED
...
Line 55: else:
Line 56: self._status = jobs.STATUS.DONE
Line 57:
Line 58: @staticmethod
Line 52: self.log.exception("Unhandled exception")
Line 53: self._status = jobs.STATUS.FAILED
Line 54: self._error = self._default_error
Line 55: else:
Line 56: self._status = jobs.STATUS.DONE
Add finally to delete vars.job_id
Line 57:
Line 58: @staticmethod
Line 59: def get_domain_host_id(sd_id):
Line 60: # FIXME: irs._obj is an ugly hack, domainMonitor needs internal locking
Line 57:
Line 58: @staticmethod
Line 59: def get_domain_host_id(sd_id):
Line 60: # FIXME: irs._obj is an ugly hack, domainMonitor needs internal locking
Line 61: return cif.getInstance().irs._obj.domainMonitor.getHostId(sd_id)
We should create Job with a host id and remove this method.
Line 62:
Line 63: def _get_artifacts_instance(self):
Line 64: vol_class = self.dom_manifest.getVolumeClass()
Line 65: if vol_class == blockVolume.BlockVolumeMetadata:
Line 67: elif vol_class == fileVolume.FileVolumeMetadata:
Line 68: artifacts_class = volumeartifacts.FileVolumeArtifacts
Line 69: else:
Line 70: raise ValueError(self.dom_manifest)
Line 71: return artifacts_class(self.dom_manifest, self.img_id, self.vol_id)
This not related to SdmJob, or even CreateVolumeContainer.
DomainManifest should have a volume_artifacts() classmethod, so we can say:
volart = self.dom_manifest.volume_artifacts(img_id, vol_id)
--
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: 1
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