Virtual machine management with decorated selfies

Question

I have a class which provides various methods for retrieving information from a VMware vSphere environment.

Over the time my class got extended and became large, so I thought I should spend some time to re-design it.

Currently I am thinking of re-designing a bit in that class and move/group related methods into separate modules as this would help with keeping the whole project modular and better organized as various functionality would be covered by dedicated modules.

All my class methods rely on an attribute which is performing the connection and interaction with the VMware vSphere SDK, so in order to move my methods into dedicated modules as functions I need to make sure to pass that attribute as well.

In order to get rid of the instance methods, I was thinking of using a decorator for my functions that would simply register functions as available tasks in a class attribute, which in turn would be available to instances of that class and can be called to perform the required operation.

Now I have this updated version of my class + a decorator to be used when creating new vSphere tasks.

""" vPoller Agent module for the VMware vSphere Poller vPoller Agents are used by the vPoller Workers, which take care of establishing the connection to the vSphere hosts and do all the heavy lifting. Check the vSphere Web Services SDK API for more information on the properties you can request for any specific vSphere managed object - https://www.vmware.com/support/developer/vc-sdk/ """ import logging from functools import wraps from vconnector.core import VConnector from vpoller.client.core import VPollerClientMessage __all__ = ['VSphereAgent', 'task'] class VSphereAgent(VConnector): """ VSphereAgent class Defines methods for retrieving vSphere object properties These are the worker agents that do the actual polling from the VMware vSphere host. Extends: VConnector """ _tasks = {} @classmethod def _add_task(cls, name, function, required): cls._tasks[name] = { 'function': function, 'required': required, } def call_task(self, name, *args, **kwargs): """ Execute a vPoller task request Args: name (str): Name of the task to be executed """ if name not in self._tasks: return {'success': 1, 'msg': 'Unknown task requested'} return self._tasks[name]['function'](self, *args, **kwargs) def _discover_objects(self, properties, obj_type): """ Helper method to simplify discovery of vSphere managed objects This method is used by the '*.discover' vPoller Worker methods and is meant for collecting properties for multiple objects at once, e.g. during object discovery operation. Args: properties (list): List of properties to be collected obj_type (pyVmomi.vim.*): Type of vSphere managed object Returns: The discovered objects in JSON format """ logging.info( '[%s] Discovering %s managed objects', self.host, obj_type.__name__ ) view_ref = self.get_container_view(obj_type=[obj_type]) try: data = self.collect_properties( view_ref=view_ref, obj_type=obj_type, path_set=properties ) except Exception as e: return {'success': 1, 'msg': 'Cannot collect properties: %s' % e} view_ref.DestroyView() result = { 'success': 0, 'msg': 'Successfully discovered objects', 'result': data, } logging.debug( '[%s] Returning result from operation: %s', self.host, result ) return result def _get_object_properties(self, properties, obj_type, obj_property_name, obj_property_value, include_mors=False): """ Helper method to simplify retrieving of properties This method is used by the '*.get' vPoller Worker methods and is meant for collecting properties for a single managed object. We first search for the object with property name and value, then create a list view for this object and finally collect it's properties. Args: properties (list): List of properties to be collected obj_type pyVmomi.vim.*): Type of vSphere managed object obj_property_name (str): Property name used for searching for the object obj_property_value (str): Property value identifying the object in question Returns: The collected properties for this managed object in JSON format """ logging.info( '[%s] Retrieving properties for %s managed object of type %s', self.host, obj_property_value, obj_type.__name__ ) # Find the Managed Object reference for the requested object try: obj = self.get_object_by_property( property_name=obj_property_name, property_value=obj_property_value, obj_type=obj_type ) except Exception as e: return {'success': 1, 'msg': 'Cannot collect properties: %s' % e} if not obj: return { 'success': 1, 'msg': 'Cannot find object %s' % obj_property_value } # Create a list view for this object and collect properties view_ref = self.get_list_view(obj=[obj]) try: data = self.collect_properties( view_ref=view_ref, obj_type=obj_type, path_set=properties, include_mors=include_mors ) except Exception as e: return {'success': 1, 'msg': 'Cannot collect properties: %s' % e} view_ref.DestroyView() result = { 'success': 0, 'msg': 'Successfully retrieved object properties', 'result': data, } logging.debug( '[%s] Returning result from operation: %s', self.host, result ) return result def _object_datastore_get(self, obj_type, name): """ Helper method used for getting the datastores available to an object This method searches for the managed object with 'name' and retrieves the 'datastore' property which contains all datastores available/used by the managed object, e.g. VirtualMachine, HostSystem. Args: obj_type (pyVmomi.vim.*): Managed object type name (str): Name of the managed object, e.g. host, vm Returns: The discovered objects in JSON format """ logging.debug( '[%s] Getting datastores for %s managed object of type %s', self.host, name, obj_type.__name__ ) # Find the object by it's 'name' property # and get the datastores available/used by it data = self._get_object_properties( properties=['datastore'], obj_type=obj_type, obj_property_name='name', obj_property_value=name ) if data['success'] != 0: return data # Get the name and datastore properties from the result props = data['result'][0] obj_datastores = props['datastore'] # Get a list view of the datastores available/used by # this object and collect properties view_ref = self.get_list_view(obj=obj_datastores) result = self.collect_properties( view_ref=view_ref, obj_type=pyVmomi.vim.Datastore, path_set=['name', 'info.url'] ) view_ref.DestroyView() r = { 'success': 0, 'msg': 'Successfully discovered objects', 'result': result, } logging.debug('[%s] Returning result from operation: %s', self.host, r) return r def _object_alarm_get(self, obj_type, obj_property_name, obj_property_value): """ Helper method for retrieving alarms for a single Managed Object Args: obj_type (pyVmomi.vim.*): Type of the Managed Object obj_property_name (str): Property name used for searching for the object obj_property_value (str): Property value identifying the object in question Returns: The triggered alarms for the Managed Object """ logging.debug( '[%s] Retrieving alarms for %s managed object of type %s', self.host, obj_property_value, obj_type.__name__ ) # Get the 'triggeredAlarmState' property for the managed object data = self._get_object_properties( properties=['triggeredAlarmState'], obj_type=obj_type, obj_property_name=obj_property_name, obj_property_value=obj_property_value ) if data['success'] != 0: return data result = [] props = data['result'][0] alarms = props['triggeredAlarmState'] for alarm in alarms: a = { 'key': str(alarm.key), 'info': alarm.alarm.info.name, 'time': str(alarm.time), 'entity': alarm.entity.name, 'acknowledged': alarm.acknowledged, 'overallStatus': alarm.overallStatus, 'acknowledgedByUser': alarm.acknowledgedByUser, } result.append(a) r = { 'success': 0, 'msg': 'Successfully retrieved alarms', 'result': result, } logging.debug( '[%s] Returning result from operation: %s', self.host, r ) return r def task(name, required): """ Decorator for creating new vPoller tasks Args: name (str): Name of the vPoller task required (list): A list of required message attributes """ def decorator(function): logging.debug( 'Creating task %s at %s, requiring %s', name, function, required ) @wraps(function) def wrapper(*args, **kwargs): # # TODO: Validate message before processing # return function(*args, **kwargs) VSphereAgent._add_task( name=name, function=wrapper, required=required ) return wrapper return decorator

