Nir Soffer has posted comments on this change. Change subject: virt-sparsify: add inplace virt-sparsify support ......................................................................
Patch Set 1: (7 comments) This is storage code, should be in the lib/vdsm/storage package, and the entry point should be in hsm.py, like sdm_create_volume. https://gerrit.ovirt.org/#/c/54427/1/lib/vdsm/virtsparsify.py File lib/vdsm/virtsparsify.py: Line 34: _VIRTSPARSIFY = utils.CommandPath("virt-sparsify", Line 35: "/usr/bin/virt-sparsify",) Line 36: Line 37: Line 38: def inplace(sdUUID, spUUID, imgUUID, volUUID, irs): virtsparsify.inplace() is nice, but since this starts a sparcify job, why not: start_sparcify_job() Also, we *must* get the job id here, and jobs.Job must get the a job id as the first parameter. Line 39: """ Line 40: Sparsify the volume in place Line 41: (instead of copying from an input disk to an output disk) Line 42: """ Line 39: """ Line 40: Sparsify the volume in place Line 41: (instead of copying from an input disk to an output disk) Line 42: """ Line 43: job = InplaceJob(volUUID, imgUUID, sdUUID, spUUID, irs) irs should be the second parameter, after the missing job_id Line 44: job.start() Line 45: jobs.add(job) Line 46: Line 47: Line 44: job.start() Line 45: jobs.add(job) Line 46: Line 47: Line 48: def delete_inplace_job(job_id): delete_job Line 49: return jobs.delete(job_id) Line 50: Line 51: Line 52: def abort_inplace_job(job_id): Line 48: def delete_inplace_job(job_id): Line 49: return jobs.delete(job_id) Line 50: Line 51: Line 52: def abort_inplace_job(job_id): abort_job Line 53: return jobs.abort(job_id) Line 54: Line 55: Line 56: @contextmanager Line 56: @contextmanager Line 57: def volume(volUUID, imgUUID, sdUUID, spUUID, irs): Line 58: res = irs.prepareImage(sdUUID, spUUID, imgUUID, volUUID) Line 59: if response.is_error(res): Line 60: raise Exception('Cannot find volume path for %r' % volUUID) Should raise public exception, we cannot raise bare Exception. Line 61: try: Line 62: yield res['path'] Line 63: finally: Line 64: try: Line 61: try: Line 62: yield res['path'] Line 63: finally: Line 64: try: Line 65: irs.teardownImage(sdUUID, spUUID, imgUUID) This never raises, it fail with a failure status code like prepare image. Line 66: except Exception: Line 67: logging.exception('Error tearing down image: %r', imgUUID) Line 68: raise Line 69: Line 64: try: Line 65: irs.teardownImage(sdUUID, spUUID, imgUUID) Line 66: except Exception: Line 67: logging.exception('Error tearing down image: %r', imgUUID) Line 68: raise This seems to repeat code in v2v - maybe we need to more this decorator to common location. Line 69: Line 70: Line 71: class InplaceJob(jobs.Job): Line 72: def __init__(self, volUUID, imgUUID, sdUUID, spUUID, irs): -- To view, visit https://gerrit.ovirt.org/54427 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie0a1269d71816a8ab25c4a00bfb483620cfefde8 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
