Adam Litke has posted comments on this change. Change subject: sdm: Add create_volume job ......................................................................
Patch Set 15: (5 comments) https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/base.py File vdsm/storage/sdm/api/base.py: Line 22: Line 23: import logging Line 24: Line 25: from vdsm import jobs Line 26: from vdsm import exception > This is now in vdsm.storage Nope. Using base exceptions from vdsm only Line 27: Line 28: from storage.threadLocal import vars Line 29: Line 30: Line 34: Line 35: def __init__(self, job_id, desc, host_id): Line 36: super(Job, self).__init__(job_id, desc) Line 37: self._status = jobs.STATUS.PENDING Line 38: self.host_id = host_id > This works now, but we would like to support different id per storage domai Right now the only way to get the host ID is to look up the domainMonitor from the cif singleton and get it from there. For one, that is not clean at all and we are going to be making some changes in that area as part of the sdm pool refactoring later. I prefer to keep this simple for now and we can always drop the host id parameter later if there ends up being a better way to look it up. Line 39: Line 40: def run(self): Line 41: self._status = jobs.STATUS.RUNNING Line 42: vars.job_id = self.id https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/create_volume.py File vdsm/storage/sdm/api/create_volume.py: Line 39: vol_format = volume.name2type(self.vol_info.vol_format) Line 40: self.sd_manifest.validateCreateVolumeParams( Line 41: vol_format, self.vol_info.parent_vol_id) Line 42: Line 43: with self.sd_manifest.domain_lock(self.host_id): > We should make the domain host id accessible to the manifest object, so we As stated in another response, let's leave this change until after the pool refactoring. Line 44: image_res_ns = sd.getNamespace(self.sd_manifest.sdUUID, Line 45: IMAGE_NAMESPACE) Line 46: with rmanager.acquireResource(image_res_ns, self.vol_info.img_id, Line 47: rm.LockType.exclusive): Line 74: Line 75: Line 76: def _required(params, name): Line 77: if name not in params: Line 78: raise ValueError("Missing required property %r" % name) > Why not use se.InvalidParameterException? Because InvalidParameterException is for reporting an invalid value, not a missing value. I can switch to exception.MissingParameter instead. Line 79: return params[name] Line 80: Line 81: Line 82: def _enum(params, name, values): Line 82: def _enum(params, name, values): Line 83: value = _required(params, name) Line 84: if value not in values: Line 85: raise ValueError("Invalid value %r for %r, expecting one of %s" % Line 86: (value, name, values)) > Same Done -- 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: 15 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
