Change in vdsm[master]: core: Unlink image run directory when deleting a snapshot

Ala Hino has uploaded a new change for review.

Change subject: core: Unlink image run directory when deleting a snapshot
......................................................................

core: Unlink image run directory when deleting a snapshot

Unlink image run directory,
/run/vdsm/storage/sdUUID/imgUUID/volUUID, when removing a
snapshot.

Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Bug-Url: https://bugzilla.redhat.com/1321018
Signed-off-by: Ala Hino <ahino(a)redhat.com&gt;
---
M vdsm/storage/blockVolume.py
1 file changed, 11 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/59725/1

diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index 4476a9e..080a858 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -604,12 +604,22 @@
         try:
             self.log.debug("Unlinking %s", vol_path)
             os.unlink(vol_path)
-            return True
         except Exception as e:
             eFound = e
             self.log.error("cannot delete volume's %s/%s link path: %s",
                            self.sdUUID, self.volUUID, vol_path, exc_info=True)
 
+        try:
+            imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
+                                     self.imgUUID, self.volUUID)
+            self.log.debug("Unlinking %s", imgRundir)
+            os.unlink(imgRundir)
+            return True
+        except Exception as e:
+            eFound = e
+            self.log.error("cannot delete volume's %s/%s link path: %s",
+                           self.sdUUID, self.volUUID, imgRundir, exc_info=True)
+
         raise eFound
 
     def extend(self, newSize):


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: core: Unlink image run directory when deleting a snapshot
......................................................................


Patch Set 1:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Unlink image run directory when deleting a snapshot
......................................................................


Patch Set 1:

(5 comments)

https://gerrit.ovirt.org/#/c/59725/1/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 604
Line 605
Line 606
Line 607
Line 608
This code is bad, don't copy it. We will not fix it in this patch but we should not
duplicate bad code.


Line 610:                            self.sdUUID, self.volUUID, vol_path, exc_info=True)
Line 611: 
Line 612:         try:
Line 613:             imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
Line 614:                                      self.imgUUID, self.volUUID)
This is the volume run path, not the image directory
Line 615:             self.log.debug("Unlinking %s", imgRundir)
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:


Line 611: 
Line 612:         try:
Line 613:             imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
Line 614:                                      self.imgUUID, self.volUUID)
Line 615:             self.log.debug("Unlinking %s", imgRundir)
Use %r
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
Line 619:             eFound = e


Line 614:                                      self.imgUUID, self.volUUID)
Line 615:             self.log.debug("Unlinking %s", imgRundir)
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
This will log errors when the run link path does not exists - very bad. Should log debug
message in this case. Use separate handler for OSError and errno == ENOENT
Line 619:             eFound = e
Line 620:             self.log.error("cannot delete volume's %s/%s link path:
%s",
Line 621:                            self.sdUUID, self.volUUID, imgRundir, exc_info=True)
Line 622: 


Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
Line 619:             eFound = e
Line 620:             self.log.error("cannot delete volume's %s/%s link path:
%s",
This is the volume run link
Line 621:                            self.sdUUID, self.volUUID, imgRundir, exc_info=True)
Line 622: 
Line 623:         raise eFound
Line 624: 


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Unlink volume run path when deleting a snapshot

gerrit-hooks has posted comments on this change.

Change subject: core: Unlink volume run path when deleting a snapshot
......................................................................


Patch Set 2:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Unlink volume run path when deleting a snapshot

Ala Hino has posted comments on this change.

Change subject: core: Unlink volume run path when deleting a snapshot
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/59725/1/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 610:                            self.sdUUID, self.volUUID, vol_path, exc_info=True)
Line 611: 
Line 612:         try:
Line 613:             imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
Line 614:                                      self.imgUUID, self.volUUID)
>  This is the volume run path, not the image directory 
Done
Line 615:             self.log.debug("Unlinking %s", imgRundir)
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:


Line 611: 
Line 612:         try:
Line 613:             imgRundir = os.path.join(constants.P_VDSM_STORAGE, self.sdUUID,
Line 614:                                      self.imgUUID, self.volUUID)
Line 615:             self.log.debug("Unlinking %s", imgRundir)
>  Use %r 
Done
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
Line 619:             eFound = e


