Don't send heartbeats if Manager reports a problem

This commit adds is_working method to base Manager class that can be
used to indicate that service has some problems and isn't running
correctly. This is used to block refreshing Service heartbeats if
Manager will return False from is_working.

The method is implemented in c-vol Manager to return initialization
state of the underlying volume driver.

Closes-Bug: 1446750
DocImpact

Change-Id: I809b2b777a0f84d12f39f2233920ec961f74f846

diff --git a/cinder/manager.py b/cinder/manager.py
index e14909e9eed68450334c4f0af7c5e300db351577..694ffdfc834785358c1abc1287ea98084565b18b 100644 (file)
--- a/cinder/manager.py
+++ b/cinder/manager.py
@@ -112,6 +112,14 @@ class Manager(base.Base, periodic_task.PeriodicTasks):
             config[key] = CONF.get(key, None)
         return config
 
+    def is_working(self):
+        """Method indicating if service is working correctly.
+
+        This method is supposed to be overriden by subclasses and return if
+        manager is working correctly.
+        """
+        return True
+
 
 class SchedulerDependentManager(Manager):
     """Periodically send capability updates to the Scheduler services.

diff --git a/cinder/service.py b/cinder/service.py
index 4312702b9f55f1de3c38896a865244d1aab367a5..13bef8dff206910c31df9db658e10f564229989d 100644 (file)
--- a/cinder/service.py
+++ b/cinder/service.py
@@ -287,6 +287,14 @@ class Service(service.Service):
 
     def report_state(self):
         """Update the state of this service in the datastore."""
+        if not self.manager.is_working():
+            # NOTE(dulek): If manager reports a problem we're not sending
+            # heartbeats - to indicate that service is actually down.
+            LOG.error(_LE('Manager for service %s is reporting problems, skip '
+                          'sending heartbeat. Service will appear "down".'),
+                      self.binary)
+            return
+
         ctxt = context.get_admin_context()
         zone = CONF.storage_availability_zone
         state_catalog = {}

diff --git a/cinder/tests/unit/test_service.py b/cinder/tests/unit/test_service.py
index 3144d393dcee194f60a4b9b301d6ae2198936b77..4a4c07ebe5a720f230c0405359d47b8c63bbd849 100644 (file)
--- a/cinder/tests/unit/test_service.py
+++ b/cinder/tests/unit/test_service.py
@@ -149,6 +149,7 @@ class ServiceTestCase(test.TestCase):
             serv.start()
             serv.report_state()
             self.assertTrue(serv.model_disconnected)
+            self.assertFalse(mock_db.service_update.called)
 
     def test_report_state_newly_connected(self):
         host = 'foo'
@@ -176,6 +177,33 @@ class ServiceTestCase(test.TestCase):
             serv.report_state()
 
             self.assertFalse(serv.model_disconnected)
+            self.assertTrue(mock_db.service_update.called)
+
+    def test_report_state_manager_not_working(self):
+        host = 'foo'
+        binary = 'bar'
+        topic = 'test'
+        service_ref = {'host': host,
+                       'binary': binary,
+                       'topic': topic,
+                       'report_count': 0,
+                       'availability_zone': 'nova',
+                       'id': 1}
+        with mock.patch.object(service, 'db') as mock_db:
+            mock_db.service_get.return_value = service_ref
+
+            serv = service.Service(
+                host,
+                binary,
+                topic,
+                'cinder.tests.unit.test_service.FakeManager'
+            )
+            serv.manager.is_working = mock.Mock(return_value=False)
+            serv.start()
+            serv.report_state()
+
+            serv.manager.is_working.assert_called_once_with()
+            self.assertFalse(mock_db.service_update.called)
 
     def test_service_with_long_report_interval(self):
         self.override_config('service_down_time', 10)

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index be8e102db85b6e63bfc7939b3ea5634bbaee7515..82b482fb0cce90807efd67dce47a3e4660d7bcf9 100644 (file)
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -330,6 +330,14 @@ class VolumeTestCase(BaseVolumeTestCase):
             manager.init_host()
         self.assertEqual(0, mock_add_p_task.call_count)
 
+    def test_is_working(self):
+        # By default we have driver mocked to be initialized...
+        self.assertTrue(self.volume.is_working())
+
+        # ...lets switch it and check again!
+        self.volume.driver._initialized = False
+        self.assertFalse(self.volume.is_working())
+
     def test_create_volume_fails_with_creating_and_downloading_status(self):
         """Test init_host in case of volume.
 

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 8f563a82dc127d51b2d2afc23681bf1bc6faaf40..b0cb0cce45651dbd997b26fb49573bf5418ee0fc 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -394,6 +394,15 @@ class VolumeManager(manager.SchedulerDependentManager):
                  resource={'type': 'driver',
                            'id': self.driver.__class__.__name__})
 
+    def is_working(self):
+        """Return if Manager is ready to accept requests.
+
+        This is to inform Service class that in case of volume driver
+        initialization failure the manager is actually down and not ready to
+        accept any requests.
+        """
+        return self.driver.initialized
+
     def create_volume(self, context, volume_id, request_spec=None,
                       filter_properties=None, allow_reschedule=True,
                       snapshot_id=None, image_id=None, source_volid=None,