I have already moved my vSphere Datacenter related methods as tasks using the @vpoller.agent.core.task decorator as shown below.

""" vSphere Agent Datacenter Tasks https://github.com/dnaeon/py-vpoller/tree/decorators/src/vpoller """ import pyVmomi from vpoller.agent.core import task @task(name='datacenter.discover', required=['hostname', 'name']) def datacenter_discover(agent, msg): """ Discover all vim.Datacenter managed objects Example client message would be: { "method": "datacenter.discover", "hostname": "vc01.example.org", } Example client message which also requests additional properties: { "method": "datacenter.discover", "hostname": "vc01.example.org", "properties": [ "name", "overallStatus" ] } Returns: The discovered objects in JSON format """ # Property names to be collected properties = ['name'] if 'properties' in msg and msg['properties']: properties.extend(msg['properties']) r = agent._discover_objects( properties=properties, obj_type=pyVmomi.vim.Datacenter ) return r @task(name='datacenter.get', required=['hostname', 'name']) def datacenter_get(agent, msg): """ Get properties of a single vim.Datacenter managed object Example client message would be: { "method": "datacenter.get", "hostname": "vc01.example.org", "name": "MyDatacenter", "properties": [ "name", "overallStatus" ] } Returns: The managed object properties in JSON format """ # Property names to be collected properties = ['name'] if 'properties' in msg and msg['properties']: properties.extend(msg['properties']) return agent._get_object_properties( properties=properties, obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] ) @task(name='datacenter.alarm.get', required=['hostname', 'name']) def datacenter_alarm_get(agent, msg): """ Get all alarms for a vim.Datacenter managed object Example client message would be: { "method": "datacenter.alarm.get", "hostname": "vc01.example.org", "name": "MyDatacenter" } Returns: The discovered alarms in JSON format """ result = agent._object_alarm_get( obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] ) return result

