Change in vdsm[master]: mount: Reassign mount specification in case of backup option

Yeela Kaplan has uploaded a new change for review.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

mount: Reassign mount specification in case of backup option

When using glusterfs volumes the backup option
'backupvolfile-server' allows mount to an alternative volume
when mount source not available.

Change-Id: I3166c6863dffa297bc0adcdeb4c22f810d18de8e
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=922744
Signed-off-by: Yeela Kaplan 
---
M vdsm/storage/mount.py
M vdsm/storage/storageServer.py
2 files changed, 44 insertions(+), 20 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/34/16534/1

diff --git a/vdsm/storage/mount.py b/vdsm/storage/mount.py
index 90cacc7..8cade18 100644
--- a/vdsm/storage/mount.py
+++ b/vdsm/storage/mount.py
@@ -187,6 +187,15 @@
     raise OSError(errno.ENOENT, 'device %s not mounted' % device)

+def _getRecord(spec, file):
+    for record in _iterMountRecords():
+        if (record.fs_spec == spec and record.fs_file == file):
+            return record
+
+    raise OSError(errno.ENOENT,
+                  "Mount of `%s` at `%s` does not exist" % (spec, file))
+
+
 class Mount(object):
     def __init__(self, fs_spec, fs_file):
         self.fs_spec = normpath(fs_spec)
@@ -261,14 +270,7 @@
         return True

def getRecord(self):
-        for record in _iterMountRecords():
-            if (record.fs_spec == self.fs_spec and
-                    record.fs_file == self.fs_file):
-                return record
-
-        raise OSError(errno.ENOENT,
-                      "Mount of `%s` at `%s` does not exist" %
-                      (self.fs_spec, self.fs_file))
+        return _getRecord(self.fs_spec, self.fs_file)

def __repr__(self):
         return ("<Mount fs_spec='%s' fs_file='%s'>" %
diff --git a/vdsm/storage/storageServer.py b/vdsm/storage/storageServer.py
index 023f607..515ebd9 100644
--- a/vdsm/storage/storageServer.py
+++ b/vdsm/storage/storageServer.py
@@ -197,14 +197,14 @@
                             self._remotePath.replace("_",
                                                      "__").replace("/", "_"))

-    def connect(self):
+    def createConnection(self):
         if self._mount.isMounted():
             return

fileUtils.createdir(self._getLocalPath())

try:
-            self._mount.mount(self.options, self._vfsType)
+            self._mount.mount(self.options, self.vfsType)
         except MountError as e:
             self.log.error("Mount failed: %s", e, exc_info=True)
             try:
@@ -214,17 +214,21 @@
                               self._getLocalPath(), exc_info=True)
             raise e

-        else:
+    def validateConnection(self):
+        try:
+            fileSD.validateDirAccess(
+                self.getMountObj().getRecord().fs_file)
+        except se.StorageServerAccessPermissionError as e:
             try:
-                fileSD.validateDirAccess(
-                    self.getMountObj().getRecord().fs_file)
-            except se.StorageServerAccessPermissionError as e:
-                try:
-                    self.disconnect()
-                except OSError:
-                    self.log.warn("Error while disconnecting after access"
-                                  "problem", exc_info=True)
-                raise e
+                self.disconnect()
+            except OSError:
+                self.log.warn("Error while disconnecting after access"
+                              "problem", exc_info=True)
+            raise e
+
+    def connect(self):
+        self.createConnection()
+        self.validateConnection()

def isConnected(self):
         return self._mount.isMounted()
@@ -255,6 +259,24 @@
     def getLocalPathBase(cls):
         return os.path.join(MountConnection.getLocalPathBase(), "glusterSD")

+    def updateBackup(self):
+        if self.options and not self.isConnected():
+            for opt in self.options.split(','):
+                if opt.strip().startswith("backupvolfile-server"):
+                    backupSpec = opt.split('=', 1)[1].strip()
+                    try:
+                        mount._getRecord(backupSpec, self._mount.fs_file)
+                    except OSError as e:
+                        self.log.warning("Mount of backup failed: %s", e)
+                    else:
+                        self._mount.fs_spec = backupSpec
+                    return
+
+    def connect(self):
+        self.createConnection()
+        self.updateBackup()
+        self.validateConnection()
+

class NFSConnection(object):
     DEFAULT_OPTIONS = ["soft", "nosharecache"]

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 1: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3209/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3130/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2319/ : SUCCESS

Saggi Mizrahi has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(7 inline comments)

