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