Using the @vpoller.agent.core.task decorator I am now able to create new tasks in dedicated modules, which is what the main goal was.

My concerns with this approach is whether this is Pythonic (passing around self to external functions) and whether there is a better way to do it.

Community · Accepted Answer · 2017-05-23 12:41:02Z

Your main concerns

My concerns with this approach is whether this is Pythonic (passing around self to external functions) and whether there is a better way to do it.

To paraphrase your design:

the agent contains a registry of tasks
the agent can execute a task from the registry
tasks are aware of the agent

So there is a two-way dependency between an agent and a task. This is not particularly non-Pythonic, and not necessarily bad. It would be cleaner to separate these responsibilities:

one class for a task registry
one class for the common functions that tasks need
one class for a task executor, that can execute tasks in the registry, passing to them the class with the common functions

Something like that. The idea is that there's no more circular dependency between the collaborating objects.

If this is too much work, then your simpler approach may be justifiable, as simplicity can be considered a merit of its own.

API: meaning of "success"

Your code uses success=0 to mean success, and success=1 to mean failure:

 r = { 'success': 0, 'msg': 'Successfully discovered objects', 'result': result, }

Sure, it's a UNIX tradition that the exit code of a program is 0 on success and non-zero on failure, but you're not making a program executor, and this practice is not common anywhere else. Take Python for example, falsy values like 0 or empty string or empty list are treated as False, everything else is treated as True. This API is not natural, and not Pythonic.

Consider this instead:

>>> r {'success': True, 'msg': 'Successfully discovered objects'} >>> json.dumps(r) '{"success": true, "msg": "Successfully discovered objects"}'

Also note that JSON parsers treat 0, 1, as numbers (naturally), and "true", "false" as booleans (see also this). Using the "success" property in a more natural sense will be a win-win for everyone.

API documentation

All the methods that are documented to return an object in JSON format actually return a dictionary. JSON format would be a string, for example what's returned by json.dumps(some_dict_or_other_serializable). It would be more accurate and less confusing to say:

The discovered objects in a JSON-serializable dictionary

Duplicate values in `properties`

The docstring in datacenter_discover gives this example for the msg parameter:

{ "method": "datacenter.discover", "hostname": "vc01.example.org", "properties": [ "name", "overallStatus" ] }

And then the code does this:

properties = ['name'] if 'properties' in msg and msg['properties']: properties.extend(msg['properties'])

With the given example, it looks like properties will contain "name" twice. Is that ok? Will your program work correctly with duplicate values in properties ?

Generic exceptions

All the try-except blocks use except Exception as e. That seems very generic. Aren't there more specific exception types you could catch? Usually it's best to catch the most specific exception types, to avoid truly unexpected exceptions from being masked, and for better readability for reviewers.

Minor inconsistencies

In _get_object_properties, this error message in the except seems a bit off:

 try: obj = self.get_object_by_property( property_name=obj_property_name, property_value=obj_property_value, obj_type=obj_type ) except Exception as e: return {'success': 1, 'msg': 'Cannot collect properties: %s' % e}

It looks as like a copy-paste error from other try-except blocks where self.collect_properties fails.

janos · Accepted Answer · 2015-07-04 11:34:06Z

Your code looks great, and I only have just a few tips on style. Although, to be completely honest, I'm really not familiar with some of the libraries/modules/frameworks being used here, but, I'm going to try for the best answer possible. Disclaimer: A few of these tips are borrowed from @Quill's answer.