....................................................
File vdsm/storage/mount.py
Line 186: 
Line 187:     raise OSError(errno.ENOENT, 'device %s not mounted' % device)
Line 188: 
Line 189: 
Line 190: def _getRecord(spec, file):
Why move it out of the instance. If you have the spec and file you can get a mount instance easily.
Line 191:     for record in _iterMountRecords():
Line 192:         if (record.fs_spec == spec and record.fs_file == file):
Line 193:             return record
Line 194:

....................................................
File vdsm/storage/storageServer.py
Line 196:         return os.path.join(self.getLocalPathBase(),
Line 197:                             self._remotePath.replace("_",
Line 198:                                                      "__").replace("/", "_"))
Line 199: 
Line 200:     def createConnection(self):
Why is this public
Line 201:         if self._mount.isMounted():
Line 202:             return
Line 203: 
Line 204:         fileUtils.createdir(self._getLocalPath())

Line 213:                 self.log.warn("Failed to remove mount point directory: %s",
Line 214:                               self._getLocalPath(), exc_info=True)
Line 215:             raise e
Line 216: 
Line 217:     def validateConnection(self):
Shouldn't be public and you don't validate the connection you validate the mount point.
Line 218:         try:
Line 219:             fileSD.validateDirAccess(
Line 220:                 self.getMountObj().getRecord().fs_file)
Line 221:         except se.StorageServerAccessPermissionError as e:

Line 253:     def __hash__(self):
Line 254:         return hash(type(self)) ^ hash(self._mount)
Line 255: 
Line 256: 
Line 257: class GlusterFSConnection(MountConnection):
Who allowed this????
Line 258: 
Line 259:     def getLocalPathBase(cls):
Line 260:         return os.path.join(MountConnection.getLocalPathBase(), "glusterSD")
Line 261:

Line 259:     def getLocalPathBase(cls):
Line 260:         return os.path.join(MountConnection.getLocalPathBase(), "glusterSD")
Line 261: 
Line 262:     def updateBackup(self):
Line 263:         if self.options and not self.isConnected():
if not self.options or self.isConnected():
    return

saves an indentation
Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
Line 267:                     try:

Line 262:     def updateBackup(self):
Line 263:         if self.options and not self.isConnected():
Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
move the iteration to it's own function
self.getBackupSpec() so the logic is not in the middle of the iteration.
Line 267:                     try:
Line 268:                         mount._getRecord(backupSpec, self._mount.fs_file)
Line 269:                     except OSError as e:
Line 270:                         self.log.warning("Mount of backup failed: %s", e)

Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
Line 267:                     try:
Line 268:                         mount._getRecord(backupSpec, self._mount.fs_file)
if mount.Mount(backupSpec, self._mout.fs_file).isMounted()
Line 269:                     except OSError as e:
Line 270:                         self.log.warning("Mount of backup failed: %s", e)
Line 271:                     else:
Line 272:                         self._mount.fs_spec = backupSpec

Yeela Kaplan has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 1: (6 inline comments)

....................................................
File vdsm/storage/mount.py
Line 186: 
Line 187:     raise OSError(errno.ENOENT, 'device %s not mounted' % device)
Line 188: 
Line 189: 
Line 190: def _getRecord(spec, file):
Done
Line 191:     for record in _iterMountRecords():
Line 192:         if (record.fs_spec == spec and record.fs_file == file):
Line 193:             return record
Line 194:

....................................................
File vdsm/storage/storageServer.py
Line 196:         return os.path.join(self.getLocalPathBase(),
Line 197:                             self._remotePath.replace("_",
Line 198:                                                      "__").replace("/", "_"))
Line 199: 
Line 200:     def createConnection(self):
Done
Line 201:         if self._mount.isMounted():
Line 202:             return
Line 203: 
Line 204:         fileUtils.createdir(self._getLocalPath())

Line 213:                 self.log.warn("Failed to remove mount point directory: %s",
Line 214:                               self._getLocalPath(), exc_info=True)
Line 215:             raise e
Line 216: 
Line 217:     def validateConnection(self):
Done
Line 218:         try:
Line 219:             fileSD.validateDirAccess(
Line 220:                 self.getMountObj().getRecord().fs_file)
Line 221:         except se.StorageServerAccessPermissionError as e:

Line 259:     def getLocalPathBase(cls):
Line 260:         return os.path.join(MountConnection.getLocalPathBase(), "glusterSD")
Line 261: 
Line 262:     def updateBackup(self):
Line 263:         if self.options and not self.isConnected():
Done
Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
Line 267:                     try:

Line 262:     def updateBackup(self):
Line 263:         if self.options and not self.isConnected():
Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
Done
Line 267:                     try:
Line 268:                         mount._getRecord(backupSpec, self._mount.fs_file)
Line 269:                     except OSError as e:
Line 270:                         self.log.warning("Mount of backup failed: %s", e)

