Open Stack

Hi,
Thanks for the detailed mail. For the first step of moving the code into
OSLO we are trying to be as conservative as possible (similar to the fork
lift of the scheduler code). That is, we are taking working code and
moving it to the common library, not doing any rewrites and using the same
functionality and flows. Once that is in and stable then we should start
to work on making it more robust. Incidentally the code that was copied is
not Nova's but Cinders. When the Cinder driver was being posted the team
made a considerable amount of improvements to the driver.
The reason that the _call_method is used is that this deals with session
failures and has support for retries etc. Please see
https://review.openstack.org/#/c/61555/.
I certainly agree that we can improve the code moving forwards. I think
that the first priority should get a working version in OSLO. Once it is
up and running then we should certain start addressing issues that you
have raised.
Thanks
Gary


On 2/6/14 12:43 PM, "Matthew Booth" <mbooth at redhat.com> wrote:

>There's currently an effort to create a common internal API to the
>vSphere/ESX API:
>
>https://urldefense.proofpoint.com/v1/url?u=https://blueprints.launchpad.ne
>t/oslo/%2Bspec/vmware-api&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=eH0pxTUZo8
>NPZyF6hgoMQu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDiG5VZAeqSk
>w8MPwOPQ4k8zs%3D%0A&s=facc68779808dd3d6fb45fbb9a7addb9c8f392421bfe850a9941
>ec732195d641
>
>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://urldefense.proofpoint.com/v1/url?u=https://bugs.launchpad.net/nova
>/%2Bbug/1275773&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=eH0pxTUZo8NPZyF6hgoM
>Qu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDiG5VZAeqSkw8MPwOPQ4k
>8zs%3D%0A&s=db08b5e7f320ff7df857d33a65fbe96e61783518e4e4ae2a65020b12bd5151
>a1, 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://urldefense.proofpoint.com/v1/url?u=https://review.openstack.org/%2
>3/c/69652/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=eH0pxTUZo8NPZyF6hgoMQu%2B
>fDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDiG5VZAeqSkw8MPwOPQ4k8zs%3
>D%0A&s=5a9b7e98691181391abe0366d40b066f01ac68ae3231d10a3711f2c54287dca6
>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
>
>_______________________________________________
>OpenStack-dev mailing list
>OpenStack-dev at lists.openstack.org
>https://urldefense.proofpoint.com/v1/url?u=http://lists.openstack.org/cgi-
>bin/mailman/listinfo/openstack-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=e
>H0pxTUZo8NPZyF6hgoMQu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDi
>G5VZAeqSkw8MPwOPQ4k8zs%3D%0A&s=84e7db6622831d91bbbefc376aa524bf9c231412f56
>86eb02f10ba386df76d0e