Hi, I have these concerns regarding command execution in Manila. I was to propose these to be discussed at the Design Summit. It might be too late for that. Then we can discuss it here -- or at the summit, just informally.
Thanks for Valeriy for his ideas about some of the topics below. Driver ancestry --------------- See ExecuteMixin [1]: - method _try_execute is not used anywhere - method set_execute only used in __init__ and it could be inlined - __init__ just inlines ShareDriver.__init__ and sets up self._execute - ExecuteMixin itself is used only in conjunction with ShareDriver (to derive specific driver classes from them) [1] http://git.openstack.org/cgit/openstack/manila/tree/manila/share/driver.py?id=f67311a#n49 Plan: eliminate ExecuteMixin - Drop _try_execute. If it's needed in future, it can be resurrected as a utility function (ie. toplevel function in utils.py). - Drop set_execute, instead set set self._execute directly. - ShareDriver.__init__ should take over execute management from ExecuteMixin, ie. set self._execute from kwargs['execute'] if available, else fall back to utils.execute. - Drop the ExecuteMixin class. Impact: a driver class definition that currently takes this from: SomeDriver(driver.ExecuteMixin, driver.ShareDriver) will be identical to the SomeDriver(driver.ShareDriver) definition after the change. SSH execution ------------- ## Signature Terminology: - by 'executor' we mean a function that's used for command execution in some way (locally, remotely, etc.) - the standard (or default, cf. previous section) executor is utils.execute; we will refer to its signature [2] as the 'standard executor signature' - 'standard signature executor': an executor the signature of which is the standard executor signature [2] in the sense of http://en.wikipedia.org/wiki/Type_signature The demand for transparent remote execution naturally arises for drivers which do command line basedd management of a resource or service that can be available both locally and remotely. That is, an instance of such a resource manager class ideally would get a standard signature executor at initialization and it would perform executions via this executor with no knowledge whether it acts locally or remotely. Providing the executor, appropriately set up for either local or remote execution, is up to the instantiator. However, currently local command execution is typically done by utils.execute (or a wrapper that changes some defaults), and remote execution is typically done with processutils.ssh_execute (or some wrapper of it). And these two functions differ in signature, because ssh_execute takes also an ssh connection object as parameter. Thus such manager classes either resign of execution transparency and come in local and remote variants, or they implement some wrapper around processutils.ssh_execute that ensures standard executor signature. My proposal is: - implement and standardize an ssh executor with standard signature - it would be built atop of processutils.ssh_execute - it would apply some basic strategy for managing the ssh connection internally - let all drivers use this executor instead of direct processutils.ssh_execute invocations (unless there is a manifest demand for custom management of the ssh connection) Here is a prototype of that idea: https://review.openstack.org/gitweb?p=openstack/manila.git;a=blob;f=manila/share/drivers/ganesha/utils.py;h=463da36;hb=7443672#l70 It's a class that's instantiated with the same parameters as an ssh connection, and the corresponding ssh connection is created upon instantiation. Moreover, it's a callable class [3], with standard executor signature, whereby a call of an instance of it performs remote execution using the ssh connection with which the instance had been initialized. [3] ie. instances of it are callable, ie. the __call__ instance method is defined, cf. https://docs.python.org/2/reference/datamodel.html#object.__call__ ## Escaping The other major difference between local and remote (SSH based) execution that former is argument array based, latter is command string based (which is passed to shell at the remote end). This is not a problem, as the argument array can be transformed in a safe and faithful manner to a shell command string by shell escaping the arguments and then joining them with space. This algorithm is indeed obligatory to get at a correct result. So the standard ssh executor will also include it properly. Therefore standardizing ssh execution will improve safety by eliminating the need to do ad hoc conversions between argument arrays and command strings. ## run_as_root processutils.ssh_exec does not handle the run_as_root parameter. That's however a low hanging fruit -- given its presence, just sudo(8) (or custom root wrapper) should be prepended to the command string. (It is actually a realistic concern, as remote nodes might be set up to allow remote log in with unprivileged user and to curate administrative tasks via sudo.) shell vs run_as_root -------------------- I found a bug in utils.execute, inherited from upstream (oslo.concurrency). See the bug item for details: https://bugs.launchpad.net/oslo.concurrency/+bug/1382873 also my propsed fix (to be resubmitted for oslo.concurrency): https://review.openstack.org/129447 Csaba _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
