Change in vdsm[master]: [wip] volume: make volume._share idempotent

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

[wip] volume: make volume._share idempotent

With this patch it will possible to run the volume._share command
multiple times without harm (idempotent) for the image and for the
running VMs.

Signed-off-by: Federico Simoncelli 
Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileVolume.py
M vdsm/storage/volume.py
3 files changed, 39 insertions(+), 16 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/70/8270/1

diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index 38285c1..ee8ba67 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -324,6 +324,7 @@
         dstPath = os.path.join(dstImgPath, self.volUUID)

self.log.debug("Share volume %s to %s", self.volUUID, dstImgPath)
+        # TODO: make this idempotent
         os.symlink(self.getDevPath(), dstPath)

@classmethod
diff --git a/vdsm/storage/fileVolume.py b/vdsm/storage/fileVolume.py
index 0aa9043..2e76944 100644
--- a/vdsm/storage/fileVolume.py
+++ b/vdsm/storage/fileVolume.py
@@ -227,29 +227,49 @@
         """
         Share this volume to dstImgPath, including the metadata and the lease
         """
-        dstVolPath = os.path.join(dstImgPath, self.volUUID)
-        dstMetaPath = self._getMetaVolumePath(dstVolPath)
-
         self.log.debug("Share volume %s to %s", self.volUUID, dstImgPath)

-        self.oop.fileUtils.safeUnlink(dstVolPath)
-        self.oop.os.link(self.getVolumePath(), dstVolPath)
+        dstVolumePath = os.path.join(dstImgPath, self.volUUID)

-        self.log.debug("Share volume metadata of %s to %s", self.volUUID,
-                       dstImgPath)
+        # Format: itemName, srcPath, dstPath, itemRequired
+        itemsList = [
+            ('volume', self.getVolumePath(), dstVolumePath, True),
+            ('metadata', self._getMetaVolumePath(),
+                         self._getMetaVolumePath(dstVolumePath), True),
+            ('lease', self._getLeaseVolumePath(),
+                      self._getLeaseVolumePath(dstVolumePath),
+                      sdCache.produce(self.sdUUID).hasVolumeLeases()),
+        ]

-        self.oop.fileUtils.safeUnlink(dstMetaPath)
-        self.oop.os.link(self._getMetaVolumePath(), dstMetaPath)
+        for itemName, srcPath, dstPath, itemRequired in itemsList:
+            self.log.debug("Sharing %s %s to %s (required: %s)", itemName,
+                           self.volUUID, dstPath, itemRequired)

-        # Link the lease file if the domain uses sanlock
-        if sdCache.produce(self.sdUUID).hasVolumeLeases():
-            dstLeasePath = self._getLeaseVolumePath(dstVolPath)
+            try:
+                srcInfo = self.oop.stat(srcPath)
+            except OSError, e:
+                if e != os.errno.ENOENT or itemRequired:
+                    # Source not found
+                    self.log.debug("TODO: <message>")
+                    raise

-            self.log.debug("Share volume lease of %s to %s", self.volUUID,
-                           dstImgPath)
+            try:
+                dstInfo = self.oop.stat(dstPath)
+            except OSError, e:
+                if e != os.errno.ENOENT:
+                    raise

-            self.oop.fileUtils.safeUnlink(dstLeasePath)
-            self.oop.os.link(self._getLeaseVolumePath(), dstLeasePath)
+                # Destination file not present, linking
+                self.log.debug("TODO: <message>")
+                self.oop.os.link(srcPath, dstPath)
+            else:
+                # If both the source and destination are present and they are
+                # the same file (already linked) then continue.
+                if srcInfo.st_ino == dstInfo.st_ino:
+                    self.log.debug("TODO: <message>")
+                else:
+                    self.log.debug("TODO: <message>")
+                    raise OSError(os.errno.EEXIST, "Destination file exists")

@classmethod
     def shareVolumeRollback(cls, taskObj, volPath):
diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py
index b9eb356..7938c6f 100644
--- a/vdsm/storage/volume.py
+++ b/vdsm/storage/volume.py
@@ -297,6 +297,8 @@
             self._share(dstImgPath)

except Exception, e:
+            self.log.debug("Cannot share volume %s to %s", self.volUUID,
+                           dstImgPath, exc_info=True)
             raise se.CannotShareVolume(self.getVolumePath(), dstPath, str(e))

def refreshVolume(self):

--
To view, visit http://gerrit.ovirt.org/8270
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/fileVolume.py
Line 237:             ('metadata', self._getMetaVolumePath(),
Line 238:                          self._getMetaVolumePath(dstVolumePath), True),
Line 239:             ('lease', self._getLeaseVolumePath(),
Line 240:                       self._getLeaseVolumePath(dstVolumePath),
Line 241:                       sdCache.produce(self.sdUUID).hasVolumeLeases()),
What I don't like here is that when calling _share during upgrade the domain is still < V3 and therefore the presence of the lease is not enforced. Anyway this should be a minor issue.
Line 242:         ]
Line 243: 
Line 244:         for itemName, srcPath, dstPath, itemRequired in itemsList:
Line 245:             self.log.debug("Sharing %s %s to %s (required: %s)", itemName,

--
To view, visit http://gerrit.ovirt.org/8270
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/fileVolume.py
Line 250:             except OSError, e:
Line 251:                 if e != os.errno.ENOENT or itemRequired:
Line 252:                     # Source not found
Line 253:                     self.log.debug("TODO: <message>")
Line 254:                     raise
else:
     continue
Line 255: 
Line 256:             try:
Line 257:                 dstInfo = self.oop.stat(dstPath)
Line 258:             except OSError, e:

--
To view, visit http://gerrit.ovirt.org/8270
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

Ayal Baron has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/fileVolume.py
Line 237:             ('metadata', self._getMetaVolumePath(),
Line 238:                          self._getMetaVolumePath(dstVolumePath), True),
Line 239:             ('lease', self._getLeaseVolumePath(),
Line 240:                       self._getLeaseVolumePath(dstVolumePath),
Line 241:                       sdCache.produce(self.sdUUID).hasVolumeLeases()),
so why not always do this?
Line 242:         ]
Line 243: 
Line 244:         for itemName, srcPath, dstPath, itemRequired in itemsList:
Line 245:             self.log.debug("Sharing %s %s to %s (required: %s)", itemName,

--
To view, visit http://gerrit.ovirt.org/8270
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/fileVolume.py
Line 237:             ('metadata', self._getMetaVolumePath(),
Line 238:                          self._getMetaVolumePath(dstVolumePath), True),
Line 239:             ('lease', self._getLeaseVolumePath(),
Line 240:                       self._getLeaseVolumePath(dstVolumePath),
Line 241:                       sdCache.produce(self.sdUUID).hasVolumeLeases()),
In fact we always *try* to do it. But "itemRequired" it means that is *mandatory* and when you're sharing a template that is V1/V2 (probably no lease file if it was generated in 3.0) the share would fail. We could try to generate the template lease file here but I'm worried that we would be tangling different things together.
Line 242:         ]
Line 243: 
Line 244:         for itemName, srcPath, dstPath, itemRequired in itemsList:
Line 245:             self.log.debug("Sharing %s %s to %s (required: %s)", itemName,

--
To view, visit http://gerrit.ovirt.org/8270
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I237e4a8f094ba04dcd4ef7bff418e03f81162d8d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

Itamar Heim has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1:

ping

Itamar Heim has posted comments on this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Patch Set 1:

abandoning - old. no reply. not touched for a while. please restore if relevant

Itamar Heim has abandoned this change.

Change subject: [wip] volume: make volume._share idempotent
......................................................................

Abandoned