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 <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
