From 2a84ae0fc015a43617521727ca31152066839d3c Mon Sep 17 00:00:00 2001
From: Michal Dulko <michal.dulko@intel.com>
Date: Thu, 4 Jun 2015 17:25:24 +0200
Subject: [PATCH] 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
---
 cinder/manager.py                 |  8 ++++++++
 cinder/service.py                 |  8 ++++++++
 cinder/tests/unit/test_service.py | 28 ++++++++++++++++++++++++++++
 cinder/tests/unit/test_volume.py  |  8 ++++++++
 cinder/volume/manager.py          |  9 +++++++++
 5 files changed, 61 insertions(+)

diff --git a/cinder/manager.py b/cinder/manager.py
index e14909e9e..694ffdfc8 100644
--- 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 4312702b9..13bef8dff 100644
--- 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 3144d393d..4a4c07ebe 100644
--- 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 be8e102db..82b482fb0 100644
--- 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 8f563a82d..b0cb0cce4 100644
--- 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,
-- 
2.45.2