openstack
/
cinder
Code Issues Proposed changes

Fail manage operations if service is down 

Existing code will fail at API level if a service is disabled and we try
to manage/get_manageable volumes/snapshots on that service, but it won't
fail at API level if the service is down, and the message will wait in
the message queue until the service is up again, which is misleading.

This patch changes this behavior and makes manage_existing,
manage_existing_snapshot, get_manageable_volumes, and
get_manageable_snapshots, fail at the API if the service is down.

Change-Id: Ifd56a46fce9ac2fe72a30820c05a5cf9adee229f
Closes-Bug: #1605109
This commit is contained in:
Gorka Eguileor 2016-07-21 10:11:30 +02:00
parent 5fdfe1f699
commit 889947c622
4 changed files with 104 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
cinder/tests/unit/api/contrib/test_snapshot_manage.py
View File

@ -112,7 +112,7 @@ class SnapshotManageTest(test.TestCase):
@mock.patch('cinder.db.service_get')
def test_manage_snapshot_ok(self, mock_db,
mock_create_snapshot, mock_rpcapi):
"""Test successful manage volume execution.
"""Test successful manage snapshot execution.
Tests for correct operation when valid arguments are passed in the
request body. We ensure that cinder.volume.api.API.manage_existing got
@ -144,6 +144,41 @@ class SnapshotManageTest(test.TestCase):
args = mock_rpcapi.call_args[0]
self.assertEqual('fake_ref', args[2])
@mock.patch('cinder.utils.service_is_up', return_value=True)
@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')
def test_manage_snapshot_disabled(self, mock_db, mock_create_snapshot,
mock_rpcapi, mock_is_up):
"""Test manage snapshot failure due to disabled service."""
mock_db.return_value = fake_service.fake_service_obj(self._admin_ctxt,
disabled=True)
body = {'snapshot': {'volume_id': fake.VOLUME_ID, 'ref': 'fake_ref'}}
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
mock_create_snapshot.assert_not_called()
mock_rpcapi.assert_not_called()
mock_is_up.assert_not_called()
@mock.patch('cinder.utils.service_is_up', return_value=False)
@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')
def test_manage_snapshot_is_down(self, mock_db, mock_create_snapshot,
mock_rpcapi, mock_is_up):
"""Test manage snapshot failure due to down service."""
mock_db.return_value = fake_service.fake_service_obj(self._admin_ctxt)
body = {'snapshot': {'volume_id': fake.VOLUME_ID, 'ref': 'fake_ref'}}
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
mock_create_snapshot.assert_not_called()
mock_rpcapi.assert_not_called()
self.assertTrue(mock_is_up.called)
def test_manage_snapshot_missing_volume_id(self):
"""Test correct failure when volume_id is not specified."""
body = {'snapshot': {'ref': 'fake_ref'}}
@ -240,3 +275,24 @@ class SnapshotManageTest(test.TestCase):
mock_api_manageable.assert_called_once_with(
self._admin_ctxt, 'fakehost', limit=10, marker='1234', offset=4,
sort_dirs=['asc'], sort_keys=['reference'])
@mock.patch('cinder.utils.service_is_up', return_value=True)
@mock.patch('cinder.db.service_get')
def test_get_manageable_snapshots_disabled(self, mock_db, mock_is_up):
mock_db.return_value = fake_service.fake_service_obj(self._admin_ctxt,
disabled=True)
res = self._get_resp_get('host_ok', False, True)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
mock_is_up.assert_not_called()
@mock.patch('cinder.utils.service_is_up', return_value=False)
@mock.patch('cinder.db.service_get')
def test_get_manageable_snapshots_is_down(self, mock_db, mock_is_up):
mock_db.return_value = fake_service.fake_service_obj(self._admin_ctxt)
res = self._get_resp_get('host_ok', False, True)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
self.assertTrue(mock_is_up.called)

45
cinder/tests/unit/api/contrib/test_volume_manage.py
View File

@ -41,7 +41,7 @@ def app():
return mapper
def service_get_by_host_and_topic(context, host, topic):
def service_get(context, host, binary):
"""Replacement for Service.service_get_by_host_and_topic.
We mock the Service.service_get_by_host_and_topic method to return
@ -50,7 +50,9 @@ def service_get_by_host_and_topic(context, host, topic):
check for existence of a host, so the content returned doesn't matter.
"""
if host == 'host_ok':
return {}
return {'disabled': False}
if host == 'host_disabled':
return {'disabled': True}
raise exception.ServiceNotFound(service_id=host)
# Some of the tests check that volume types are correctly validated during a
@ -128,8 +130,7 @@ def api_get_manageable_volumes(*args, **kwargs):
@ddt.ddt
@mock.patch('cinder.objects.service.Service.get_by_host_and_topic',
service_get_by_host_and_topic)
@mock.patch('cinder.db.service_get', service_get)
@mock.patch('cinder.volume.volume_types.get_volume_type_by_name',
vt_get_volume_type_by_name)
@mock.patch('cinder.volume.volume_types.get_volume_type',
@ -210,6 +211,26 @@ class VolumeManageTest(test.TestCase):
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int)
@mock.patch('cinder.utils.service_is_up', return_value=True)
def test_manage_volume_disabled(self, mock_is_up):
"""Test manage volume failure due to disabled service."""
body = {'volume': {'host': 'host_disabled', 'ref': 'fake_ref'}}
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
mock_is_up.assert_not_called()
@mock.patch('cinder.utils.service_is_up', return_value=False)
def test_manage_volume_is_down(self, mock_is_up):
"""Test manage volume failure due to down service."""
body = {'volume': {'host': 'host_ok', 'ref': 'fake_ref'}}
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
self.assertTrue(mock_is_up.called)
@mock.patch('cinder.volume.api.API.manage_existing', api_manage)
@mock.patch(
'cinder.api.openstack.wsgi.Controller.validate_name_and_description')
@ -335,3 +356,19 @@ class VolumeManageTest(test.TestCase):
"metadata": value}}
res = self._get_resp_post(body)
self.assertEqual(400, res.status_int)
@mock.patch('cinder.utils.service_is_up', return_value=True)
def test_get_manageable_volumes_disabled(self, mock_is_up):
res = self._get_resp_get('host_disabled', False, True)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
mock_is_up.assert_not_called()
@mock.patch('cinder.utils.service_is_up', return_value=False)
def test_get_manageable_volumes_is_down(self, mock_is_up):
res = self._get_resp_get('host_ok', False, True)
self.assertEqual(400, res.status_int, res)
self.assertEqual(exception.ServiceUnavailable.message,
res.json['badRequest']['message'])
self.assertTrue(mock_is_up.called)

2
cinder/tests/unit/api/v3/test_volume_manage.py
View File

@ -37,7 +37,7 @@ def app():
@mock.patch('cinder.objects.service.Service.get_by_host_and_topic',
test_contrib.service_get_by_host_and_topic)
test_contrib.service_get)
@mock.patch('cinder.volume.volume_types.get_volume_type_by_name',
test_contrib.vt_get_volume_type_by_name)
@mock.patch('cinder.volume.volume_types.get_volume_type',

5
cinder/volume/api.py
View File

@ -1485,6 +1485,11 @@ class API(base.Base):
'service.'), resource)
raise exception.ServiceUnavailable()
if not utils.service_is_up(service):
LOG.error(_LE('Unable to manage existing %s on a service that is '
'down.'), resource)
raise exception.ServiceUnavailable()
return service
def manage_existing(self, context, host, ref, name=None, description=None,