Open Stack

There's currently an effort to create a common internal API to the
vSphere/ESX API:

https://blueprints.launchpad.net/oslo/+spec/vmware-api

I see there's some code already in place which essentially copies what's
currently in Nova. Having spent some time digging in this code recently,
I would take the opportunity of this refactor to fix a few design issues
in the current code, which has an 'organic' feel to it.

The current code has 2 main objects:

* driver.VMwareAPISession

This object creates a Vim object and manages a session in it. It
provides _call_method(), which calls a method in an external module and
retries it if it failed because of a bad session. _call_method has also
had shoehorned into it the ability to make direct Vim calls.

* vim.Vim

This object creates a connection to vSphere/ESX and provides an API to
make remote calls.

Here are 2 typical uses of the API, both taken from vmops.py:

---
  hardware_devices = self._session._call_method(vim_util,
              "get_dynamic_property", vm_ref,
              "VirtualMachine", "config.hardware.device")
---

This is using _call_method() to wrap:

  vim_util.get_dynamic_property(vm_ref, "VirtualMachine,
                                "config.hardware.device")

vim_util.get_dynamic_property() does an amount of work and creates a
number of objects before ultimately calling:

 return vim.RetrievePropertiesEx(...)

Note that in the event that this call fails, for example due to a
session timeout or a network error, the entire function will be
needlessly re-executed.

---
  reconfig_task = self._session._call_method(
                                  self._session._get_vim(),
                                  "ReconfigVM_Task", vm_ref,
                                  spec=vmdk_attach_config_spec)
  self._session._wait_for_task(instance_uuid, reconfig_task)
---

This is using _call_method() to wrap:

  reconfig_task = vim.ReconfigVM_Task(
      vm_ref, spec=vmdk_attach_config_spec)
  wait_for_task(reconfig_task)              [1]

Things wrong with both of the above:
* It obfuscates the intention.
* It makes backtraces confusing.
* It's possible to forget to use _call_method() and it will still work,
resulting in uncaught intermittent faults.

Additionally, the choice of the separation of driver.VMwareAPISession
and vim.Vim results in several confused messes. In particular, notice
how the fault checker called by vim_request_handler can raise
FAULT_NOT_AUTHENTICATED, which then has to be caught and rechecked by
the driver because the required context isn't available in Vim.

As somebody who has come to this code recently, I can also attest that
the varying uses of _call_method with a module or a vim object, and the
fact that it isn't used at all in some places, is highly confusing to
anybody who isn't intimately familiar with the driver.

There's also a subtle point to do with the Vim object's use of
__getattr__ to syntactically sugar remote API calls: it can lead to
non-obvious behaviour. An example of this is
https://bugs.launchpad.net/nova/+bug/1275773, where the use of a Vim
object in boolean context to test for None[2] resulted in an attempt to
make a remote API call '__nonzero__'. Another example is
https://review.openstack.org/#/c/69652/ where the indirection, combined
with my next point about object orientedness, disguised a syntactically
incorrect call which wasn't being covered by a test.

The syntactic sugar isn't even working in our favour, as it doesn't
model the remote API. The vSphere API is very much object oriented, with
methods being properties of the managed object they are being called on
(e.g. a PropertyCollector or SessionManager). With that in mind, a
python programmer would expect to do something like:

  propertyCollector.RetrievePropertiesEx(<args>)

However, what we actually do is:

  vim.RetrievePropertiesEx(propertyCollector, <args>)

With all of the above in mind, I would replace both VMwareAPISession and
Vim with a single new class called VIM (not to be confused with the old
one).

class VIM(object):
  def __init__(self, host_ip=CONF.vmware.host_ip,
               username=CONF.vmware.host_username,
               password=CONF.vmware.host_password,
               retry_count=CONF.vmware.api_retry_count,
               scheme="https"):
    # This same arguments as to the old VMwareAPISession
    # Create a suds client and log in

  def get_service_object():
    # Return a service object using the suds client

  def call(object, method, *args, **kwargs):
    # Ditch __getattr__(). No unexpected remote API calls.
    # call() always takes the same first 2 arguments.
    #
    # call() will do session management, and retry the call if it fails.
    # It will also create a new suds client if necessary, for example
    # in the event of a network error. Note that it will only retry a
    # single api call, not a whole function.
    #
    # All remote API calls outside the VIM object will use this method.
    #
    # Fault checking lives here.

  def _call_no_session(object, method, *args, **kwargs):
    # In doing session management itself, there are certain calls which
    # it doesn't make sense to wrap in session management (e.g. Login).
    # This method should not be used outside the VIM object itself.
    # call() will be a wrapper round this method.

  def wait_for_task(task):
    # Block until completion of a task
    # N.B. We could potentially make this automatic in call(), as we
    # currently always wait for completion of tasks. call() would then
    # return the result of the task.

With this change, all calls in vim_util would be updated to use
VIM.call(), the same as all other code. In this way, session retries
would be automatic and confined to just the remote API call.

Incidentally, I can cut/paste this to the blueprint if that's a better
place for this kind of discussion.

Matt

[1] Note that the first argument to the current _wait_for_task() isn't
actually used.

[2] PEP8 recommends against this, btw.
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490