Nir Soffer has posted comments on this change.

Change subject: Live Merge: Unlink volume runtime dir after live merge
......................................................................


Patch Set 6:

(8 comments)

Did you verify that we don't leave stale symlinks on file storage?

https://gerrit.ovirt.org/#/c/59725/6//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-22 16:41:56 +0300
Line 4: Commit:     Ala Hino <ah...@redhat.com>
Line 5: CommitDate: 2016-09-22 17:01:55 +0300
Line 6: 
Line 7: Live Merge: Unlink volume runtime dir after live merge
Remove volume run link after live merge?
Line 8: 
Line 9: Unlink volume runtime dir after live megre.
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4


Line 5: CommitDate: 2016-09-22 17:01:55 +0300
Line 6: 
Line 7: Live Merge: Unlink volume runtime dir after live merge
Line 8: 
Line 9: Unlink volume runtime dir after live megre.
Please explain the context like you did in the previous patch.

Note that we are removing a symlink, not a directory.
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Line 12: Bug-Url: https://bugzilla.redhat.com/1321018


https://gerrit.ovirt.org/#/c/59725/6/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, imgUUID, volUUID):
This change belongs to the previous patch.
Line 815:         lvm.deactivateLVs(self.sdUUID, [volUUID])
Line 816:         self.removeVolumeRunDir(imgUUID, volUUID)
Line 817: 
Line 818:     def removeVolumeRunDir(self, imgUUID, volUUID):


Line 814:     def teardownVolume(self, imgUUID, volUUID):
Line 815:         lvm.deactivateLVs(self.sdUUID, [volUUID])
Line 816:         self.removeVolumeRunDir(imgUUID, volUUID)
Line 817: 
Line 818:     def removeVolumeRunDir(self, imgUUID, volUUID):
removeVolumeRunLink
Line 819:         """
Line 820:         Remove /run/vdsm/storage/sdUUID/imgUUID/volUUID
Line 821:         """
Line 822:         sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)


Line 820:         Remove /run/vdsm/storage/sdUUID/imgUUID/volUUID
Line 821:         """
Line 822:         sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)
Line 823:         imgRunDir = os.path.join(sdRunDir, imgUUID)
Line 824:         volRunDir = os.path.join(imgRunDir, volUUID)
volSymlink, this is not a directory

We don't need 3 calls to os.path.join - use:

    volRunlink = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID, imgUUID, 
volUUID)
Line 825:         try:
Line 826:             self.log.info("Unlinking volme runtime dir: %s", 
volRunDir)
Line 827:             os.unlink(volRunDir)
Line 828:         except OSError as e:


Line 822:         sdRunDir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID)
Line 823:         imgRunDir = os.path.join(sdRunDir, imgUUID)
Line 824:         volRunDir = os.path.join(imgRunDir, volUUID)
Line 825:         try:
Line 826:             self.log.info("Unlinking volme runtime dir: %s", 
volRunDir)
The log cannot raise OSError, it should always be out of the try block.

Please check the other file system related logs and use the same format.
Line 827:             os.unlink(volRunDir)
Line 828:         except OSError as e:
Line 829:             if e.error == errno.ENOENT:
Line 830:                 self.log.debug("Link doesn't exist")


Line 830:                 self.log.debug("Link doesn't exist")
Line 831:             else:
Line 832:                 self.log.error("Failed to unlink vol runtime dir: %s",
Line 833:                                volRunDir)
Line 834:                 raise
Logging and raising is bad practice. If you cannot handle the error, raise, and 
let someone else handle it and log about it.

Also we don't have here two equal cases (if, else). We have:

- Can we handle this error? if not raise and let someone else handle it. We 
want to do this first.
- Handle missing link by logging about it

So this is the code that reveal this intent best:

    if e.error != errno.ENOENT:
        raise
    self.log.debug("Volume run link %r does not exists", volRunlink)
Line 835: 
Line 836: 
Line 837: class BlockStorageDomain(sd.StorageDomain):
Line 838:     manifestClass = BlockStorageDomainManifest


https://gerrit.ovirt.org/#/c/59725/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

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
Line 4785:         self.teardownVolume(self.drive.imageID, 
self.job['topVolume'])
All these changes belong to the previous patch.
Line 4786: 
Line 4787:     def isSuccessful(self):
Line 4788:         """
Line 4789:         Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/59725
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 6
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

Reply via email to