Saggi Mizrahi has uploaded a new change for review.
Change subject: Refactor storage connection subsystem ......................................................................
Refactor storage connection subsystem
The current one is an abysmal mess. This is a tramsitional patch retrofitting the old API to some of the concept we intend to implement.
This introduces ConnectionMonitor. It currently doesn't do any monitoring just connects and disconnects from storage. It uses URIs for specifying storage targets which is more generic and simple to parse.
Notice: Permissions are no longer checked on storage connect only on domain initialization. This is good as the storage connection is far from an apropriate place for that check.
I've removed stale root folder handling for NFS. I will add it back I can't do everything at once.
There are also no proper engine acceptable exceptions yet
Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d --- M vdsm.spec.in M vdsm/storage/Makefile.am A vdsm/storage/connectionMonitor.py M vdsm/storage/fileSD.py M vdsm/storage/hsm.py M vdsm/storage/iscsi.py M vdsm/storage/localFsSD.py M vdsm/storage/misc.py M vdsm/storage/mount.py M vdsm/storage/multipath.py M vdsm/storage/nfsSD.py A vdsm/storage/storageMailbox.py D vdsm/storage/storage_connection.py A vdsm/storage/sync.py 14 files changed, 884 insertions(+), 486 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/1038/1 -- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 1: (2 inline comments)
.................................................... File vdsm/storage/sync.py Line 1: from threading import Thread, Event GPL boilerplate missing
Line 28: except Exception as e: consider storing (or logging) the backtrace too.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com
Ayal Baron has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(27 inline comments)
.................................................... Commit Message Line 9: The current one is an abysmal mess. This is a tramsitional patch s/The current one/Currently connection management/ s/tramsitional/transitional/
Line 10: retrofitting the old API to some of the concept we intend to implement. s/concept/concepts/
Line 19: apropriate place for that check. s/apropriate/appropriate/
.................................................... File vdsm/storage/connectionMonitor.py Line 38: # This are properties used to define how to parse the uri for this type of These properties are used...
Line 39: # connection. Declaring all of them is mendatory. s/mendatory/mandatory/
Line 56: Do not vlidate the scheme of the URI as it is declared by the monitor object. s/vlidate/validate/
Line 66: """Disconnect if connected. If already disconnected fail silantly""" s/silantly/silently/
Line 79: """Implement this! It will be used to detect multiple connection to the s/connection/connections/
Line 99: def parseParam(param): what about a comment stating what param looks like? it is unclear when reviewing the function and parseParam says very little.
given input: "a='a'&b='b'&anotherparam"
Iiuc above would yield: ((a,"a"),(b,"b"),("anotherparam"))
?
Line 130: j = r Here we're missing several examples and expected output, something of the sort:
nfs://abcd (ex. 6?) gluster:something@anotherthing (ex pos of @) gluster:something@anotherthing?blabla (ex pos of ?) gluster:something@anotherthing?blabla;blublu (ex pos of ;)
Line 137: nonHierarchical = True double negative is annoying, why did they name it this way? 'hierarchical = False' is simpler
Line 142: localPathBase = "/tmp" why is this hard coded?
Line 190: if self._mount.isMounted(): why not start with this check?
Line 220: DEFAULT_OPTIONS = ["soft", "nosharecache", "vers=3"] I thought we got rid of 'vers=3' to allow for autonegotiation by default...
Line 245: pass what is the point of the above section? Placeholder for something that still needs to be fixed? if so, why don't you have a FIXME? also, you throw and catch and don't do anything with it (i.e. you ignore) is that intentional? Also, the comment makes no sense, I'm guessing what you meant was: hostname:port/path ???
Line 247: if uri.username: I'm assuming this is needed for better vers=4 support?
Line 249: if uri.password: can password be passed without username (similar to iscsi's shared secret)
Line 250: raise ValueError("Custom login is not supported") Again, FIXMEs? Also, messages are not very descriptive. Consider writing something like: mounting NFS with custom login is not supported.
Line 252: for key, val in params: what's wrong with: if key in ("timeout", "retrans", "version"): kwargs[key] = int(val)
???
Line 283: usesFragment = False add space line
Line 293: username ="" space before ""
Line 314: kwargs["target"] = uri.path[1:] # ignore the / at path start Is there no possibility of passing path without leading '/'?
Line 325: elif key == "initiator_name": why is the key name different than what we're passing? (and don't say backward compatibility as uri support is not bc)
Line 437: raise ValueError("Custom port is not supported") bad copy paste?
Line 441: if uri.username: I've stopped reviewing here, will try to continue at a later point in time.
.................................................... File vdsm/storage/Makefile.am Line 28: connectionMonitor.py \ what kind of connections does this monitor? I'm not sure the name is expressive enough
Line 59: sync.py \ what sync does is wrap calls and execute async? Unless I'm missing something this name is confusing. asyncDispatcher or something?
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 1: (13 inline comments)
.................................................... File vdsm/storage/connectionMonitor.py Line 137: nonHierarchical = True I think so too but this is how they declare it in the python URI module.
Line 142: localPathBase = "/tmp" It's a default. You can change is with setLocaPathBase() It is changed on HSM init.
Line 190: if self._mount.isMounted(): No reason. It doesn't really matter
Line 220: DEFAULT_OPTIONS = ["soft", "nosharecache", "vers=3"] Yea, I kinda did both patches at the same time. I need to remove this.
Line 245: pass The point is that uri.port is a property and can throw a Value Error. I missed the fact that I throw one as well. I will have to find a way around this.
Line 247: if uri.username: Maybe in the future.
Line 249: if uri.password: a uri can have a password with blank user name.
Line 250: raise ValueError("Custom login is not supported") I'm actually kinda hating the overly specific errors. You know what you are doing when you fail.
When you get ENOENT from open you don't need it to say you failed during file open you already there, you got the stack trace.
Line 252: for key, val in params: I think you mean:
for key in ("timeout", "retrans", "version"): if not key in params: continue kwargs[key] = int(params[key])
The arg parsing part is something I'm not sure I want per class yet and might like a way for the class to declare what part of the uri should contain what arg and in what form and just have a method that does it right.
Anyway it's not really important.
Line 314: kwargs["target"] = uri.path[1:] # ignore the / at path start How would you differentiate from the host
google.comindex.html?
Line 325: elif key == "initiator_name": Our variable names are mixedCaps but I would like to have the URIs to be case insensitive so they use underscores to separate between words so they are easy to read in any form,
Line 437: raise ValueError("Custom port is not supported") Intentional see above comments
.................................................... File vdsm/storage/Makefile.am Line 59: sync.py \ In the future I will move all sync related stuff from misc to this file. Like RWLock, sampling methods, etc...
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Ayal Baron has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 2:
You rebased it! it's impossible to compare to patch set 1 and see what was changed. Patches of this size should either not be rebased until all changes are accepted or rebase upload patchset (same as previous patchset only rebased) then change and upload patchset.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 4: (8 inline comments)
Not finished yet. It's too big and complicated
.................................................... File vdsm.spec.in Line 477: %{_datadir}/%{vdsm_name}/storage/storageProividerConnectionMonitor.py* Did we agreed about sorting?
.................................................... File vdsm/storage/hsm.py Line 127: # ISCSI domain shouldn't even be on the list but VDSM use to just Do you mean FCP?
Line 1871: status = 200 Use se.StorageException.code
Line 1897: corosponding domains type corresponding
Line 1900: with keys depnding ont the type ont???
Line 1904: :returns: a list of statuses status will be 0 if disconnection was successful Ah, you fixed docstring here. I wrote in you previous patch dedicated to docstring that you should change this one too. Consider to merge this two patches
Line 1927: status = 200 Use se.StorageException.code
.................................................... File vdsm/storage/Makefile.am Line 28: storageProviderConnectionMonitor.py \ Did we agreed about sorting?
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 8: (2 inline comments)
Partial review, but I feel this code could be much simplified by using urlparse.
.................................................... File vdsm/storage/storageProviderConnectionMonitor.py Line 117: return tuple(params) It feels like you're implementing urlparse.parse_qs here.
Line 146: return url[i:j] Again, this feels a lot like implementing urlparse.urlparse.
parsed = urlparse.urlparse('iscsi://user:pass@hostname.example.org/iqn;portal=3') (parsed.hostname or "") + parsed.path.split(';', 1)[0]
'hostname.example.org/iqn'
The or "" is needed for file:// where there is no hostname.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 10: I would prefer that you didn't submit this
(14 inline comments)
.................................................... File vdsm.spec.in Line 486: %{_datadir}/%{vdsm_name}/storage/storageProviderConnectionMonitor.py* I think you sorted it by yourself
.................................................... File vdsm/storage/hsm.py Line 127: # ISCSI domain shouldn't even be on the list but VDSM use to just ISCSI ->FCP
Line 1858: ac = self._connectionMonitor.manage(uri, uri) according to signature: def manage(self, uri, conId) Should it be ac = self._connectionMonitor.manage(uri, conId) ?
Line 1871: status = 200 Can you please use the proper error code from storage_exception.py ? Something like se.XXX.code
Line 1897: corosponding domains type corosponding -> coresponding
Line 1900: with keys depnding ont the type depnding -> depending, ont -> on
Line 1906: """ I already wrote this, but you ignored it. You fixed in your prev patch (http://gerrit.ovirt.org/#change,1098) same doc string in connectStorageServer. Please consider to squash these two patches.
Line 1915: ac = self._connectionMonitor.unmanage(uri) the signature is: def unmanage(self, conId). So, should it be self._connectionMonitor.unmanage(conId) ?
Line 1927: status = 200 Can you please use the proper error code from storage_exception.py ? Something like se.XXX.code
.................................................... File vdsm/storage/storageProviderConnectionMonitor.py Line 117: return tuple(params) I am not sure, but did you addressed Ewoud's comments from patchset 8 ?
Line 146: return url[i:j] Same as above
Line 186: raise ValueError("Custom login is not supported") login -> password
Line 266: raise ValueError("Custom login is not supported") login -> password
Line 465: raise ValueError("Custom login is not supported") login -> password
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 10: (6 inline comments)
.................................................... File vdsm/storage/hsm.py Line 1858: ac = self._connectionMonitor.manage(uri, uri) I actually use the uri as the ID because I don't get a unique ID and this is pretty unique for the connection
Line 1871: status = 200 I'ts on my todo list
Line 1906: """ F**ing rebase
Line 1915: ac = self._connectionMonitor.unmanage(uri) As I said, I'm using the uri as ID in the backward compatible functions
.................................................... File vdsm/storage/storageProviderConnectionMonitor.py Line 117: return tuple(params) I'm removing this entire thing in future patches. We decided not to continue with URIs. Just leave it be
Line 146: return url[i:j] I'm removing this entire thing in future patches. We decided not to continue with URIs. Just leave it be
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 11: I would prefer that you didn't submit this
(5 inline comments)
.................................................... File vdsm/storage/hsm.py Line 1865: conInfo = _connectionDict2ConnectionInfo(conDef) Should it get 2 parameters as in signature: _connectionDict2ConnectionInfo(conTypeId, conDict) ? Should you pass domType too?
Line 1874: status = 200 Use exception's code
Line 1913: conInfo = _connectionDict2ConnectionInfo(conDef) Should it get 2 parameters as in signature: _connectionDict2ConnectionInfo(conTypeId, conDict) ? Should you pass domType too?
Line 1922: status = 200 exception code
Line 1931: def _translateConnectionError(self, e): Why you need this? No one using it.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 11: (3 inline comments)
.................................................... File vdsm/storage/hsm.py Line 1865: conInfo = _connectionDict2ConnectionInfo(conDef) You are right. My bad.
Line 1913: conInfo = _connectionDict2ConnectionInfo(conDef) You are right once again!
Line 1931: def _translateConnectionError(self, e): I actually forgot I wrote that. I should use this
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 13: Looks good to me, but someone else must approve
(1 inline comment)
Oh, it's so big and complicated. I have a minor question inline, but it's too big to be revied without your explanations. I hope that I didn't missed things. Is it passed pre-integ?
.................................................... File vdsm/storage/storageServer.py Line 106: return os.path.join(self.localPathBase, self._remotePath.replace("/", "_").replace("_", "__")) Why we need it? Is it mean that each '/' and '_' will be replaced to '__' ? How you will differentiate between '/' an '_' ?
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 14: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Federico Simoncelli has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 14: (2 inline comments)
.................................................... File vdsm/storage/iscsiadm.py Line 12: class IscsiInterfaceError(IscsiError): pass Is it possible to squash these changes into "ISCSI Subsystem refactoring"? http://gerrit.ovirt.org/1607
.................................................... File vdsm/storage/Makefile.am Line 59: storageServer.py \ Tab inconsistency.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Federico Simoncelli has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 15:
There are two comments in patch set 14 that were probably overlooked.
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 15: I would prefer that you didn't submit this
(1 inline comment)
1. Please address Federico's comments from patch set 14. 2. Please open the ticket to pre-integration
.................................................... File vdsm/storage/hsm.py Line 174: return storageServer.ConnectionInfo(typeName, params) Do you need define 'params' in typeName == 'iscsi' ?
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 15: Fails
(6 inline comments)
pyflakes failures cause a build failure.
.................................................... File vdsm/storage/storageServer.py Line 22: import logging pyflakes:
vdsm/storage/storageServer.py:22: 'logging' imported but unused
Line 23: from os.path import normpath, basename, splitext pyflakes:
vdsm/storage/storageServer.py:23: 'splitext' imported but unused vdsm/storage/storageServer.py:23: 'basename' imported but unused
Line 25: from threading import RLock, Lock, Event, Thread pyflakes:
vdsm/storage/storageServer.py:25: 'Thread' imported but unused vdsm/storage/storageServer.py:25: 'RLock' imported but unused vdsm/storage/storageServer.py:25: 'Lock' imported but unused vdsm/storage/storageServer.py:25: 'Event' imported but unused
Line 27: import pickle pyflakes:
vdsm/storage/storageServer.py:27: 'pickle' imported but unused
Line 28: import glob pyflakes:
vdsm/storage/storageServer.py:28: 'glob' imported but unused
Line 34: from sync import asyncmethod, AsyncCallStub pyflakes:
vdsm/storage/storageServer.py:34: 'asyncmethod' imported but unused vdsm/storage/storageServer.py:34: 'AsyncCallStub' imported but unused
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: shu ming sming56@gmail.com
Dan Kenigsberg has posted comments on this change.
Change subject: Refactor storage connection subsystem ......................................................................
Patch Set 20: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: shu ming sming56@gmail.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Refactor storage connection subsystem ......................................................................
Refactor storage connection subsystem
Currently connection management is an abysmal mess. This is a transitional patch retrofitting the old API to some of the concepts we intend to implement.
Notice: Permissions are no longer checked on storage connect only on domain initialization. This is good as the storage connection is far from an appropriate place for that check.
I've removed stale root folder handling for NFS. I will add it back I can't do everything at once.
There are also no proper engine acceptable exceptions yet
Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d --- M vdsm.spec.in M vdsm/storage/Makefile.am M vdsm/storage/fileSD.py M vdsm/storage/hsm.py M vdsm/storage/iscsiadm.py M vdsm/storage/localFsSD.py M vdsm/storage/nfsSD.py A vdsm/storage/storageServer.py D vdsm/storage/storage_connection.py M vdsm/storage/storage_exception.py 10 files changed, 553 insertions(+), 433 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/1038 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: Ifc5c160aba9b4cc88225dde0c44f9b766c6a4c0d Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: shu ming sming56@gmail.com
vdsm-patches@lists.fedorahosted.org