Line 264:             for opt in self.options.split(','):
Line 265:                 if opt.strip().startswith("backupvolfile-server"):
Line 266:                     backupSpec = opt.split('=', 1)[1].strip()
Line 267:                     try:
Line 268:                         mount._getRecord(backupSpec, self._mount.fs_file)
Done
Line 269:                     except OSError as e:
Line 270:                         self.log.warning("Mount of backup failed: %s", e)
Line 271:                     else:
Line 272:                         self._mount.fs_spec = backupSpec

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2646/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3453/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3537/ : SUCCESS

Ayal Baron has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 269:             return
Line 270: 
Line 271:         backupSpec = self._getBackupSpec()
Line 272:         if backupSpec:
Line 273:             if mount.Mount(backupSpec, self._mount.fs_file).isMounted():
backupvolfile-server specifies an alternative to the main mount path in case the latter fails in mounting.  By default, if mount to the regular path succeeds then backupSpec would not be mounted.
IIUC here you expect it always to be mounted which is incorrect.
We should only check it if _validateMountPoint fails.
Line 274:                 self._mount.fs_spec = backupSpec
Line 275:             else:
Line 276:                 self.log.warning("Mount of backup: %s to: %s failed",
Line 277:                                  backupSpec, self._mount.fs_file)

Yeela Kaplan has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 269:             return
Line 270: 
Line 271:         backupSpec = self._getBackupSpec()
Line 272:         if backupSpec:
Line 273:             if mount.Mount(backupSpec, self._mount.fs_file).isMounted():
I do not expect it to be mounted,

At the beginning of '_updateBackup' I check if connected to the original mount point and return if it is...
Line 274:                 self._mount.fs_spec = backupSpec
Line 275:             else:
Line 276:                 self.log.warning("Mount of backup: %s to: %s failed",
Line 277:                                  backupSpec, self._mount.fs_file)

Ayal Baron has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: No score

(1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 269:             return
Line 270: 
Line 271:         backupSpec = self._getBackupSpec()
Line 272:         if backupSpec:
Line 273:             if mount.Mount(backupSpec, self._mount.fs_file).isMounted():
Got it.
I have 2 different questions though:
1. does gluster support more than one backupvolfile-server option? (if so, this wouldn't cover the case of successful mount to third host I believe)
2. does this affect connectionMonitor?
Line 274:                 self._mount.fs_spec = backupSpec
Line 275:             else:
Line 276:                 self.log.warning("Mount of backup: %s to: %s failed",
Line 277:                                  backupSpec, self._mount.fs_file)

Yeela Kaplan has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 269:             return
Line 270: 
Line 271:         backupSpec = self._getBackupSpec()
Line 272:         if backupSpec:
Line 273:             if mount.Mount(backupSpec, self._mount.fs_file).isMounted():
1. Currently only one backup option is available, but according to this bug recently opened it will change and be internal to gluster, trying multiple mount points, making it tricky for us to know the actual mount:

https://bugzilla.redhat.com/show_bug.cgi?id=986429

2. ConnectionMonitor is not yet in use. But if it were then it serializes connection information sent from engine to vdsm. Engine is not aware of the actual mount connection available only in vdsm (since vdsm returns to engine only connection id), therefore we will persist the original connection and not only the backup.
Line 274:                 self._mount.fs_spec = backupSpec
Line 275:             else:
Line 276:                 self.log.warning("Mount of backup: %s to: %s failed",
Line 277:                                  backupSpec, self._mount.fs_file)

Ayal Baron has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: Looks good to me, approved

Saggi Mizrahi has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2: I would prefer that you didn't submit this

I still don't really understand what you are trying to do. I would like a better commit message to explaing modifying mount values through private interfaces

Ayal Baron has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2:

(1 comment)

....................................................
Commit Message
Line 7: mount: Reassign mount specification in case of backup option
Line 8: 
Line 9: When using glusterfs volumes the backup option
Line 10: 'backupvolfile-server' allows mount to an alternative volume
Line 11: when mount source not available.
Please add something like:
Gluster supports the ability to mount using a failback server in case mounting using the primary server fails.  This ability is specified by the 'backupvolfile-server' mount option.
Since mount can end up using a different server than the one specified in the main uri, vdsm needs to take this possibility into account.
This patch adds support for this mount option.
Line 12: 
Line 13: Change-Id: I3166c6863dffa297bc0adcdeb4c22f810d18de8e
Line 14: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=922744

Itamar Heim has posted comments on this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Patch Set 2:

ping?

Itamar Heim has abandoned this change.

Change subject: mount: Reassign mount specification in case of backup option
......................................................................

Abandoned

abandoning per no reply. please restore if still relevant.