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.

Answer 1

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.

Answer 2

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")

Answer 3

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!