Use get_by_args instead of host_and_topic

The replication v2.1 code (cheesecake) requires a number
of service queries.  The default by_host_and_topic however
doesn't include an interface to get disabled services which
is required at times.

Rather than adding a new version of get_by_host_and_topic and
introduce the arg, we probably shouldn't have been using this
particular query at all.  The only service we're interested in
is the cinder-volume service, granted in most cases volume_topic
is set to this as a default but we shouldn't be relying on it.

This patch changes the get_by_host_and_topic to use the more
appropriate (in this case) get_by_args and specifically requests
the cinder-volume binary.

Note that for manage_existing, we check the resturned service and
ensure that it disabled == False, otherwise we raise an exception.

Change-Id: I2e8c5142c589af6c1ddadeb661f86e7194faac7a
Closes-Bug: #1552006

diff --git a/cinder/tests/unit/api/contrib/test_snapshot_manage.py b/cinder/tests/unit/api/contrib/test_snapshot_manage.py
index a6ff239984da1f5c5a6fd78ef2c1c4e834dfe677..3c2787ed9347724169bd95fab45070fedf5d96a0 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_snapshot_manage.py
+++ b/cinder/tests/unit/api/contrib/test_snapshot_manage.py
@@ -68,7 +68,7 @@ class SnapshotManageTest(test.TestCase):
 
     @mock.patch('cinder.volume.rpcapi.VolumeAPI.manage_existing_snapshot')
     @mock.patch('cinder.volume.api.API.create_snapshot_in_db')
-    @mock.patch('cinder.db.service_get_by_host_and_topic')
+    @mock.patch('cinder.db.service_get_by_args')
     def test_manage_snapshot_ok(self, mock_db,
                                 mock_create_snapshot, mock_rpcapi):
         """Test successful manage volume execution.
@@ -79,7 +79,9 @@ class SnapshotManageTest(test.TestCase):
         code to the caller.
         """
         ctxt = context.RequestContext('admin', 'fake', True)
-        mock_db.return_value = fake_service.fake_service_obj(ctxt)
+        mock_db.return_value = fake_service.fake_service_obj(
+            ctxt,
+            binary='cinder-volume')
 
         body = {'snapshot': {'volume_id': 'fake_volume_id', 'ref': 'fake_ref'}}
         res = self._get_resp(body)

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index a0b6107f20f0205937145cc1586a391bd953adc7..c2c50ca1e3a19dbf1a1dd28ca33447c24ef67ace 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -1524,13 +1524,18 @@ class API(base.Base):
             elevated = context.elevated()
             try:
                 svc_host = volume_utils.extract_host(host, 'backend')
-                service = objects.Service.get_by_host_and_topic(
-                    elevated, svc_host, CONF.volume_topic)
+                service = objects.Service.get_by_args(
+                    elevated, svc_host, 'cinder-volume')
             except exception.ServiceNotFound:
                 with excutils.save_and_reraise_exception():
                     LOG.error(_LE('Unable to find service: %(service)s for '
                                   'given host: %(host)s.'),
                               {'service': CONF.volume_topic, 'host': host})
+            if service.disabled:
+                LOG.error(_LE('Unable to manage_existing volume on a disabled '
+                              'service.'))
+                raise exception.ServiceUnavailable()
+
             availability_zone = service.get('availability_zone')
 
         manage_what = {
@@ -1569,13 +1574,19 @@ class API(base.Base):
                                  metadata=None):
         host = volume_utils.extract_host(volume['host'])
         try:
-            objects.Service.get_by_host_and_topic(context.elevated(), host,
-                                                  CONF.volume_topic)
+            # NOTE(jdg): We don't use this, we just make sure it's valid
+            # and exists before sending off the call
+            service = objects.Service.get_by_args(
+                context.elevated(), host, 'cinder-volume')
         except exception.ServiceNotFound:
             with excutils.save_and_reraise_exception():
                 LOG.error(_LE('Unable to find service: %(service)s for '
                               'given host: %(host)s.'),
                           {'service': CONF.volume_topic, 'host': host})
