Change in vdsm[master]: vdsm: Throw MountError in mount.getRecord()(#883877)

Vered Volansky has uploaded a new change for review.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

vdsm: Throw MountError in mount.getRecord()(#883877)

getRecord() in mount.py threw OSError, while _translateConnectionError in hsm.py
expectes MountError. getRecord now throws MountError. Other calling methods
expecting OSError are now expecting MountError, according to the fix.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=883877
Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Signed-off-by: Vered Volansky 
---
M vdsm/storage/mount.py
M vdsm/storage/storageServer.py
2 files changed, 5 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/66/10966/1

diff --git a/vdsm/storage/mount.py b/vdsm/storage/mount.py
index 81ca17f..df3ea7b 100644
--- a/vdsm/storage/mount.py
+++ b/vdsm/storage/mount.py
@@ -252,7 +252,7 @@
     def isMounted(self):
         try:
             self.getRecord()
-        except OSError:
+        except MountError:
             return False

return True
@@ -263,9 +263,9 @@
                     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))
+        raise MountError(errno.ENOENT,
+                         "Mount of `%s` at `%s` does not exist" %
+                         (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 b9c0847..b653a71 100644
--- a/vdsm/storage/storageServer.py
+++ b/vdsm/storage/storageServer.py
@@ -268,7 +268,7 @@
                 return 4
             else:
                 return 3
-        except OSError:
+        except MountError:
             # We are not connected
             pass

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/703/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/668/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Vered Volansky has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

Patch Set 1: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

Patch Set 1:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/703/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

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

(1 inline comment)

I think that the mount.py module is fine as it is.
Where is the problematic call to getRecord()? MountConnection.connect? I'd rather catch OSError there, and translate it to whatever is needed.

....................................................
File vdsm/storage/mount.py
Line 262:             if (record.fs_spec == self.fs_spec and
Line 263:                     record.fs_file == self.fs_file):
Line 264:                 return record
Line 265: 
Line 266:         raise MountError(errno.ENOENT,
"MountError" means: the mount executable did not succeed.

This is not the case here. What happened here is "there is no such mountpoint as the one you are asking for". ENOENT seems just fine to represent this.
Line 267:                          "Mount of `%s` at `%s` does not exist" %
Line 268:                          (self.fs_spec, self.fs_file))
Line 269: 
Line 270:     def __repr__(self):

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Shu Ming has posted comments on this change.

Change subject: vdsm: Throw MountError in mount.getRecord()(#883877)
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/mount.py
Line 262:             if (record.fs_spec == self.fs_spec and
Line 263:                     record.fs_file == self.fs_file):
Line 264:                 return record
Line 265: 
Line 266:         raise MountError(errno.ENOENT,
Also,  I think making getRecord to raise an exception is not reasonable when the record is not existing.  Why not just return a None when the record is not existing? By returning None,  it is easy to handle for the upper level function.
Line 267:                          "Mount of `%s` at `%s` does not exist" %
Line 268:                          (self.fs_spec, self.fs_file))
Line 269: 
Line 270:     def __repr__(self):

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1193/ (1/3)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1228/ (3/3)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/339/ (2/3)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 2:

Build Successful

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 2314:             return se.iSCSIifaceError.code, se.iSCSIifaceError.message
Line 2315:         if isinstance(e, iscsi.iscsiadm.IscsiError):
Line 2316:             return se.iSCSISetupError.code, se.iSCSISetupError.message
Line 2317:         if isinstance(e, os.OSError, os.OSError.message):
Line 2318:             return os.OSError.code, os.OSError.message
I do not think that

os.OSError.code

exists, and suspect that you'd want to translate the exception to a StorageException, legible by Engine.
Line 2319: 
Line 2320:         if hasattr(e, 'code'):
Line 2321:             return e.code, e.message
Line 2322:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1615/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1580/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3:

Build Successful

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

Ayal Baron has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

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

(1 inline comment)

I'd appreciate a response to my former question:
"Where is the problematic call to getRecord()? MountConnection.connect? I'd rather catch OSError there, and translate it to whatever is needed."

....................................................
File vdsm/storage/hsm.py
Line 2352:             return 0, ""
Line 2353: 
Line 2354:         if isinstance(e, mount.MountError):
Line 2355:             return se.MountError.code, se.MountError.message
Line 2356:         if isinstance(e, OSError):
_translateConnectionError() is called in 6 different places. I find it hard to believe that in all of these cases, any type of OSError means StorageServerValidationError.
Line 2357:             return (se.StorageServerValidationError.code,
Line 2358:                     se.StorageServerValidationError.message)
Line 2359:         if isinstance(e, iscsi.iscsiadm.IscsiAuthenticationError):
Line 2360:             return se.iSCSILoginAuthError.code, se.iSCSILoginAuthError.message

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

Eduardo has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

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

(2 inline comments)

translateConnectionError() should be removed since is redundant.
In addition in each call the except Exception clause should be removed and the correct type catched.

....................................................
File vdsm/storage/hsm.py
Line 2346:         # so invalidate the caches
Line 2347:         sdCache.refreshStorage()
Line 2348:         return dict(statuslist=res)
Line 2349: 
Line 2350:     def _translateConnectionError(self, e):
This is function is doing nothing.
What's the difference of each case?
In any case returns (code, msg).
Please remove it.
Line 2351:         if e is None:
Line 2352:             return 0, ""
Line 2353: 
Line 2354:         if isinstance(e, mount.MountError):

Line 2352:             return 0, ""
Line 2353: 
Line 2354:         if isinstance(e, mount.MountError):
Line 2355:             return se.MountError.code, se.MountError.message
Line 2356:         if isinstance(e, OSError):
+1.OSError in the iscsi flows has a different meaning.
Line 2357:             return (se.StorageServerValidationError.code,
Line 2358:                     se.StorageServerValidationError.message)
Line 2359:         if isinstance(e, iscsi.iscsiadm.IscsiAuthenticationError):
Line 2360:             return se.iSCSILoginAuthError.code, se.iSCSILoginAuthError.message

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

Vered Volansky has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3: (2 inline comments)

Dan, I didn't and still don't see such previous suggestion.
The problematic call to getRecord() is from storageServer.py:
fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file) .
Eduardo, If I'm removing my change from _translateConnectionError(), then I have no business beautifying it in this patch. Will Resolve the solution with Dan offline.

....................................................
File vdsm/storage/hsm.py
Line 2346:         # so invalidate the caches
Line 2347:         sdCache.refreshStorage()
Line 2348:         return dict(statuslist=res)
Line 2349: 
Line 2350:     def _translateConnectionError(self, e):
I don't get what you're saying, this function is obviously doing something. In any case, Dan's comment suggests this is not the function I should be touching for this bug.
Line 2351:         if e is None:
Line 2352:             return 0, ""
Line 2353: 
Line 2354:         if isinstance(e, mount.MountError):

Line 2352:             return 0, ""
Line 2353: 
Line 2354:         if isinstance(e, mount.MountError):
Line 2355:             return se.MountError.code, se.MountError.message
Line 2356:         if isinstance(e, OSError):
Ack.
Line 2357:             return (se.StorageServerValidationError.code,
Line 2358:                     se.StorageServerValidationError.message)
Line 2359:         if isinstance(e, iscsi.iscsiadm.IscsiAuthenticationError):
Line 2360:             return se.iSCSILoginAuthError.code, se.iSCSILoginAuthError.message

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

Ayal Baron has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3:

wrt removing _translateConnectionError it should be discussed u/s first as I'm not sure Saggi would agree with that, but in general I dislike it as well.  however, that should be dealt with separately (i.e. out of the scope of this fix).

Dan, the OSError is *generated* by mount.py "raise OSError(errno.ENOENT, 'Mount target %s not found' % target)" so there is no real lower level to catch it.
mount.py is built in this way (dealing with OS level errors only).  This can be debated, but again not relevant to this patch (a bigger rehaul perhaps).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: Catch OSError in _translateConnectionError

Eduardo has posted comments on this change.

Change subject: vdsm: Catch OSError in _translateConnectionError
......................................................................

Patch Set 3:

As stated in:
https://bugzilla.redhat.com/show_bug.cgi?id=883877#c10
mount.py is raising OSError when fails it fails to find the dst directory that just succeed to remove, after a failed mount. 
The mount error is catched and swallowed and verify is called immediately. 
Applying something in the spirit of:
http://gerrit.ovirt.org/#/c/12042/
makes mount.py raise the correct MountError for errors during mount, making this change redundant.

If OSErrors occurs in mount, there is nothing much to do
in higher levels of vdsm. It may be due to mount point
being created/deleted by other thread or another _strange Thing_ that is probably a programming error.

I thing that is worth to distinguish between the failed mount that is something for the user to fix (permissions, existence of the mount, etc.) and errors during the manipulation of the mount point.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1666/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1622/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: Verified

Initial solution missed the origin of the problem, which is that MountError is razed and then ignored, causing getRecord() to fail, though it shouldn't be called to begin with. Note that commit message changed accordingly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4:

Build Successful

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

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

(2 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
I think is better to merge patch:
http://gerrit.ovirt.org/12042
with my comments addressed.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Line 214:             except OSError:
Line 215:                 pass
os.rmdir() should not raise anything but OSError.
Line 216:             raise e
Line 217: 
Line 218:         try:
Line 219:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (3 inline comments)

....................................................
Commit Message
Line 7: vdsm: propagate MountError when mount fails
Line 8: 
Line 9: When mount fails the MountError is not propagated, causing an error when
Line 10: getRecord() in mount.py is called. getRecord() shouldn't be reached
Line 11: after the MountError to begin with. This fix now re-razes the previously
Will fix to razes typo in next commit.
Line 12: swallowed MountError, sending the right error code to the engine.
Line 13: 
Line 14: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=883877
Line 15: Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181

....................................................
File vdsm/storage/storageServer.py
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
I disagree. This patch fixes a specific bug, while 12042 sounds like (from commit message) "lets make things more beautiful" and is unrelated to the bug this patch should fix. Not in comment and not in bug url existence.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Line 214:             except OSError:
Line 215:                 pass
In general this is true, yet here we would like to remove the directory should the mount fail on the one hand, and reflect the true problem in the connect function on the other. We don't really care whether the removal of the directory succeeded or not, yet we want to true cause of the problem to be propagated.
Line 216:             raise e
Line 217: 
Line 218:         try:
Line 219:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
Please look  at the code!

The patch was sent a long time before the bug was open.

Sure the commit message should be updated, and the code improved.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
I see what you mean, but I don't see a clash between the two. The other patch is not sufficient because the MountError will not be propagated to the engine, so the bug will not be solved unless this patch is enclosed at least in the other. I also think that since the two don't clash and there's a working solution it should also be merged, unless it's nacked on it's own merrits.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
Also - totally not true about the timeline. The opposite is correct.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (5 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 200:         if self._mount.isMounted():
Line 201:             return
Line 202: 
Line 203:         try:
Line 204:             os.makedirs(self._getLocalPath())
Use fileutils.createdir() instead.
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208:

Line 207:                 raise
Line 208: 
Line 209:         try:
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
You need to log here the MountError.
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Line 214:             except OSError:
Line 215:                 pass

Line 209:         try:
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Please remove this try clause. There is no any reason to assume that failing to remove the mount point is OK  here. Nobody should fiddle with the mount point during this thread mount process.
Line 214:             except OSError:
Line 215:                 pass
Line 216:             raise e
Line 217:

Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Line 214:             except OSError:
Is a very bad practice to silently pass exceptions.

Anyway, remove this try-except.
Line 215:                 pass
Line 216:             raise e
Line 217: 
Line 218:         try:

Line 214:             except OSError:
Line 215:                 pass
Line 216:             raise e
Line 217: 
Line 218:         try:
The necessity for this check should be reconsidered if fileutils.createdir() was used.

If it remains this block should be an else block.
Line 219:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)
Line 220:         except se.StorageServerAccessPermissionError:
Line 221:             try:
Line 222:                 self.disconnect()

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1674/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1630/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5:

Build Successful

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Ayal Baron has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 4: (3 inline comments)

Only thing that needs revising here is the logging issue (need to log mount failure)

....................................................
File vdsm/storage/storageServer.py
Line 200:         if self._mount.isMounted():
Line 201:             return
Line 202: 
Line 203:         try:
Line 204:             os.makedirs(self._getLocalPath())
Since Edu asked to review in light of 12042, then note that the suggestion to use fileutils.createdir here is irrelevant since the permissions you'd use are not relevant once dir is mounted (hence permissions check below is mandatory)
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208:

Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208: 
Line 209:         try:
What is missing here and available there is the logging that mount failed.
Line 210:             self._mount.mount(self.options, self._vfsType)
Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())

Line 211:         except MountError as e:
Line 212:             try:
Line 213:                 os.rmdir(self._getLocalPath())
Line 214:             except OSError:
Line 215:                 pass
right, not catching here would be masking a first order problem (mount failure) with a second order problem (removal of mount point failed when trying to clean up after mount failure)
Line 216:             raise e
Line 217: 
Line 218:         try:
Line 219:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5:

The reason for fileuitls.createdir() is to avoid repeat again the EENOENT code.

The most probably reason for the remove dir to fail is the that the directory was already removed. In this case you have a race that can cause the mount to fail. Therefore this a "zero" cause that should be treated differently. At least log it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Ayal Baron has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 5:

Edu, there is no ENOENT there so unless you comment in the code I'm not sure what you're referring to...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1681/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1636/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: Verified

Added log message, the rest is the same.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6:

Build Successful

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/storageServer.py
Line 213:                            exc_info=True)
Line 214:             try:
Line 215:                 os.rmdir(self._getLocalPath())
Line 216:             except OSError:
Line 217:                 pass
You should log here. Failing to remove the mount point points to a severe programming issue and may be the cause that failed the mount.
Please remove the comment.
Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: (2 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 200:         if self._mount.isMounted():
Line 201:             return
Line 202: 
Line 203:         try:
Line 204:             os.makedirs(self._getLocalPath())
Please replace by fileutils.createdir()
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208:

Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221: 
Line 222:         try:
An else block will avoid future errors.
Line 223:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)
Line 224:         except se.StorageServerAccessPermissionError:
Line 225:             try:
Line 226:                 self.disconnect()

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Ayal Baron has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: (2 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 213:                            exc_info=True)
Line 214:             try:
Line 215:                 os.rmdir(self._getLocalPath())
Line 216:             except OSError:
Line 217:                 pass
failing to delete a directory 99% of the cases would just mean that we do not have permissions to delete it.  Since 'mount' is run as root then most likely there is no connection between this failure and mount failure.
Although I agree about logging here, this is no way a severe programming issue.
Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221:

Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221: 
Line 222:         try:
Besides the fact that it's redundant like adding an else to the first 'if' in this function, in this case it might also mask errors (reaching end of function successfully when we should be failing).
Line 223:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)
Line 224:         except se.StorageServerAccessPermissionError:
Line 225:             try:
Line 226:                 self.disconnect()

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Eduardo has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: (3 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 213:                            exc_info=True)
Line 214:             try:
Line 215:                 os.rmdir(self._getLocalPath())
Line 216:             except OSError:
Line 217:                 pass
If the directory was already removed at this point you have a raise.
Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221:

Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221: 
Line 222:         try:
You should not validate if the mount failed. If another clause, which not raises, is added to the try mount you have the same bug that you are correcting now. Else avoids this issue.
Line 223:             fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file)
Line 224:         except se.StorageServerAccessPermissionError:
Line 225:             try:
Line 226:                 self.disconnect()

