Return a meaningful value or raise an excpetion for libvirt

Currently, there are some cases libvirt cannot inspect instance's memory
usage, and it is a normal behavior so libvirt just logs messages and
returns None. However, memory pollster doesn't check return value, so
AttributeError will be raised in such case, which leading to unnecessary
exception log messages in every cycle and for each instance, this will
bother cloud operator.

This patch uses virt.inspector exceptions for libvirt, so when there is no
valid value can be returned, we raise an exception, pollsters can catch
those exceptions and log proper messages. Since all of them are non-fatal
exception, log level is set to warn, which is excatly same as previous.

Note, this patch refactors some code of libvirt to suit this change.

Change-Id: I2c94aab90e827c75a602403d3c64fd6f67f73007
Closes-Bug: #1393415

ceilometer/compute/pollsters/disk.py

@@ -27,7 +27,7 @@ import ceilometer
 from ceilometer.compute import pollsters
 from ceilometer.compute.pollsters import util
 from ceilometer.compute.virt import inspector as virt_inspector
-from ceilometer.i18n import _
+from ceilometer.i18n import _, _LW
 from ceilometer.openstack.common import log
 from ceilometer import sample
 
@@ -119,6 +119,11 @@ class _Base(pollsters.BaseComputePollster):
             except virt_inspector.InstanceNotFoundException as err:
                 # Instance was deleted while getting samples. Ignore it.
                 LOG.debug(_('Exception while getting samples %s'), err)
