Open Stack

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