Add disabled kwarg to service_get_all_by_topic

service_get_all_by_topic disabled kwarg allows results to include
where disabled is True or False or both.

Updated code where service_get_all_by_topic is used to make use of
disabled kwarg instead of filtering afterwards.

Removed logging of disabled services in host_manager.

Closes-Bug: #1285673
Change-Id: I748461e64acd4e9563c4016781c5e1d4b116dac8

diff --git a/cinder/backup/api.py b/cinder/backup/api.py
index 544cde7238d299027749c9ed1cf5e6f2fcc5a3a2..37851e4bd51248662564b49be2210d3965b6676a 100644 (file)
--- a/cinder/backup/api.py
+++ b/cinder/backup/api.py
@@ -88,10 +88,12 @@ class API(base.Base):
         """Check if there is a backup service available."""
         topic = CONF.backup_topic
         ctxt = context.get_admin_context()
-        services = self.db.service_get_all_by_topic(ctxt, topic)
+        services = self.db.service_get_all_by_topic(ctxt,
+                                                    topic,
+                                                    disabled=False)
         for srv in services:
             if (srv['availability_zone'] == volume['availability_zone'] and
-                    srv['host'] == volume_host and not srv['disabled'] and
+                    srv['host'] == volume_host and
                     utils.service_is_up(srv)):
                 return True
         return False

diff --git a/cinder/db/api.py b/cinder/db/api.py
index 6d05ba7665d74f9e24a093693bac39e1d675cc3e..b81b04a38114ebc27952e59d096cb717d35bb7ce 100644 (file)
--- a/cinder/db/api.py
+++ b/cinder/db/api.py
@@ -99,9 +99,9 @@ def service_get_all(context, disabled=None):
     return IMPL.service_get_all(context, disabled)
 
 
-def service_get_all_by_topic(context, topic):
+def service_get_all_by_topic(context, topic, disabled=None):
     """Get all services for a given topic."""
-    return IMPL.service_get_all_by_topic(context, topic)
+    return IMPL.service_get_all_by_topic(context, topic, disabled=disabled)
 
 
 def service_get_all_by_host(context, host):

diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index 0add648bb7d78551fcf14ee72348940df7a23630..219740b1c8f5e16684a3b27e4b6e68baab57f2fb 100644 (file)
--- a/cinder/db/sqlalchemy/api.py
+++ b/cinder/db/sqlalchemy/api.py
@@ -279,12 +279,15 @@ def service_get_all(context, disabled=None):
 
 
 @require_admin_context
