Ala Hino has posted comments on this change. Change subject: Live Merge: teardown volume on HSM after live merge ......................................................................
Patch Set 1: (8 comments) https://gerrit.ovirt.org/#/c/64301/1/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py: Line 810: offset = ((slot + blockVolume.RESERVED_LEASES) * self.logBlkSize * Line 811: sd.LEASE_BLOCKS) Line 812: return clusterlock.Lease(volUUID, self.getLeasesFilePath(), offset) Line 813: Line 814: def teardownVolume(self, volUUID): > Add image uuid, standard api for volume methods. Done Line 815: lvm.deactivateLVs(self.sdUUID, [volUUID]) Line 816: Line 817: Line 818: class BlockStorageDomain(sd.StorageDomain): https://gerrit.ovirt.org/#/c/64301/1/vdsm/storage/sd.py File vdsm/storage/sd.py: Line 524: Line 525: if preallocate is not None and preallocate not in sc.VOL_TYPE: Line 526: raise se.IncorrectType(preallocate) Line 527: Line 528: def teardownVolume(self, volUUID): > Volume is defined by domain id, image id and volume id. Please add the imag Done Line 529: """ Line 530: By default, this method does nothing. It is overriden in blockSD Line 531: to do the necessary cleaning. Line 532: """ Line 527: Line 528: def teardownVolume(self, volUUID): Line 529: """ Line 530: By default, this method does nothing. It is overriden in blockSD Line 531: to do the necessary cleaning. > Replace the text with something like: Done Line 532: """ Line 533: pass Line 534: Line 535: Line 529: """ Line 530: By default, this method does nothing. It is overriden in blockSD Line 531: to do the necessary cleaning. Line 532: """ Line 533: pass > pass is not needed, having a docstring is enough. Done Line 534: Line 535: Line 536: class StorageDomain(object): Line 537: log = logging.getLogger("storage.StorageDomain") https://gerrit.ovirt.org/#/c/64301/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 64: from vdsm.virt.vmpowerdown import VmShutdown, VmReboot Line 65: from vdsm.virt.utils import isVdsmImage, cleanup_guest_socket Line 66: from storage import outOfProcess as oop Line 67: from storage import sd Line 68: from storage.sdc import sdCache > Please avoid this bad practice - this must be: Done Line 69: Line 70: # local imports Line 71: # In future those should be imported via .. Line 72: import caps Line 4763: self.vm._setVolumeSize(self.drive.domainID, self.drive.poolID, Line 4764: self.drive.imageID, baseVolUUID, Line 4765: topVolInfo['capacity']) Line 4766: Line 4767: def teardownVolume(self, volUUID): > This should match other methods like update_base_size - no arguments, lower Done Line 4768: dom_manifest = sdCache.produce_manifest(self.drive.domainID) Line 4769: dom_manifest.teardownVolume(volUUID) Line 4770: Line 4771: @utils.traceback() Line 4764: self.drive.imageID, baseVolUUID, Line 4765: topVolInfo['capacity']) Line 4766: Line 4767: def teardownVolume(self, volUUID): Line 4768: dom_manifest = sdCache.produce_manifest(self.drive.domainID) > We call this sd_manifest elsewhere. Done Line 4769: dom_manifest.teardownVolume(volUUID) Line 4770: Line 4771: @utils.traceback() Line 4772: def run(self): Line 4780: self.vm.enableDriveMonitor() Line 4781: self.success = True Line 4782: self.vm.log.info("Synchronization completed (job %s)", Line 4783: self.job['jobID']) Line 4784: # teardown the merged volume > The comment is not needed, the function name should reveal the intent. If y Done Line 4785: self.teardownVolume(self.job['topVolume']) Line 4786: Line 4787: def isSuccessful(self): Line 4788: """ -- To view, visit https://gerrit.ovirt.org/64301 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@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: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