+            except virt_inspector.InstanceShutOffException as e:
+                LOG.warn(_LW('Instance %(instance_id)s was shut off while '
+                             'getting samples of %(pollster)s: %(exc)s'),
+                         {'instance_id': instance.id,
+                          'pollster': self.__class__.__name__, 'exc': e})
             except ceilometer.NotImplementedError:
                 # Selected inspector does not implement this pollster.
                 LOG.debug(_('%(inspector)s does not provide data for '

ceilometer/compute/pollsters/memory.py

@@ -16,7 +16,7 @@ import ceilometer
 from ceilometer.compute import pollsters
 from ceilometer.compute.pollsters import util
 from ceilometer.compute.virt import inspector as virt_inspector
-from ceilometer.i18n import _
+from ceilometer.i18n import _, _LW
 from ceilometer.openstack.common import log
 from ceilometer import sample
 
@@ -45,6 +45,16 @@ class MemoryUsagePollster(pollsters.BaseComputePollster):
             except virt_inspector.InstanceNotFoundException as err:
                 # Instance was deleted while getting samples. Ignore it.
                 LOG.debug(_('Exception while getting samples %s'), err)
+            except virt_inspector.InstanceShutOffException as e:
+                LOG.warn(_LW('Instance %(instance_id)s was shut off while '
+                             'getting samples of %(pollster)s: %(exc)s'),
+                         {'instance_id': instance.id,
+                          'pollster': self.__class__.__name__, 'exc': e})
+            except virt_inspector.NoDataException as e:
+                LOG.warn(_LW('Cannot inspect data of %(pollster)s for '
+                             '%(instance_id)s, non-fatal reason: %(exc)s'),
+                         {'pollster': self.__class__.__name__,
+                          'instance_id': instance.id, 'exc': e})
             except ceilometer.NotImplementedError:
                 # Selected inspector does not implement this pollster.
                 LOG.debug(_('Obtaining Memory Usage is not implemented for %s'

ceilometer/compute/pollsters/net.py

@@ -26,7 +26,7 @@ from ceilometer.compute import pollsters
 from ceilometer.compute.pollsters import util
 from ceilometer.compute import util as compute_util
 from ceilometer.compute.virt import inspector as virt_inspector
-from ceilometer.i18n import _
+from ceilometer.i18n import _, _LW
 from ceilometer.openstack.common import log
 from ceilometer import sample
 
@@ -107,6 +107,11 @@ class _Base(pollsters.BaseComputePollster):
             except virt_inspector.InstanceNotFoundException as err:
                 # Instance was deleted while getting samples. Ignore it.
                 LOG.debug(_('Exception while getting samples %s'), err)
+            except virt_inspector.InstanceShutOffException as e:
+                LOG.warn(_LW('Instance %(instance_id)s was shut off while '
+                             'getting samples of %(pollster)s: %(exc)s'),
+                         {'instance_id': instance.id,
+                          'pollster': self.__class__.__name__, 'exc': e})
             except ceilometer.NotImplementedError:
                 # Selected inspector does not implement this pollster.
                 LOG.debug(_('%(inspector)s does not provide data for '

ceilometer/compute/virt/inspector.py

@@ -143,6 +143,14 @@ class InstanceNotFoundException(InspectorException):
     pass
 
 
+class InstanceShutOffException(InspectorException):
+    pass
+
+
+class NoDataException(InspectorException):
+    pass
+
+
 # Main virt inspector abstraction layering over the hypervisor API.
 #
 class Inspector(object):

ceilometer/compute/virt/libvirt/inspector.py

@@ -53,7 +53,7 @@ def retry_on_disconnect(function):
             if (e.get_error_code() == libvirt.VIR_ERR_SYSTEM_ERROR and
                 e.get_error_domain() in (libvirt.VIR_FROM_REMOTE,
                                          libvirt.VIR_FROM_RPC)):
-                LOG.debug('Connection to libvirt broken')
+                LOG.debug(_('Connection to libvirt broken'))
                 self.connection = None
                 return function(self, *args, **kwargs)
             else:
@@ -78,7 +78,7 @@ class LibvirtInspector(virt_inspector.Inspector):
             global libvirt
             if libvirt is None:
                 libvirt = __import__('libvirt')
-            LOG.debug('Connecting to libvirt: %s', self.uri)
+            LOG.debug(_('Connecting to libvirt: %s'), self.uri)
             self.connection = libvirt.openReadOnly(self.uri)
 
         return self.connection
@@ -96,13 +96,13 @@ class LibvirtInspector(virt_inspector.Inspector):
                 ex.get_error_domain() in (libvirt.VIR_FROM_REMOTE,
                                           libvirt.VIR_FROM_RPC)):
                 raise
-            msg = ("Error from libvirt while looking up instance Name "
+            msg = _("Error from libvirt while looking up instance "
-                   "%(instance_name)s UUID %(instance_uuid)s: "
+                    "<name=%(name)s, id=%(id)>: "
                     "[Error Code %(error_code)s] "
-                   "%(ex)s" % {'instance_name': instance_name,
+                    "%(ex)s") % {'name': instance_name,
-                               'instance_uuid': instance.id,
+                                 'id': instance.id,
                                  'error_code': error_code,
-                               'ex': ex})
+                                 'ex': ex}
             raise virt_inspector.InstanceNotFoundException(msg)
 
     def inspect_cpus(self, instance):
@@ -110,17 +110,23 @@ class LibvirtInspector(virt_inspector.Inspector):
         dom_info = domain.info()
         return virt_inspector.CPUStats(number=dom_info[3], time=dom_info[4])
 
-    def inspect_vnics(self, instance):
+    def _get_domain_not_shut_off_or_raise(self, instance):
         instance_name = util.instance_name(instance)
         domain = self._lookup_by_uuid(instance)
+
         state = domain.info()[0]
         if state == libvirt.VIR_DOMAIN_SHUTOFF:
-            LOG.warn(_('Failed to inspect vnics of instance Name '
+            msg = _('Failed to inspect data of instance '
-                       '%(instance_name)s UUID %(instance_uuid)s, '
+                    '<name=%(name)s, id=%(id)s>, '
-                       'domain is in state of SHUTOFF'),
+                    'domain state is SHUTOFF.') % {
-                     {'instance_name': instance_name,
+                'name': instance_name, 'id': instance.id}
-                      'instance_uuid': instance.id})
+            raise virt_inspector.InstanceShutOffException(msg)
-            return
+
+        return domain
+
+    def inspect_vnics(self, instance):
+        domain = self._get_domain_not_shut_off_or_raise(instance)
+
         tree = etree.fromstring(domain.XMLDesc(0))
         for iface in tree.findall('devices/interface'):
             target = iface.find('target')
@@ -149,16 +155,8 @@ class LibvirtInspector(virt_inspector.Inspector):
             yield (interface, stats)
 
     def inspect_disks(self, instance):
-        instance_name = util.instance_name(instance)
+        domain = self._get_domain_not_shut_off_or_raise(instance)
-        domain = self._lookup_by_uuid(instance)
+
-        state = domain.info()[0]
-        if state == libvirt.VIR_DOMAIN_SHUTOFF:
-            LOG.warn(_('Failed to inspect disks of instance Name '
-                       '%(instance_name)s UUID %(instance_uuid)s, '
-                       'domain is in state of SHUTOFF'),
-                     {'instance_name': instance_name,
-                      'instance_uuid': instance.id})
-            return
         tree = etree.fromstring(domain.XMLDesc(0))
         for device in filter(
                 bool,
@@ -175,15 +173,7 @@ class LibvirtInspector(virt_inspector.Inspector):
 
     def inspect_memory_usage(self, instance, duration=None):
         instance_name = util.instance_name(instance)
-        domain = self._lookup_by_uuid(instance)
+        domain = self._get_domain_not_shut_off_or_raise(instance)
-        state = domain.info()[0]
-        if state == libvirt.VIR_DOMAIN_SHUTOFF:
-            LOG.warn(_('Failed to inspect memory usage of instance Name '
-                       '%(instance_name)s UUID %(instance_uuid)s, '
-                       'domain is in state of SHUTOFF'),
-                     {'instance_name': instance_name,
-                      'instance_uuid': instance.id})
-            return
 
         try:
             memory_stats = domain.memoryStats()
@@ -196,15 +186,15 @@ class LibvirtInspector(virt_inspector.Inspector):
                 memory_used = memory_used / units.Ki
                 return virt_inspector.MemoryUsageStats(usage=memory_used)
             else:
-                LOG.warn(_('Failed to inspect memory usage of instance Name '
+                msg = _('Failed to inspect memory usage of instance '
-                           '%(instance_name)s UUID %(instance_uuid)s, '
+                        '<name=%(name)s, id=%(id)s>, '
-                           'can not get info from libvirt'),
+                        'can not get info from libvirt.') % {
-                         {'instance_name': instance_name,
+                    'name': instance_name, 'id': instance.id}
-                          'instance_uuid': instance.id})
+                raise virt_inspector.NoDataException(msg)
-        # memoryStats might launch an exception if the method
+        # memoryStats might launch an exception if the method is not supported
-        # is not supported by the underlying hypervisor being
+        # by the underlying hypervisor being used by libvirt.
-        # used by libvirt
         except libvirt.libvirtError as e:
-            LOG.warn(_('Failed to inspect memory usage of %(instance_uuid)s, '
+            msg = _('Failed to inspect memory usage of %(instance_uuid)s, '
-                       'can not get info from libvirt: %(error)s'),
+                    'can not get info from libvirt: %(error)s') % {
-                     {'instance_uuid': instance.id, 'error': e})
+                'instance_uuid': instance.id, 'error': e}
+            raise virt_inspector.NoDataException(msg)

ceilometer/tests/compute/pollsters/test_memory.py

@@ -31,24 +31,36 @@ class TestMemoryPollster(base.TestPollsterBase):
         next_value = iter((
             virt_inspector.MemoryUsageStats(usage=1.0),
             virt_inspector.MemoryUsageStats(usage=2.0),
+            virt_inspector.NoDataException(),
+            virt_inspector.InstanceShutOffException(),
         ))
 
         def inspect_memory_usage(instance, duration):
-            return next(next_value)
+            value = next(next_value)
+            if isinstance(value, virt_inspector.MemoryUsageStats):
+                return value
+            else:
+                raise value
 
-        (self.inspector.
+        self.inspector.inspect_memory_usage = mock.Mock(
-         inspect_memory_usage) = mock.Mock(side_effect=inspect_memory_usage)
+            side_effect=inspect_memory_usage)
 
         mgr = manager.AgentManager()
         pollster = memory.MemoryUsagePollster()
 
-        def _verify_memory_metering(expected_memory_mb):
+        @mock.patch('ceilometer.compute.pollsters.memory.LOG')
-            cache = {}
+        def _verify_memory_metering(expected_count, expected_memory_mb, mylog):
-            samples = list(pollster.get_samples(mgr, cache, [self.instance]))
+            samples = list(pollster.get_samples(mgr, {}, [self.instance]))
-            self.assertEqual(1, len(samples))
+            self.assertEqual(expected_count, len(samples))
+            if expected_count > 0:
                 self.assertEqual(set(['memory.usage']),
                                  set([s.name for s in samples]))
                 self.assertEqual(expected_memory_mb, samples[0].volume)
+            else:
+                self.assertEqual(1, mylog.warn.call_count)
+            self.assertEqual(0, mylog.exception.call_count)
 
-        _verify_memory_metering(1.0)
+        _verify_memory_metering(1, 1.0)
-        _verify_memory_metering(2.0)
+        _verify_memory_metering(1, 2.0)
+        _verify_memory_metering(0, 0)
+        _verify_memory_metering(0, 0)

ceilometer/tests/compute/virt/libvirt/test_inspector.py

@@ -180,8 +180,9 @@ class TestLibvirtInspection(base.BaseTestCase):
                                mock.patch.object(self.domain, 'info',
                                                  return_value=(5L, 0L, 0L,
                                                                2L, 999999L))):
-            interfaces = list(self.inspector.inspect_vnics(self.instance))
+            inspect = self.inspector.inspect_vnics
-            self.assertEqual([], interfaces)
+            self.assertRaises(virt_inspector.InstanceShutOffException,
+                              list, inspect(self.instance))
 
     def test_inspect_disks(self):
         dom_xml = """
@@ -228,8 +229,9 @@ class TestLibvirtInspection(base.BaseTestCase):
                                mock.patch.object(self.domain, 'info',
                                                  return_value=(5L, 0L, 0L,
                                                                2L, 999999L))):
-            disks = list(self.inspector.inspect_disks(self.instance))
+            inspect = self.inspector.inspect_disks
-            self.assertEqual([], disks)
+            self.assertRaises(virt_inspector.InstanceShutOffException,
+                              list, inspect(self.instance))
 
     def test_inspect_memory_usage(self):
         fake_memory_stats = {'available': 51200L, 'unused': 25600L}
@@ -252,9 +254,9 @@ class TestLibvirtInspection(base.BaseTestCase):
             with mock.patch.object(self.domain, 'info',
                                    return_value=(5L, 0L, 0L,
                                                  2L, 999999L)):
-                memory = self.inspector.inspect_memory_usage(
+                self.assertRaises(virt_inspector.InstanceShutOffException,
+                                  self.inspector.inspect_memory_usage,
                                   self.instance)
-                self.assertIsNone(memory)
 
     def test_inspect_memory_usage_with_empty_stats(self):
         connection = self.inspector.connection
@@ -265,9 +267,9 @@ class TestLibvirtInspection(base.BaseTestCase):
                                                  2L, 999999L)):
                 with mock.patch.object(self.domain, 'memoryStats',
                                        return_value={}):
-                    memory = self.inspector.inspect_memory_usage(
+                    self.assertRaises(virt_inspector.NoDataException,
+                                      self.inspector.inspect_memory_usage,
                                       self.instance)
-                    self.assertIsNone(memory)
 
 
 class TestLibvirtInspectionWithError(base.BaseTestCase):