-def service_get_all_by_topic(context, topic):
-    return model_query(
+def service_get_all_by_topic(context, topic, disabled=None):
+    query = model_query(
         context, models.Service, read_deleted="no").\
-        filter_by(disabled=False).\
-        filter_by(topic=topic).\
-        all()
+        filter_by(topic=topic)
+
+    if disabled is not None:
+        query = query.filter_by(disabled=disabled)
+
+    return query.all()
 
 
 @require_admin_context

diff --git a/cinder/scheduler/host_manager.py b/cinder/scheduler/host_manager.py
index 5da8101faae140413ea73a94ca983af6fa776046..5c68fbd32d5869b6110a0815fabfc0920de2e1b1 100644 (file)
--- a/cinder/scheduler/host_manager.py
+++ b/cinder/scheduler/host_manager.py
@@ -289,13 +289,14 @@ class HostManager(object):
 
         # Get resource usage across the available volume nodes:
         topic = CONF.volume_topic
-        volume_services = db.service_get_all_by_topic(context, topic)
+        volume_services = db.service_get_all_by_topic(context,
+                                                      topic,
+                                                      disabled=False)
         active_hosts = set()
         for service in volume_services:
             host = service['host']
-            if not utils.service_is_up(service) or service['disabled']:
-                LOG.warn(_("volume service is down or disabled. "
-                           "(host: %s)") % host)
+            if not utils.service_is_up(service):
+                LOG.warn(_("volume service is down. (host: %s)") % host)
                 continue
             capabilities = self.service_states.get(host, None)
             host_state = self.host_state_map.get(host)

diff --git a/cinder/tests/api/contrib/test_backups.py b/cinder/tests/api/contrib/test_backups.py
index 432db8f0df29144eda0f084d7c89e232f7ab0cbd..3cda44a48876d690d3fcf6d261ea15d3423290d1 100644 (file)
--- a/cinder/tests/api/contrib/test_backups.py
+++ b/cinder/tests/api/contrib/test_backups.py
@@ -512,10 +512,7 @@ class BackupsAPITestCase(test.TestCase):
         az_not_match = [{'availability_zone': "strange_az", 'host': test_host,
                          'disabled': 0, 'updated_at': timeutils.utcnow()}]
         #service disabled
-        disabled_service = [{'availability_zone': "fake_az",
-                             'host': test_host,
-                             'disabled': 1,
-                             'updated_at': timeutils.utcnow()}]
+        disabled_service = []
 
         #dead service that last reported at 20th century
         dead_service = [{'availability_zone': "fake_az", 'host': alt_host,

diff --git a/cinder/tests/scheduler/fakes.py b/cinder/tests/scheduler/fakes.py
index ed6437d0bc85c72cfc9d876cda0abc03fe764fba..61bafa7610d1ff4cbd2e4d020fa402d67a3f6d5f 100644 (file)
--- a/cinder/tests/scheduler/fakes.py
+++ b/cinder/tests/scheduler/fakes.py
@@ -66,7 +66,7 @@ class FakeHostState(host_manager.HostState):
             setattr(self, key, val)
 
 
-def mock_host_manager_db_calls(mock_obj):
+def mock_host_manager_db_calls(mock_obj, disabled=None):
     services = [
         dict(id=1, host='host1', topic='volume', disabled=False,
              availability_zone='zone1', updated_at=timeutils.utcnow()),
@@ -80,4 +80,8 @@ def mock_host_manager_db_calls(mock_obj):
         dict(id=5, host='host5', topic='volume', disabled=True,
              availability_zone='zone4', updated_at=timeutils.utcnow()),
     ]
-    mock_obj.return_value = services
+    if disabled is None:
+        mock_obj.return_value = services
+    else:
+        mock_obj.return_value = [service for service in services
+                                 if service['disabled'] == disabled]

diff --git a/cinder/tests/scheduler/test_allocated_capacity_weigher.py b/cinder/tests/scheduler/test_allocated_capacity_weigher.py
index 8a02ac92f257f5f3e387e811c75db6df51a6c2ce..043a90069c5109f8298aa505b3c2ccf17ce4e769 100644 (file)
--- a/cinder/tests/scheduler/test_allocated_capacity_weigher.py
+++ b/cinder/tests/scheduler/test_allocated_capacity_weigher.py
@@ -42,12 +42,13 @@ class AllocatedCapacityWeigherTestCase(test.TestCase):
                                                        weight_properties)[0]
 
     @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic')
-    def _get_all_hosts(self, _mock_service_get_all_by_topic):
+    def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False):
         ctxt = context.get_admin_context()
-        fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic)
+        fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic,
+                                         disabled=disabled)
         host_states = self.host_manager.get_all_host_states(ctxt)
         _mock_service_get_all_by_topic.assert_called_once_with(
-            ctxt, CONF.volume_topic)
+            ctxt, CONF.volume_topic, disabled=disabled)
         return host_states
 
     def test_default_of_spreading_first(self):

diff --git a/cinder/tests/scheduler/test_capacity_weigher.py b/cinder/tests/scheduler/test_capacity_weigher.py
index 680dac1179629819716fa271705c1f33b5570726..28e266ef8f004323931b4b1d78b1635fe96d2fe7 100644 (file)
--- a/cinder/tests/scheduler/test_capacity_weigher.py
+++ b/cinder/tests/scheduler/test_capacity_weigher.py
@@ -43,12 +43,13 @@ class CapacityWeigherTestCase(test.TestCase):
                                                        weight_properties)[0]
 
     @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic')
-    def _get_all_hosts(self, _mock_service_get_all_by_topic):
+    def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False):
         ctxt = context.get_admin_context()
-        fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic)
+        fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic,
+                                         disabled=disabled)
         host_states = self.host_manager.get_all_host_states(ctxt)
         _mock_service_get_all_by_topic.assert_called_once_with(
-            ctxt, CONF.volume_topic)
+            ctxt, CONF.volume_topic, disabled=disabled)
         return host_states
 
     def test_default_of_spreading_first(self):

