Virtual machine management with decorated selfies

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_ta

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.

StackExchange Code Review Q#70163, answer score: 5