Nir Soffer has posted comments on this change. Change subject: Live Merge: teardown volume on HSM after live merge ......................................................................
Patch Set 1: (8 comments) Nice! 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. 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 image id to this function in this patch (you added it in the next patch). 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: Called when a volume is detached from a prepared image during live merge flow. In this case the volume will not be teardown when the image is tear down. This does nothing, subclass should override this if needed. 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. 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: from storage import sdc 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_case: def teardown_top_volume(self): The flow in _run() should read like documentation. Add TODO to move this to storage public api. This is a layering violation, virt code should not call storage code except via public storage apis (e.g. irs.getVolumeSize()) 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. Get volume and image id here, from self.job. 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 you need more documentation - why we must do this, add a docstring to the function. 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