diff --git a/cinder/tests/scheduler/test_host_manager.py b/cinder/tests/scheduler/test_host_manager.py
index 04ba8506dfdb189ea08a0439a38300e0e9a66139..7140fa1a7bfe69afe43d34f71fbb6bcf2f71f118 100644 (file)
--- a/cinder/tests/scheduler/test_host_manager.py
+++ b/cinder/tests/scheduler/test_host_manager.py
@@ -133,9 +133,6 @@ class HostManagerTestCase(test.TestCase):
                  availability_zone='zone2', updated_at=timeutils.utcnow()),
             dict(id=4, host='host4', topic='volume', disabled=False,
                  availability_zone='zone3', updated_at=timeutils.utcnow()),
-            # service on host5 is disabled
-            dict(id=5, host='host5', topic='volume', disabled=True,
-                 availability_zone='zone4', updated_at=timeutils.utcnow()),
         ]
 
         # First test: service_is_up is always True, host5 is disabled
@@ -144,15 +141,15 @@ class HostManagerTestCase(test.TestCase):
         _mock_warning = mock.Mock()
         host_manager.LOG.warn = _mock_warning
 
-        # Get all states, make sure host5 is reported as down/disabled
+        # Get all states
         self.host_manager.get_all_host_states(context)
-        _mock_service_get_all_by_topic.assert_called_with(context, topic)
+        _mock_service_get_all_by_topic.assert_called_with(context,
+                                                          topic,
+                                                          disabled=False)
         expected = []
         for service in services:
             expected.append(mock.call(service))
         self.assertEqual(expected, _mock_service_is_up.call_args_list)
-        _mock_warning.assert_called_with("volume service is down or disabled. "
-                                         "(host: host5)")
 
         # Get host_state_map and make sure we have the first 4 hosts
         host_state_map = self.host_manager.host_state_map
@@ -164,20 +161,22 @@ class HostManagerTestCase(test.TestCase):
 
         # Second test: Now service_is_up returns False for host4
         _mock_service_is_up.reset_mock()
-        _mock_service_is_up.side_effect = [True, True, True, False, True]
+        _mock_service_is_up.side_effect = [True, True, True, False]
         _mock_service_get_all_by_topic.reset_mock()
         _mock_warning.reset_mock()
 
-        # Get all states, make sure hosts 4 and 5 is reported as down/disabled
+        # Get all states, make sure host 4 is reported as down
         self.host_manager.get_all_host_states(context)
-        _mock_service_get_all_by_topic.assert_called_with(context, topic)
+        _mock_service_get_all_by_topic.assert_called_with(context,
+                                                          topic,
+                                                          disabled=False)
         expected = []
         for service in services:
             expected.append(mock.call(service))
         self.assertEqual(expected, _mock_service_is_up.call_args_list)
         expected = []
-        for num in ['4', '5']:
-            expected.append(mock.call("volume service is down or disabled. "
+        for num in ['4']:
+            expected.append(mock.call("volume service is down. "
                                       "(host: host" + num + ")"))
         self.assertEqual(expected, _mock_warning.call_args_list)
 

diff --git a/cinder/tests/test_db_api.py b/cinder/tests/test_db_api.py
index f11d9156bebd6da4abf1df4201d195a888cd5a43..532ba5a78f793bdb904c4ed171b19dcdd958453b 100644 (file)
--- a/cinder/tests/test_db_api.py
+++ b/cinder/tests/test_db_api.py
@@ -188,11 +188,11 @@ class DBAPIServiceTestCase(BaseTest):
         values = [
             {'host': 'host1', 'topic': 't1'},
             {'host': 'host2', 'topic': 't1'},
-            {'disabled': True, 'topic': 't1'},
+            {'host': 'host4', 'disabled': True, 'topic': 't1'},
             {'host': 'host3', 'topic': 't2'}
         ]
         services = [self._create_service(vals) for vals in values]
-        expected = services[:2]
+        expected = services[:3]
         real = db.service_get_all_by_topic(self.ctxt, 't1')
         self._assertEqualListsOfObjects(expected, real)
 

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 088f95c250d61e7e364088d7cf7df720cc01bfa9..1ce4ecbb0f05c6fdf727417d42fe0faf7a1cd6d8 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -793,7 +793,9 @@ class API(base.Base):
         # Make sure the host is in the list of available hosts
         elevated = context.elevated()
         topic = CONF.volume_topic
-        services = self.db.service_get_all_by_topic(elevated, topic)
+        services = self.db.service_get_all_by_topic(elevated,
+                                                    topic,
+                                                    disabled=False)
         found = False
         for service in services:
             if utils.service_is_up(service) and service['host'] == host: