On 2/6/14 1:58 PM, "Matthew Booth" <[email protected]> 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" <[email protected]> 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 >>> [email protected] >>> >>>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 >> [email protected] >> >>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 _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