Line 614:                                      self.imgUUID, self.volUUID)
Line 615:             self.log.debug("Unlinking %s", imgRundir)
Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
>  This will log errors when the run link path does not exists - very
> bad. Sho 
Will do this: check if os.path.islink and then unlink it
Line 619:             eFound = e
Line 620:             self.log.error("cannot delete volume's %s/%s link path:
%s",
Line 621:                            self.sdUUID, self.volUUID, imgRundir, exc_info=True)
Line 622: 


Line 616:             os.unlink(imgRundir)
Line 617:             return True
Line 618:         except Exception as e:
Line 619:             eFound = e
Line 620:             self.log.error("cannot delete volume's %s/%s link path:
%s",
>  This is the volume run link 
Done
Line 621:                            self.sdUUID, self.volUUID, imgRundir, exc_info=True)
Line 622: 
Line 623:         raise eFound
Line 624: 


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: core: Unlink volume run path when deleting a snapshot

gerrit-hooks has posted comments on this change.

Change subject: core: Unlink volume run path when deleting a snapshot
......................................................................


Patch Set 3:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: core: Unlink volume run path when deleting a snapshot

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: core: Unlink volume run path when deleting a snapshot
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/59725/3/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 608
Line 609
Line 610
Line 611
Line 612
This seems to be the wrong place to delete the volume link, since it is not created by
this class.

Can you explain where this link is created? It should be deleted by the same class that
created it.


