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 ykaplan@redhat.com --- 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"]
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
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.
vdsm-patches@lists.fedorahosted.org