Change in vdsm[master]: GuestIF Refactoring

[Vinzenz Feenstra]

Vinzenz Feenstra has uploaded a new change for review.

Change subject: GuestIF Refactoring
......................................................................

GuestIF Refactoring

Change-Id: Ib357d770a26ef1dc80b89a32bf6808551a7d622d
Signed-off-by: Vinzenz Feenstra 
---
M vdsm/guestIF.py
1 file changed, 114 insertions(+), 76 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/18/24618/1

diff --git a/vdsm/guestIF.py b/vdsm/guestIF.py
index 229a55d..96ad68c 100644
--- a/vdsm/guestIF.py
+++ b/vdsm/guestIF.py
@@ -39,6 +39,115 @@
     union(set(range(0x86, 0x9F + 1)))

+class UnknownMessageError(Exception):
+    def __init__(self, message, args):
+        Exception.__init__(self, 'Unknown or unsupported guest agent message '
+                                 '"%s" received with args "%s"' % (message,
+                                                                   str(args)))
+
+
+class MessageHandler(object):
+    def __init__(self, agent):
+        self.log = agent.log
+        self._agent = agent
+
+    def __call__(self, message, args):
+        handler = self.getattr(self, message.replace('-', '_'), None)
+        if handler:
+            handler(args)
+        else:
+            raise UnknownMessageError(message, args)
+
+    def applications(self, args):
+        self._agent.guestInfo['appsList'] = args['applications']
+
+    def fqdn(self, args):
+        self._agent.guestInfo['guestFQDN'] = args['fqdn']
+
+    def host_name(self, args):
+        self._agent.guestInfo['guestName'] = args['name']
+
+    def os_version(self, args):
+        self._agent.guestInfo['guestOs'] = args['version']
+
+    def session_lock(self, args):
+        self.agent.guestInfo['session'] = 'Locked'
+
+    def session_logoff(self, args):
+        self.agent.guestInfo['session'] = 'LoggedOff'
+
+    def session_logon(self, args):
+        self.agent.guestInfo['session'] = 'UserLoggedOn'
+
+    def session_unlock(self, args):
+        self.agent.guestInfo['session'] = 'Active'
+
+    def session_shutdown(self, args):
+        self.log.debug('Guest system shuts down')
+
+    def session_startup(self, args):
+        self.log.debug('Guest system started or restarted')
+
+    def uninstalled(self, args):
+        self.log.debug('Guest agent was uninstalled')
+        self._agent.guestInfo['appsList'] = []
+
+    def heartbeat(self, args):
+        self._agent.guestStatus = 'Up'
+        self._agent.guestInfo['memUsage'] = int(args['free-ram'])
+        # ovirt-guest-agent reports the following fields in 'memory-stat':
+        # 'mem_total', 'mem_free', 'mem_unused', 'swap_in', 'swap_out',
+        # 'pageflt' and 'majflt'
+        if 'memory-stat' in args:
+            for (k, v) in args['memory-stat'].iteritems():
+                # Convert the value to string since 64-bit integer is not
+                # supported in XMLRPC
+                self._agent.guestInfo['memoryStats'][k] = str(v)
+
+        if 'apiVersion' in args:
+            # The guest agent supports API Versioning
+            self._agent._handleAPIVersion(args['apiVersion'])
+        elif self._agent.effectiveApiVersion != _IMPLICIT_API_VERSION_ZERO:
+            # Older versions of the guest agent (before the introduction
+            # of API versioning) do not report this field
+            # Disable the API if not already disabled (e.g. after
+            # downgrade of the guest agent)
+            self.log.debug("API versioning no longer reported by guest.")
+            self._agent.effectiveApiVersion = _IMPLICIT_API_VERSION_ZERO
+
+    def network_interfaces(self, args):
+        interfaces = []
+        old_ips = ''
+        for iface in args['interfaces']:
+            iface['inet'] = iface.get('inet', [])
+            iface['inet6'] = iface.get('inet6', [])
+            interfaces.append(iface)
+            # Provide the old information which includes
+            # only the IP addresses.
+            old_ips += ' '.join(iface['inet']) + ' '
+        self._agent.guestInfo['netIfaces'] = interfaces
+        self._agent.guestInfo['guestIPs'] = old_ips.strip()
+
+    def active_user(self, args):
+        currentUser = args['name']
+        if ((currentUser != self._agent.guestInfo['username']) and
+            not (currentUser == 'Unknown' and
+                 self._agent.guestInfo['username'] == 'None')):
+            self._agent.guestInfo['username'] = currentUser
+            self._agent.guestInfo['lastLogin'] = time.time()
+        self.log.debug("username: %s", repr(self.guestInfo['username']))
+
+    def disks_usage(self, args):
+        disks = []
+        for disk in args['disks']:
+            # Converting to string because XML-RPC doesn't support 64-bit
+            # integers.
+            disk['total'] = str(disk['total'])
+            disk['used'] = str(disk['used'])
+            disks.append(disk)
+        self._agent.guestInfo['disksUsage'] = disks
+
+
 def _filterXmlChars(u):
     """
     The set of characters allowed in XML documents is described in
@@ -109,6 +218,7 @@

def __init__(self, socketName, channelListener, log, user='Unknown',
                  ips='', connect=True):
+        self.handler = MessageHandler(self)
         self.effectiveApiVersion = _IMPLICIT_API_VERSION_ZERO
         self.log = log
         self._socketName = socketName
@@ -223,82 +333,10 @@
         self.log.log(logging.TRACE, "Guest's message %s: %s", message, args)
         if self.guestStatus is None:
             self.guestStatus = 'Up'
-        if message == 'heartbeat':
-            self.guestStatus = 'Up'
-            self.guestInfo['memUsage'] = int(args['free-ram'])
-            # ovirt-guest-agent reports the following fields in 'memory-stat':
-            # 'mem_total', 'mem_free', 'mem_unused', 'swap_in', 'swap_out',
-            # 'pageflt' and 'majflt'
-            if 'memory-stat' in args:
-                for (k, v) in args['memory-stat'].iteritems():
-                    # Convert the value to string since 64-bit integer is not
-                    # supported in XMLRPC
-                    self.guestInfo['memoryStats'][k] = str(v)
-
-            if 'apiVersion' in args:
-                # The guest agent supports API Versioning
-                self._handleAPIVersion(args['apiVersion'])
-            elif self.effectiveApiVersion != _IMPLICIT_API_VERSION_ZERO:
-                # Older versions of the guest agent (before the introduction
-                # of API versioning) do not report this field
-                # Disable the API if not already disabled (e.g. after
-                # downgrade of the guest agent)
-                self.log.debug("API versioning no longer reported by guest.")
-                self.effectiveApiVersion = _IMPLICIT_API_VERSION_ZERO
-        elif message == 'host-name':
-            self.guestInfo['guestName'] = args['name']
-        elif message == 'os-version':
-            self.guestInfo['guestOs'] = args['version']
-        elif message == 'network-interfaces':
-            interfaces = []
-            old_ips = ''
-            for iface in args['interfaces']:
-                iface['inet'] = iface.get('inet', [])
-                iface['inet6'] = iface.get('inet6', [])
-                interfaces.append(iface)
-                # Provide the old information which includes
-                # only the IP addresses.
-                old_ips += ' '.join(iface['inet']) + ' '
-            self.guestInfo['netIfaces'] = interfaces
-            self.guestInfo['guestIPs'] = old_ips.strip()
-        elif message == 'applications':
-            self.guestInfo['appsList'] = args['applications']
-        elif message == 'active-user':
-            currentUser = args['name']
-            if ((currentUser != self.guestInfo['username']) and
-                not (currentUser == 'Unknown' and
-                     self.guestInfo['username'] == 'None')):
-                self.guestInfo['username'] = currentUser
-                self.guestInfo['lastLogin'] = time.time()
-            self.log.debug("username: %s", repr(self.guestInfo['username']))
-        elif message == 'session-logon':
-            self.guestInfo['session'] = "UserLoggedOn"
-        elif message == 'session-lock':
-            self.guestInfo['session'] = "Locked"
-        elif message == 'session-unlock':
-            self.guestInfo['session'] = "Active"
-        elif message == 'session-logoff':
-            self.guestInfo['session'] = "LoggedOff"
-        elif message == 'uninstalled':
-            self.log.debug("RHEV agent was uninstalled.")
-            self.guestInfo['appsList'] = []
-        elif message == 'session-startup':
-            self.log.debug("Guest system is started or restarted.")
-        elif message == 'fqdn':
-            self.guestInfo['guestFQDN'] = args['fqdn']
-        elif message == 'session-shutdown':
-            self.log.debug("Guest system shuts down.")
-        elif message == 'disks-usage':
-            disks = []
-            for disk in args['disks']:
-                # Converting to string because XML-RPC doesn't support 64-bit
-                # integers.
-                disk['total'] = str(disk['total'])
-                disk['used'] = str(disk['used'])
-                disks.append(disk)
-            self.guestInfo['disksUsage'] = disks
-        else:
-            self.log.error('Unknown message type %s', message)
+        try:
+            self.handler(message, args)
+        except UnknownMessageError as e:
+            self.log.error(e)

def stop(self):
         self._stopped = True

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7299/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6397/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7181/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7300/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6398/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7182/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7307/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6405/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7189/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 3:

(3 comments)

http://gerrit.ovirt.org/#/c/24618/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 51:         self.log = agent.log
Line 52:         self._agent = agent
Line 53: 
Line 54:     def __call__(self, message, args):
Line 55:         handler = getattr(self, message.replace('-', '_'), None)
maybe this is a good case for (something inspired to) the
Null object pattern:

http://en.wikipedia.org/wiki/Null_Object_pattern

probably just a default, one-line, handler which raises UnknownMessageError will be enough.
Line 56:         if handler:
Line 57:             handler(args)
Line 58:         else:
Line 59:             raise UnknownMessageError(message, args)

Line 70:     def os_version(self, args):
Line 71:         self._agent.guestInfo['guestOs'] = args['version']
Line 72: 
Line 73:     def session_lock(self, args):
Line 74:         self.agent.guestInfo['session'] = 'Locked'
typo? here and below
Line 75: 
Line 76:     def session_logoff(self, args):
Line 77:         self.agent.guestInfo['session'] = 'LoggedOff'
Line 78:

Line 76:     def session_logoff(self, args):
Line 77:         self.agent.guestInfo['session'] = 'LoggedOff'
Line 78: 
Line 79:     def session_logon(self, args):
Line 80:         self.agent.guestInfo['session'] = 'UserLoggedOn'
all the above are maybe repleacable with some generic dict juggling, but I can't focus on a specific suggestion :(
Line 81: 
Line 82:     def session_unlock(self, args):
Line 83:         self.agent.guestInfo['session'] = 'Active'
Line 84:

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 3:

(3 comments)

http://gerrit.ovirt.org/#/c/24618/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 51:         self.log = agent.log
Line 52:         self._agent = agent
Line 53: 
Line 54:     def __call__(self, message, args):
Line 55:         handler = getattr(self, message.replace('-', '_'), None)
> maybe this is a good case for (something inspired to) the

Done
Line 56:         if handler:
Line 57:             handler(args)
Line 58:         else:
Line 59:             raise UnknownMessageError(message, args)

Line 70:     def os_version(self, args):
Line 71:         self._agent.guestInfo['guestOs'] = args['version']
Line 72: 
Line 73:     def session_lock(self, args):
Line 74:         self.agent.guestInfo['session'] = 'Locked'
> typo? here and below

Done
Line 75: 
Line 76:     def session_logoff(self, args):
Line 77:         self.agent.guestInfo['session'] = 'LoggedOff'
Line 78:

Line 76:     def session_logoff(self, args):
Line 77:         self.agent.guestInfo['session'] = 'LoggedOff'
Line 78: 
Line 79:     def session_logon(self, args):
Line 80:         self.agent.guestInfo['session'] = 'UserLoggedOn'
> all the above are maybe repleacable with some generic dict juggling, but I

The only thing I could imagine is something like:

session_logoff = _session_fun('LoggedOff')

os_version = _assign_fun('guestOs', 'version')

applications = _assign_fun('appsList', 'applications')

but this makes it rather hacky than nice

There will be on top of this refactoring some unit tests, so issues like this typo should be caught, and the implementation is rather straight forward than saving some lines here and there.
Line 81: 
Line 82:     def session_unlock(self, args):
Line 83:         self.agent.guestInfo['session'] = 'Active'
Line 84:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7313/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6411/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7195/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/24618/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 76:     def session_logoff(self, args):
Line 77:         self.agent.guestInfo['session'] = 'LoggedOff'
Line 78: 
Line 79:     def session_logon(self, args):
Line 80:         self.agent.guestInfo['session'] = 'UserLoggedOn'
> The only thing I could imagine is something like:

I agree.
Line 81: 
Line 82:     def session_unlock(self, args):
Line 83:         self.agent.guestInfo['session'] = 'Active'
Line 84:

Francesco Romani has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/24618/4/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 42: class UnknownMessageError(Exception):
Line 43:     def __init__(self, msg, args):
Line 44:         Exception.__init__(self, 'Unknown or unsupported guest agent message '
Line 45:                                  '"%s" received with args "%s"' % (msg,
Line 46:                                                                    str(args)))
Please use super() here
Line 47: 
Line 48: 
Line 49: class MessageHandler(object):
Line 50:     def __init__(self, agent):

Itamar Heim has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 4:

ping

Itamar Heim has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 4:

ping

[Jenkins CI RO]

Jenkins CI RO has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 4:

Abandoned due to no activity - please restore if still relevant

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: GuestIF Refactoring
......................................................................

Abandoned

Abandoned due to no activity - please restore if still relevant

gerrit-hooks has posted comments on this change.

Change subject: GuestIF Refactoring
......................................................................

Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found