Line 226:                 self.disconnect()
Line 227:             except OSError:
Line 228:                 self.log.warn("Error while disconnecting after access problem",
Line 229:                               exc_info=True)
Line 230:             raise
raises OSError instead StorageServerAccessPermissionError
Line 231: 
Line 232:     def isConnected(self):
Line 233:         return self._mount.isMounted()
Line 234:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has posted comments on this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: (3 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 200:         if self._mount.isMounted():
Line 201:             return
Line 202: 
Line 203:         try:
Line 204:             os.makedirs(self._getLocalPath())
Is this really in the scope of this patch?
Line 205:         except OSError as e:
Line 206:             if e.errno != errno.EEXIST:
Line 207:                 raise
Line 208:

Line 213:                            exc_info=True)
Line 214:             try:
Line 215:                 os.rmdir(self._getLocalPath())
Line 216:             except OSError:
Line 217:                 pass
Will add log.
Line 218:             #We raise the MountError here since this is the real issue and not
Line 219:             #the minor rmdir success.
Line 220:             raise e
Line 221:

Line 226:                 self.disconnect()
Line 227:             except OSError:
Line 228:                 self.log.warn("Error while disconnecting after access problem",
Line 229:                               exc_info=True)
Line 230:             raise
I believe this is beyond the scope of this patch as it attempts to fix a specific bug. I don't object to submitting another one after this one. Ayal?
Line 231: 
Line 232:     def isConnected(self):
Line 233:         return self._mount.isMounted()
Line 234:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: vdsm: propagate MountError when mount fails

Vered Volansky has abandoned this change.

Change subject: vdsm: propagate MountError when mount fails
......................................................................

Patch Set 6: Abandoned

Dealt with in 12042

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I0f36b3ea18690d7cf53439e5a0342b1495f4f181
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: oVirt Jenkins CI Server