Open Stack

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.

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://review.openstack.org/#/c/61555/.

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=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
> 
> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 


-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

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