[openstack-dev] [Nova][vmware] A new VMwareAPISession
Gary Kotton
gkotton at vmware.com
Thu Feb 6 12:07:35 UTC 2014
On 2/6/14 1:58 PM, "Matthew Booth" <mbooth at redhat.com> wrote:
>On 06/02/14 11:24, Gary Kotton wrote:
>> 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.
>>In moving the code to oslo there's going to be some donkey work required
>to find all current uses of the code and fix them up. I'm proposing this
>change now because it would be a great opportunity to do that donkey
>work just once.
The work has already been done - https://review.openstack.org/#/c/70175/
This also has a +1 from the minesweeper which means that the API's are
working correctly.
>>Also notice that the actual usage of the proposed API is very similar to
>the old one. The donkey work would essentially amount to:
>>* Change all instances of vim.Method(object, args) in vim_util to
>vim.call(object, Method, args)
>>* Change all instances of session._call_method(vim_util, 'method', args)
>everywhere to vim_util.method(session, args)
>>Note that the changes are mechanical, and you're going to have to touch
>it for the move to oslo anyway.
>>Also note that the proposed API would, at least in the first instance,
>be substantially a cut and paste of existing code.
>>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.
>>I've read it. It's certainly much prettier python, but the design is the
>same as the Nova 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://urldefense.proofpoint.com/v1/url?u=https://review.openstack.org/%
>>23/c/61555/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=eH0pxTUZo8NPZyF6hgoMQu%
>>2BfDtysg45MkPhCZFxPEq8%3D%0A&m=co4zzllM3r0kGeYRL5kq9xJgzPe0T9gSqW%2B2XOJ%
>>2FTKY%3D%0A&s=3efe43037653b6caff24d2d253f566c1a1defa1fe4f0a902f57a17bf1bf
>>d2311.
>>That's one of the explicit design goals of the proposed API. Notice that
>mistakes like this are no longer possible, as the call() method does
>session management and retries, and there is no other exposed interface
>to make remote API calls.
>>> 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.
>>I think it's almost no additional work to fix it at this stage, given
>that the code is being refactored anyway and it will require donkey work
>in the driver to match up with it. If we wait until later it becomes a
>whole additional task.
>>Matt
>>> 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=eH0pxTUZ
>>>o8
>>>>>>NPZyF6hgoMQu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDiG5VZAeq
>>>Sk
>>>>>>w8MPwOPQ4k8zs%3D%0A&s=facc68779808dd3d6fb45fbb9a7addb9c8f392421bfe850a99
>>>41
>>> 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/no
>>>va
>>>>>>/%2Bbug/1275773&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=eH0pxTUZo8NPZyF6hg
>>>oM
>>>>>>Qu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3ZwDiG5VZAeqSkw8MPwOPQ
>>>4k
>>>>>>8zs%3D%0A&s=db08b5e7f320ff7df857d33a65fbe96e61783518e4e4ae2a65020b12bd51
>>>51
>>> 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/cg
>>>i-
>>>>>>bin/mailman/listinfo/openstack-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r
>>>=e
>>>>>>H0pxTUZo8NPZyF6hgoMQu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=8nIWvYjr1NVyVpYg3Zw
>>>Di
>>>>>>G5VZAeqSkw8MPwOPQ4k8zs%3D%0A&s=84e7db6622831d91bbbefc376aa524bf9c231412f
>>>56
>>> 86eb02f10ba386df76d0e
>>>>>> _______________________________________________
>> 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
>>=eH0pxTUZo8NPZyF6hgoMQu%2BfDtysg45MkPhCZFxPEq8%3D%0A&m=co4zzllM3r0kGeYRL5
>>kq9xJgzPe0T9gSqW%2B2XOJ%2FTKY%3D%0A&s=42623707260a67a0f10ba942de955d96167
>>4bd36f1b8a7d2dbdef6bffe4fb987
>>>>>--
>Matthew Booth, RHCA, RHCSS
>Red Hat Engineering, Virtualisation Team
>>GPG ID: D33C3490
>GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
More information about the OpenStack-dev
mailing list