Rather than storing return values in variables named result, and then returning result, wouldn't it be easier just to return the result directly? Here's an example of what you're currently doing.

result = agent._object_alarm_get( obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] ) return result

Rather than doing that, you can do this:

return agent._object_alarm_get( obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] )

Something about your spacing in docstrings bugs me. For example, you include an empty line at the end of each of your docstrings. While this doesn't violate PEP8, it's slightly odd, and I'd recommend not doing it.

In between your two functions datacenter_discover, datacenter_get, and datacenter_alarm_get, you only have one blank line between these three functions. PEP8 requires that two blank lines must be between these functions.

Finally, I noticed that in various places, you're using % for string formatting, over the better, newer alternative, str.format. Here's a few examples of str.format in action.

# Normal use of str.format, without positional # arguments, or named arguments. print "{} {}".format("Hello", "world") # Use of str.format with positional arguments. print "{1} {0}".format("world", "Hello") # Use of str.format with named arguments print "{word1} {word2}".format(word1="Hello", word2="world")

You do realize that you're not addressing the main questions of the OP: My concerns with this approach is whether this is Pythonic (passing around self to external functions) and whether there is a better way to do it. — janos
– janos, Commented Jul 5, 2015 at 8:31

Quill · Accepted Answer · 2015-07-06 13:18:26Z

Your code looks really nice, and as I am unfamiliar with the framework you are coding for, my review is somewhat limited, nonetheless, I'll try my best.

When you are going over items in a dictionary or array, you can the comma on the next line, so that it's easier to understand. However, trailing commas are not against Python, so leaving them as they are is no problem.

data = self.collect_properties( view_ref=view_ref, obj_type=obj_type, path_set=properties, include_mors=include_mors )

into:

data = self.collect_properties( view_ref=view_ref , obj_type=obj_type , path_set=properties , include_mors=include_mors )

Entirely up to you which you use, however, I'd personally suggest the latter.

~~As shown in the example above, you should put whitespace between your operators, for the purpose of improving readability.~~

~~Also, during mass assignment like in the example above, you should space out all the operators & variables, so that they're vertically aligned.~~

Whoops, this isn't entirely correct, PEP8, the official style guide for Python says you should have whitespace before and after Binary operators, but not keyword arguments.
See here for more information.

In this particular instance, you don't use the value of r/result other than to output, meaning, you could return it with assigning a variable, if you like:

result = agent._object_alarm_get( obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] ) return result

into:

return agent._object_alarm_get( obj_type=pyVmomi.vim.Datacenter, obj_property_name='name', obj_property_value=msg['name'] )

I would also avoid calling some results r and some result, sticking to result is more consistent and easier to understand.

In the following code snippet, please refrain from using multiline comments with # instead of """:

 @wraps(function) def wrapper(*args, **kwargs): # # TODO: Validate message before processing # return function(*args, **kwargs)

into:

# TODO: Validate message before processing

You don't really need to leave a line between your dependencies:

import pyVmomi from vpoller.agent.core import task

You should replace the following code with str.format:

 logging.info( '[%s] Discovering %s managed objects', self.host, obj_type.__name__ )

into:

logging.info("[{0}] Discovering {1} managed objects".format(self.host, obj_type.__name__))

The same for similarly constructed strings.

Other those few things, your code looks really nice, Good Work!

@ferada I don't see why they can't be. TODO: in a docstring means the exact same thing. — Ethan Bierlein
– Ethan Bierlein, Commented Jul 2, 2015 at 23:16
TODO is a personal note, @ferada is trying to say that personal notes shouldn't really be stored in a docstring, but rather a # comment — Quill
– Quill, Commented Jul 2, 2015 at 23:19

Stack Exchange Network

Virtual machine management with decorated selfies

3 Answers 3

Your main concerns

API: meaning of "success"

API documentation

Duplicate values in `properties`

Generic exceptions

Minor inconsistencies

You must log in to answer this question.

Hot Network Questions

Virtual machine management with decorated selfies

3 Answers 3

Your main concerns

API: meaning of "success"

API documentation

Duplicate values in properties

Generic exceptions

Minor inconsistencies

You must log in to answer this question.

Related

Hot Network Questions

Duplicate values in `properties`