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