Open Stack

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