Oved Ourfali has uploaded a new change for review.
Change subject: wip - vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
wip - vdsm: improve message when trying to attach import domain with wrong permissions (#842146)
https://bugzilla.redhat.com/842146
When a storage domain is attached, and there is something wrong with the directory permissions, then the operation fails with "Cannot obtain lock" error. This patch improves the error in this case, by performing pre-attach validations, which, on file domains, include checking the permissions of the domain directory, printing a suitable error message.
Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Signed-off-by: Oved Ourfali oourfali@redhat.com --- M vdsm/storage/fileSD.py M vdsm/storage/sd.py M vdsm/storage/sp.py 3 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/39/7339/1
diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index ea817b7..2df10c3 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -217,6 +217,18 @@ return fileVolume.FileVolume
+ def preAttachValidations(self): + """ + Check that we have access to the domain directory + """ + try: + validateDirAccess(self.domaindir) + except OSError as e: + if e.errno == errno.EACCES: + raise se.StorageServerAccessPermissionError(self.domaindir) + raise + return + def volumeExists(self, imgPath, volUUID): """ Return True if the volume volUUID exists diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py index 174adee..2eb7349 100644 --- a/vdsm/storage/sd.py +++ b/vdsm/storage/sd.py @@ -365,6 +365,12 @@ """ pass
+ def preAttachValidations(self): + """ + Do all validataion checks relevant when attaching storage domain + """ + return + @classmethod def validateCreateVolumeParams(cls, volFormat, preallocate, srcVolUUID): """ diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py index ee45151..0ff39ed 100644 --- a/vdsm/storage/sp.py +++ b/vdsm/storage/sp.py @@ -921,6 +921,7 @@ raise se.TooManyDomainsInStoragePoolError()
dom = sdCache.produce(sdUUID) + dom.preAttachValidations() dom.acquireHostId(self.id)
try:
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com
Oved Ourfali has posted comments on this change.
Change subject: wip - vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 1:
I've put WIP, as I'm still validating it. Wanted to make sure that the general approach is acceptable.
Thank you, Oved
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: wip - vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/535/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Ayal Baron has posted comments on this change.
Change subject: wip - vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 1: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 2: Verified
Verified. Only changed commit message (removed the wip).
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/537/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Shu Ming has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/sd.py Line 366: pass Line 367: Line 368: def preAttachValidations(self): Line 369: """ Line 370: Do all validataion checks relevant when attaching storage domain Please fix the spell error: validataion --->validation
Would it be better like the below?
Do all relevant validation checks when attaching storage domain. Line 371: """ Line 372: return Line 373: Line 374: @classmethod
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/574/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: wip vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 4:
New version uses a different approach, and return the error in the connectStorageServer (that calls MountConnection-->connect function).
The UI returns ""There is no storage domain under the specified path. Please check path." - in red letters, in the import dialog.
An event saying a permissions problem has happened is created, so the user can see it in the UI.
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: wip vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/585/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Ayal Baron has posted comments on this change.
Change subject: wip vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(4 inline comments)
+1 for nfs/posix logic. -1 for lacking same in local
.................................................... File vdsm/storage/storageServer.py Line 113: except OSError as e: Line 114: if e.errno == errno.EACCES: Line 115: raise StorageServerAccessPermissionError(path) Line 116: raise Line 117: return no need for 'return' Line 118: Line 119: class ExampleConnection(object): Line 120: """Do not inherit from this object it is just to show and document the Line 121: connection object interface"""
Line 193: Line 194: if self.isConnected(): Line 195: try: Line 196: validatePermissions(self.getMountObj().getRecord().fs_file) Line 197: except StorageServerAccessPermissionError as ex: no need for as Line 198: self.disconnect() Line 199: raise ex Line 200: Line 201: def isConnected(self):
Line 195: try: Line 196: validatePermissions(self.getMountObj().getRecord().fs_file) Line 197: except StorageServerAccessPermissionError as ex: Line 198: self.disconnect() Line 199: raise ex no need for 'ex' Line 200: Line 201: def isConnected(self): Line 202: return self._mount.isMounted() Line 203:
Line 416: def _getLocalPath(self): Line 417: return os.path.join(self.localPathBase, self._path.replace("_", "__").replace("/", "_")) Line 418: Line 419: def checkTarget(self): Line 420: return os.path.exists(self._path) and os.path.isdir(self._path) need to validate permissions here. need to verify that vdsm cannot only write but can also delete the directory Line 421: Line 422: def checkLink(self): Line 423: lnPath = self._getLocalPath() Line 424: if os.path.lexists(lnPath):
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: wip vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 4: (1 inline comment)
.................................................... File vdsm/storage/storageServer.py Line 416: def _getLocalPath(self): Line 417: return os.path.join(self.localPathBase, self._path.replace("_", "__").replace("/", "_")) Line 418: Line 419: def checkTarget(self): Line 420: return os.path.exists(self._path) and os.path.isdir(self._path) Currently, the validateDirAccess function checks for os.R_OK and os.X_OK. Weird that it doesn't check for W_OK. Changing it will effect all locations that use it, but it seems right, as we obviously need write permissions there, right? Line 421: Line 422: def checkLink(self): Line 423: lnPath = self._getLocalPath() Line 424: if os.path.lexists(lnPath):
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 5: Verified
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/610/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) ......................................................................
Patch Set 5: (1 inline comment)
.................................................... Commit Message Line 3: AuthorDate: 2012-08-22 11:14:41 +0300 Line 4: Commit: Oved Ourfali oourfali@redhat.com Line 5: CommitDate: 2012-08-23 10:31:23 +0300 Line 6: Line 7: vdsm: improve message when trying to attach import domain with wrong permissions (#842146) Please drop the "vdsm:" prefix. We know this is a Vdsm patch.
Please adhere to project standard about mentioning the BZ#. I think they are currently
Bug-Id: BZ#XXXXXX Line 8: Line 9: https://bugzilla.redhat.com/842146 Line 10: Line 11: When a storage domain is attached, and there is something wrong with the
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/611/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Ayal Baron has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 6: Looks good to me, but someone else must approve
(1 inline comment)
.................................................... File vdsm/storage/storageServer.py Line 190: raise Line 191: Line 192: self._mount.mount(self.options, self._vfsType) Line 193: Line 194: if self.isConnected(): check is irrelevant here. Line 195: try: Line 196: validatePermissions(self.getMountObj().getRecord().fs_file) Line 197: except StorageServerAccessPermissionError: Line 198: self.disconnect()
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 7:
addressed Ayal's comments (removed the redundant test).
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/620/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Eduardo has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 7: I would prefer that you didn't submit this
(4 inline comments)
.................................................... File vdsm/storage/storageServer.py Line 28: import glob Line 29: from collections import namedtuple Line 30: import misc Line 31: from functools import partial Line 32: from storage_exception import InvalidParameterException, StorageServerAccessPermissionError Please use import exception as se and correct the InvalidParameterException too. Line 33: Line 34: import mount Line 35: import fileSD Line 36: import iscsi
Line 108: """ Line 109: Check that we have access to the path Line 110: """ Line 111: try: Line 112: fileSD.validateDirAccess(path) Please change the exception in validateDirAccess(), and remove this function. Remove the the try catch clauses uses. And please the StorageServerAccessPermissionError change remove the default ''" parameter. Line 113: except OSError as e: Line 114: if e.errno == errno.EACCES: Line 115: raise StorageServerAccessPermissionError(path) Line 116: raise
Line 192: self._mount.mount(self.options, self._vfsType) Line 193: Line 194: try: Line 195: validatePermissions(self.getMountObj().getRecord().fs_file) Line 196: except StorageServerAccessPermissionError: Please try to correct permissions here. Out out the disconnect since it can fails too. Check disconnect policy with Ayal. Line 197: self.disconnect() Line 198: raise Line 199: Line 200: def isConnected(self):
Line 415: def _getLocalPath(self): Line 416: return os.path.join(self.localPathBase, self._path.replace("_", "__").replace("/", "_")) Line 417: Line 418: def checkTarget(self): Line 419: return os.path.exists(self._path) and os.path.isdir(self._path) and validatePermissions(self._path) Please remove redundant is exists() call. Line 420: Line 421: def checkLink(self): Line 422: lnPath = self._getLocalPath() Line 423: if os.path.lexists(lnPath):
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 8:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/663/ : FAILURE
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Ayal Baron has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 8: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/storage/storage_exception.py Line 916: " access to the storage. Verify permission settings."\ Line 917: " Cause: %s." Line 918: def __init__(self, targetPath, cause): Line 919: self.value = targetPath Line 920: self.value = cause this is a bug (you're overriding 'value' with cause after setting it with targetPath).
vdsm deliberately does not support dynamic exceptions due to support of localization in engine. Simplest would probably be to add an exception: StorageServerAccessOwnershipError Line 921: self.message = self.message % (targetPath, cause) Line 922: Line 923: class MountTypeError(StorageException): Line 924: code = 470
.................................................... File vdsm/storage/storageServer.py Line 181: Line 182: try: Line 183: validatePermissions(self.getMountObj().getRecord().fs_file) Line 184: except StorageServerAccessPermissionError: Line 185: self.disconnect() disconnect should be in try...catch clause otherwise an exception there would mask the real problem here. Line 186: raise Line 187: Line 188: def isConnected(self): Line 189: return self._mount.isMounted()
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 8:
New storage exception requires handling in the engine. Will be pushed in a different patch there.
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 10:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/666/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 11:
1. no need for two errors, as the two tests in validateDirAccess test for vdsm and for qemu user. Not performing testing of permissions and ownership separately.
2. Addressing all Eyal's comments.
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Eduardo has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 11: I would prefer that you didn't submit this
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Eduardo has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 12: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/storageServer.py Line 181: self._mount.mount(self.options, self._vfsType) Line 182: Line 183: try: Line 184: fileSD.validateDirAccess(self.getMountObj().getRecord().fs_file) Line 185: except se.StorageServerAccessPermissionError ':' Where? Line 186: try: Line 187: self.disconnect() Line 188: except OSError: Line 189: self.log.warn("Error while disconnecting after access problem", exc_info=True)
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Eduardo has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 13: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Oved Ourfali has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 13: Verified
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 13:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/671/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
Patch Set 13: Looks good to me, approved
Automatic score escalation, trusting Eduardo more than myself.
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: improve message when trying to attach import domain with wrong permissions ......................................................................
improve message when trying to attach import domain with wrong permissions
When a storage domain is attached, and there is something wrong with the directory permissions, then the operation fails with "Cannot obtain lock" error. This patch improves the error in this case, by performing validations when doing the server connection. If the permission validation fails, we disconnect.
The user will get an event specifying the permissions error. And, he will get a UI error saying: "There is no storage domain under the specified path. Please check path."
This fix also applies to posix and local storage.
Bug-Id: BZ#842146 Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Signed-off-by: Oved Ourfali oourfali@redhat.com --- M vdsm/storage/fileSD.py M vdsm/storage/localFsSD.py M vdsm/storage/nfsSD.py M vdsm/storage/storageServer.py M vdsm/storage/storage_exception.py 5 files changed, 27 insertions(+), 24 deletions(-)
Approvals: Eduardo: Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved Oved Ourfali: Verified
-- To view, visit http://gerrit.ovirt.org/7339 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I76880b1445f259431d7aa691eda676e1210308e4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org