+        if service.disabled:
+            LOG.error(_LE('Unable to manage_existing snapshot on a disabled '
+                          'service.'))
+            raise exception.ServiceUnavailable()
 
         snapshot_object = self.create_snapshot_in_db(context, volume, name,
                                                      description, False,
@@ -1595,7 +1606,7 @@ class API(base.Base):
         svc_host = volume_utils.extract_host(host, 'backend')
 
         service = objects.Service.get_by_args(
-            ctxt, svc_host, CONF.volume_topic)
+            ctxt, svc_host, 'cinder-volume')
         expected = {'replication_status': [fields.ReplicationStatus.ENABLED,
                     fields.ReplicationStatus.FAILED_OVER]}
         result = service.conditional_update(
@@ -1618,9 +1629,8 @@ class API(base.Base):
         ctxt = context.get_admin_context()
         svc_host = volume_utils.extract_host(host, 'backend')
 
-        # NOTE(jdg): get_by_host_and_topic filters out disabled
         service = objects.Service.get_by_args(
-            ctxt, svc_host, CONF.volume_topic)
+            ctxt, svc_host, 'cinder-volume')
         expected = {'frozen': False}
         result = service.conditional_update(
             {'frozen': True}, expected)
@@ -1639,9 +1649,8 @@ class API(base.Base):
         ctxt = context.get_admin_context()
         svc_host = volume_utils.extract_host(host, 'backend')
 
-        # NOTE(jdg): get_by_host_and_topic filters out disabled
         service = objects.Service.get_by_args(
-            ctxt, svc_host, CONF.volume_topic)
+            ctxt, svc_host, 'cinder-volume')
         expected = {'frozen': True}
         result = service.conditional_update(
             {'frozen': False}, expected)

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 1c318ff1243b5551bc50caff0d74803eefa4e169..de9a11c4c74b81ce5333e492f438406fa6f55148 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -245,9 +245,10 @@ class VolumeManager(manager.SchedulerDependentManager):
         curr_active_backend_id = None
         svc_host = vol_utils.extract_host(self.host, 'backend')
         try:
-            service = objects.Service.get_by_host_and_topic(
-                context.get_admin_context(), svc_host,
-                CONF.volume_topic)
+            service = objects.Service.get_by_args(
+                context.get_admin_context(),
+                svc_host,
+                'cinder-volume')
         except exception.ServiceNotFound:
             # NOTE(jdg): This is to solve problems with unit tests
             LOG.info(_LI("Service not found for updating "
@@ -503,9 +504,10 @@ class VolumeManager(manager.SchedulerDependentManager):
         try:
             # NOTE(jdg): may be some things to think about here in failover
             # scenarios
-            service = objects.Service.get_by_host_and_topic(
-                context.get_admin_context(), svc_host,
-                CONF.volume_topic)
+            service = objects.Service.get_by_args(
+                context.get_admin_context(),
+                svc_host,
+                'cinder-volume')
         except exception.ServiceNotFound:
             # FIXME(jdg): no idea what we'd do if we hit this case
             LOG.info(_LI("Service not found for updating "
@@ -3258,10 +3260,10 @@ class VolumeManager(manager.SchedulerDependentManager):
         """
         svc_host = vol_utils.extract_host(self.host, 'backend')
 
-        # NOTE(jdg): get_by_host_and_topic filters out disabled
         service = objects.Service.get_by_args(
-            context, svc_host,
-            CONF.volume_topic)
+            context,
+            svc_host,
+            'cinder-volume')
         volumes = objects.VolumeList.get_all_by_host(context, self.host)
 
         exception_encountered = False
@@ -3357,10 +3359,10 @@ class VolumeManager(manager.SchedulerDependentManager):
                             'notification to driver has failed.'))
         svc_host = vol_utils.extract_host(self.host, 'backend')
 
-        # NOTE(jdg): get_by_host_and_topic filters out disabled
         service = objects.Service.get_by_args(
-            context, svc_host,
-            CONF.volume_topic)
+            context,
+            svc_host,
+            'cinder-volume')
         service.disabled = True
         service.disabled_reason = "frozen"
         service.save()
@@ -3390,10 +3392,10 @@ class VolumeManager(manager.SchedulerDependentManager):
             return False
         svc_host = vol_utils.extract_host(self.host, 'backend')
 
-        # NOTE(jdg): get_by_host_and_topic filters out disabled
         service = objects.Service.get_by_args(
-            context, svc_host,
-            CONF.volume_topic)
+            context,
+            svc_host,
+            'cinder-volume')
         service.disabled = False
         service.disabled_reason = ""
         service.save()