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

alitke at redhat.com alitke at redhat.com
Wed Dec 9 22:01:31 UTC 2015


Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume_container job
......................................................................


Patch Set 1:

(12 comments)

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?
sure, I can change that.


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 was hoping the verbs could stand out in the file list.  What sort of naming convention should we adopt for verb modules? verbCreateVolumeContainer.py ?
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.
You need this if you want to use the dist_*_PYTHON rule below.
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?
This rule adds the listed files to the distribution and also compiles the .c and .o files during the build.  We do this in other parts of the tree too.  It's automake boilerplate basically.
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
ok
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.
Done
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 cou
How would this look in practice?  namedtuples? classes?  bare dicts?
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?
yeah, but not sure how other jobs might want to do this.  It doesn't hurt to move it though so I will.
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
ok.  Will add a precursor patch.
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?
I think so.  hsm will only instantiate the object and schedule it.
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 
We don't necessarily need to split them out, but they are nice logical state transitions so I'd like to hold on to the separation at least for now.
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 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 
What is the generic type of error if it is in jobs.Job?  Here I want it to be a StorageException.
Line 42:         return sdm_info
Line 43: 
Line 44:     def run(self):
Line 45:         try:


-- 
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