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

nsoffer at redhat.com nsoffer at redhat.com
Tue Dec 22 00:50:33 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 8: Code-Review-1

(13 comments)

Partial review

https://gerrit.ovirt.org/#/c/50221/8/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3688:                             their meaning.
Line 3689:         """
Line 3690:         return {'domains': self.domainMonitor.getHostStatus(domains)}
Line 3691: 
Line 3692:     def sdm_schedule(self, name, job):
No need for name parameter, use the job.id or add job.name.
Line 3693:         """
Line 3694:         SDM jobs currently run using the old TaskManager thread pool but none
Line 3695:         of the other old Task features (ie. rollbacks, persistence) are
Line 3696:         supported.  SDM tasks are managed using the Host Jobs API in jobs.py.


Line 3695:         of the other old Task features (ie. rollbacks, persistence) are
Line 3696:         supported.  SDM tasks are managed using the Host Jobs API in jobs.py.
Line 3697:         """
Line 3698:         def start_job(*args, **kwargs):
Line 3699:             pass
Leftover?
Line 3700: 
Line 3701:         jobs.add(job)
Line 3702:         self.taskMng.scheduleJob("sdm", None, vars.task, name, job.run)
Line 3703: 


Line 3701:         jobs.add(job)
Line 3702:         self.taskMng.scheduleJob("sdm", None, vars.task, name, job.run)
Line 3703: 
Line 3704:     @public
Line 3705:     def sdm_create_volume(self, job_id, vol_info_params):
We don't need _params - we get a vol_info dict, and wrap it in CreateVolumeInfo, replacing vol_info so we cannot touch it.

    vol_info = Wapper(vol_info)

This is good pattern when you do this in the start of the function.

In the future, the bridge will wrap the vol_info dict for us, based on the schema type.
Line 3706:         vol_info = sdm.api.create_volume.CreateVolumeInfo(vol_info_params)
Line 3707:         dom_manifest = sdCache.produce(vol_info.sd_id).manifest
Line 3708:         host_id = self.domainMonitor.getHostId(vol_info.sd_id)
Line 3709:         job = sdm.api.create_volume.Job(job_id, host_id, dom_manifest,


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

Line 31: rmanager = rm.ResourceManager.getInstance()
Line 32: 
Line 33: 
Line 34: class Job(sdm_job.SdmJob):
Line 35:     def __init__(self, job_id, host_id, dom_manifest, vol_info):
Lets use "sd" instead of "dom".
Line 36:         super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 37:         self.dom_manifest = dom_manifest
Line 38:         self.vol_info = vol_info
Line 39: 


Line 37:         self.dom_manifest = dom_manifest
Line 38:         self.vol_info = vol_info
Line 39: 
Line 40:     def _run(self):
Line 41:         vol_format = volume.name2type(self.vol_info.vol_format)
vol_info.vol_format can be the format enum - the api must use only strings, the code can use enums. If we use this system, we don't have to do this kind of translation by hand.
Line 42:         self.dom_manifest.validateCreateVolumeParams(
Line 43:             vol_format, self.vol_info.parent_vol_id)
Line 44: 
Line 45:         with self.dom_manifest.domain_lock(self.host_id):


Line 54:                 try:
Line 55:                     artifacts.create(
Line 56:                         self.vol_info.virtual_size, vol_format,
Line 57:                         self.vol_info.disk_type, self.vol_info.description,
Line 58:                         self.vol_info.parent_vol_id)
Too many parameter, can we pass vol_info as is?
Line 59:                     artifacts.commit()
Line 60:                 except (volume_artifacts.CannotCreateVolumeArtifacts,
Line 61:                         volume_artifacts.VolumeArtifactsNotFound):
Line 62:                     self.log.exception("Error creating volume artifacts")


Line 59:                     artifacts.commit()
Line 60:                 except (volume_artifacts.CannotCreateVolumeArtifacts,
Line 61:                         volume_artifacts.VolumeArtifactsNotFound):
Line 62:                     self.log.exception("Error creating volume artifacts")
Line 63:                     raise se.VolumeCreationError()
This error translation is bad, and hiding the original error. The concept of raising specific error in vdsm for each verb is usless, and we should stop it now.

We should have set of common errors, and raise any of them from any verb, based the error (e.g OutOfSpace error is good for create volume or snapshot or most operations).

Engine has all the context to show translated error about creating a volume, it does not need CreateVolumeError from vdsm. If can use more specific errors from vdsm like OutOfSpace or PermisionDenied, or NoSuchFile to show useful error message.

For example:

    Cannot create volume "Foo"

    There is no available space on storage domain "Bar".

The first part of the error is based on what engine knows. The second part of what vdsm knows.

We should not do any translation, just fail in any level you like with the proper error and let the job toplevel error handler catch the error.

We also do not need log anything, the top level error handler can log the entire traceback if needed.

Just remove the try: except: block, and raise public errors from volume_artifacts.
Line 64: 
Line 65: 
Line 66: # TODO: Adopt the properties framework for managing complex verb parameters
Line 67: 


Line 72:         self.img_id = _get_required(params, 'img_id')
Line 73:         self.vol_id = _get_required(params, 'vol_id')
Line 74:         self.virtual_size = _get_required(params, 'virtual_size')
Line 75:         vol_types = [volume.VOLUME_TYPES[vt]
Line 76:                      for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]
Move to constants or to volume.py
Line 77:         self.vol_format = _get_enum(params, 'vol_format', vol_types)
Line 78:         self.disk_type = _get_enum(params, 'disk_type',
Line 79:                                    image.DISK_TYPES.values())
Line 80:         self.description = _get_param_default(params, 'description', '')


Line 73:         self.vol_id = _get_required(params, 'vol_id')
Line 74:         self.virtual_size = _get_required(params, 'virtual_size')
Line 75:         vol_types = [volume.VOLUME_TYPES[vt]
Line 76:                      for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]
Line 77:         self.vol_format = _get_enum(params, 'vol_format', vol_types)
This must a string in new code - no more magic integers in the api, but the object we keep can be the constant.
Line 78:         self.disk_type = _get_enum(params, 'disk_type',
Line 79:                                    image.DISK_TYPES.values())
Line 80:         self.description = _get_param_default(params, 'description', '')
Line 81:         self.parent_img_id = _get_param_default(params, 'parent_img_id',


Line 75:         vol_types = [volume.VOLUME_TYPES[vt]
Line 76:                      for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]
Line 77:         self.vol_format = _get_enum(params, 'vol_format', vol_types)
Line 78:         self.disk_type = _get_enum(params, 'disk_type',
Line 79:                                    image.DISK_TYPES.values())
This must a string in new code - no more magic integers in the api, but the object we keep can be the constant.
Line 80:         self.description = _get_param_default(params, 'description', '')
Line 81:         self.parent_img_id = _get_param_default(params, 'parent_img_id',
Line 82:                                                 volume.BLANK_UUID)
Line 83:         self.parent_vol_id = _get_param_default(params, 'parent_vol_id',


Line 86: 
Line 87: 
Line 88: def _get_required(params, name):
Line 89:     if name not in params:
Line 90:         raise ValueError("Missing required property %r" % name)
This should be api error such as InvalidParameterException
Line 91:     return params[name]
Line 92: 
Line 93: 
Line 94: def _get_enum(params, name, values):


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

Line 38:         self.host_id = host_id
Line 39: 
Line 40:     def run(self):
Line 41:         vars.job_id = self.id
Line 42:         self._status = jobs.STATUS.RUNNING
Move up so vars.job_id = self.id comes before the try:
Line 43:         try:
Line 44:             self._run()
Line 45:         except se.StorageException as e:
Line 46:             self.log.exception("Job (id:%s desc:%s) failed",


Line 52:                                self.id, self.description)
Line 53:             self._error = utils.GeneralException(str(e))
Line 54:             self._status = jobs.STATUS.FAILED
Line 55:         else:
Line 56:             self._status = jobs.STATUS.DONE
Add finally: to ensure that we don't leave stale job_id in vars.


-- 
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list