Line 614:                                       self.imgUUID, self.volUUID)
Line 615:             if os.path.islink(volRunPath):
Line 616:                 self.log.debug("Unlinking %r", volRunPath)
Line 617:                 os.unlink(volRunPath)
Line 618:                 return True
This will cause the operation to fail if the link does not exist.
Line 619:         except Exception as e:
Line 620:             eFound = e
Line 621:             self.log.debug("cannot delete volume's %s/%s run link:
%r",
Line 622:                            self.sdUUID, self.volUUID, volRunPath,


-- 
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
......................................................................


Patch Set 4:

* #1377849::Update tracker: OK
* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1377849::OK, public bug
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1377849::OK, Correct classification oVirt
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
......................................................................


Patch Set 4: Verified+1

Need to verify wipe after delete logic when deactivating the LV before deleting the
volume

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
......................................................................


Patch Set 4: Code-Review-1

Did we agree that this is not the right approach?

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: teardown volume on HSM after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
......................................................................


Patch Set 4: Verified-1

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

gerrit-hooks has posted comments on this change.

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


Patch Set 5:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

gerrit-hooks has posted comments on this change.

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


Patch Set 6:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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 <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

Ala Hino has posted comments on this change.

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


Patch Set 6: Verified+1

-- 
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 <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

[Nir Soffer]

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 <ahino(a)redhat.com&gt;
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 <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

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


Patch Set 6:

(1 comment)

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

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.
Please also explain here why this is not needed in file storage.
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Line 12: Bug-Url: https://bugzilla.redhat.com/1321018


-- 
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 <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Unlink volume runtime dir after live merge

gerrit-hooks has posted comments on this change.

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


Patch Set 7:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 6:

(9 comments)

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 <ahino(a)redhat.com&gt;
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? 
Done
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.
Done
Line 10: 
Line 11: Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Line 12: Bug-Url: https://bugzilla.redhat.com/1321018


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 also explain here why this is not needed in file storage.
Done
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. 
Done
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 
Done
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 
Done
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. 
Done
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, 
Done
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. 
Done
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 <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 8:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 9:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

https://gerrit.ovirt.org/#/c/59725/9/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

PS9, Line 829: self.log.error("Failed to unlink volume run link: %r",
             :                                volRunlink)
I'd just raise without logging


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/8/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821:         """
Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
Read again the comments in version 6 (your replied Done, but ignored the comments).
Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 self.log.error("Failed to unlink volume run link:
%r",


https://gerrit.ovirt.org/#/c/59725/9/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 self.log.error("Failed to unlink volume run link:
%r",
Line 830:                                volRunlink)
>  I'd just raise without logging 
I commented about this in
https://gerrit.ovirt.org/#/c/59725/6/vdsm/storage/blockSD.py

Seems like leftover from older version.
Line 831:                 raise
Line 832:             self.log.debug("Volume run link %r does not exist",
volRunlink)
Line 833: 
Line 834: 


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 10:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/9/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

PS9, Line 829: self.log.error("Failed to unlink volume run link: %r",
             :                                volRunlink)
>  I'd just raise without logging 
Done


Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 self.log.error("Failed to unlink volume run link:
%r",
Line 830:                                volRunlink)
>  I commented about this in 
Done
Line 831:                 raise
Line 832:             self.log.debug("Volume run link %r does not exist",
volRunlink)
Line 833: 
Line 834: 


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 10: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/59725/10/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821:         """
Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
./vdsm/storage/blockSD.py:825: undefined name 'volRunLink'

This is the result of using mixedCase names in Python. In Java this works since the
compiler detect this errors, in Python using this style is a recipe for failure.

To avoid this, lets use only lowercase_with_underscores names for all local variables.

Unfortunately, for method names and arguments we should match the module style.

Also, you ignored one of my comment about the log above in previous version, please
address it.
Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 raise


Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
Line 826:             os.unlink(volRunLink)
Same error here.
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 raise
Line 830:             self.log.debug("Volume run link %r does not exist",
volRunlink)


-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 10:

(2 comments)

https://gerrit.ovirt.org/#/c/59725/10/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821:         """
Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
>  ./vdsm/storage/blockSD.py:825: undefined name 'volRunLink'
Which comment? I thought I addressed all comments
Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 raise


Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
Line 826:             os.unlink(volRunLink)
>  Same error here. 
Done
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 raise
Line 830:             self.log.debug("Volume run link %r does not exist",
volRunlink)


-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 11:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/59725/10/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 821:         """
Line 822:         volRunlink = os.path.join(constants.P_VDSM_STORAGE,
Line 823:                                   self.sdUUID, imgUUID, volUUID)
Line 824:         try:
Line 825:             self.log.info("Unlinking volme runtime link: %r",
volRunLink)
>  Which comment? I thought I addressed all comments 
The comment
about this log line.
Line 826:             os.unlink(volRunLink)
Line 827:         except OSError as e:
Line 828:             if e.error != errno.ENOENT:
Line 829:                 raise


-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 12:

* #1321018::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

Ala Hino has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 12: Verified+1

-- 
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 13:

* update_tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 13: Code-Review+1

Waiting for more reviews.

-- 
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

Adam Litke has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 13: Code-Review+2

-- 
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 14: Continuous-Integration+1

-- 
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Nir Soffer]

Nir Soffer has submitted this change and it was merged.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Live Merge: Remove volume run link after live merge

When deleting a volume while the VM is running, volume teardown doesn't
remove the volume run symbolic link: /run/vdsm/storage/sdUUID/volUUID.
In patch Iec3b6a (Live Merge: teardown volume on HSM after live merge)
we added volume teardown logic that, for block storage it deactivated
the volume. In this patch we extend volume teardown logic to unlink
volume run link.
Note that this change isn't required for file storage as no symbolic
links are created.

Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Bug-Url: https://bugzilla.redhat.com/1321018
Signed-off-by: Ala Hino <ahino(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/59725
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-by: Adam Litke <alitke(a)redhat.com&gt;
Continuous-Integration: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M vdsm/storage/blockSD.py
1 file changed, 15 insertions(+), 0 deletions(-)

Approvals:
  Adam Litke: Looks good to me, approved
  Nir Soffer: Looks good to me, but someone else must approve; Passed CI tests
  Ala Hino: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib88bf92e702ac6c324b87c9459b01adf165eaca4
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 14:

* #1321018::Update tracker: OK
* #59725::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1321018::OK, public bug
* Check Product::#1321018::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

[Jenkins CI RO]

Jenkins CI has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 15:

Propagate review hook: Continuous Integration value inherited from patch 14

-- 
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: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Live Merge: Remove volume run link after live merge

gerrit-hooks has posted comments on this change.

Change subject: Live Merge: Remove volume run link after live merge
......................................................................


Patch Set 15:

* #1321018::Update tracker: OK
* #59725::Update tracker: OK
* Set MODIFIED::bug 1321018::::#1321018::IGNORE, skipping for branch 'master'

-- 
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: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No