openstack/manila
Code Issues Proposed changes

Fix share server info in CGs created from CGs

Browse Source 
Currently when a POST request is made to /consistency-groups
with a source cg-snapshot, the API does not register the share
network information (share_server_id and share_network_id) in
the database row newly created for the CG being created.

This information is essential to any shares that are being created
along with the consistency group.

- Disallow providing a share_network_id when using a source cg_snapshot_id
- Copy share network information from the parent CG
- Fix the share_server_id that was incorrect in the API response

 APIImpact

 Closes-Bug: #1571594
 Closes-Bug: #1572742

Change-Id: I1c3581c81e0b845f46eef3cd0acddb55850447a5
This commit is contained in:
Goutham Pacha Ravi 2016-04-21 13:17:49 -04:00
parent 669bcd1557
commit 118d440a90
14 changed files with 247 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
manila/api/v2/consistency_groups.py
View File

@ -192,6 +192,12 @@ class CGController(wsgi.Controller, wsgi.AdminActionsMixin):
raise exc.HTTPBadRequest(explanation=msg)
kwargs['share_type_ids'] = _share_types
if 'share_network_id' in cg and 'source_cgsnapshot_id' in cg:
msg = _("Cannot supply both 'share_network_id' and "
"'source_cgsnapshot_id' attributes as the share network "
"is inherited from the source.")
raise exc.HTTPBadRequest(explanation=msg)
if 'source_cgsnapshot_id' in cg:
source_cgsnapshot_id = cg.get('source_cgsnapshot_id')
if not uuidutils.is_uuid_like(source_cgsnapshot_id):
@ -199,7 +205,7 @@ class CGController(wsgi.Controller, wsgi.AdminActionsMixin):
raise exc.HTTPBadRequest(explanation=six.text_type(msg))
kwargs['source_cgsnapshot_id'] = source_cgsnapshot_id
if 'share_network_id' in cg:
elif 'share_network_id' in cg:
share_network_id = cg.get('share_network_id')
if not uuidutils.is_uuid_like(share_network_id):
msg = _("The 'share_network_id' attribute must be a uuid.")

2
manila/api/views/consistency_groups.py
View File

@ -59,7 +59,7 @@ class CGViewBuilder(common.ViewBuilder):
'links': self._get_links(request, cg['id']),
}
if context.is_admin:
cg_dict['share_server_id'] = cg_dict.get('share_server_id')
cg_dict['share_server_id'] = cg.get('share_server_id')
return {'consistency_group': cg_dict}
def _list_view(self, func, request, shares):

8
manila/consistency_group/api.py
View File

@ -54,6 +54,9 @@ class API(base.Base):
cgsnapshot = None
original_cg = None
# NOTE(gouthamr): share_server_id is inherited from the parent CG if a
# CG snapshot is specified, else, it will be set in the share manager.
share_server_id = None
if source_cgsnapshot_id:
cgsnapshot = self.db.cgsnapshot_get(context, source_cgsnapshot_id)
if cgsnapshot['status'] != constants.STATUS_AVAILABLE:
@ -65,6 +68,8 @@ class API(base.Base):
'consistency_group_id'])
share_type_ids = [s['share_type_id'] for s in original_cg[
'share_types']]
share_network_id = original_cg['share_network_id']
share_server_id = original_cg['share_server_id']
# Get share_type_objects
share_type_objects = []
@ -116,6 +121,7 @@ class API(base.Base):
options = {
'source_cgsnapshot_id': source_cgsnapshot_id,
'share_network_id': share_network_id,
'share_server_id': share_server_id,
'name': name,
'description': description,
'user_id': context.user_id,
@ -135,7 +141,7 @@ class API(base.Base):
for member in members:
share_type = share_types.get_share_type(
context, member['share_type_id'])
member['share'] = self.db.share_instance_get(
member['share_instance'] = self.db.share_instance_get(
context, member['share_instance_id'],
with_share_data=True)
self.share_api.create(context, member['share_proto'],

10
manila/share/api.py
View File

@ -277,10 +277,16 @@ class API(base.Base):
share_network_id=share_network_id))
if cgsnapshot_member:
host = cgsnapshot_member['share']['host']
# Inherit properties from the cgsnapshot_member
member_share_instance = cgsnapshot_member['share_instance']
updates = {
'host': member_share_instance['host'],
'share_network_id': member_share_instance['share_network_id'],
'share_server_id': member_share_instance['share_server_id'],
}
share = self.db.share_instance_update(context,
share_instance['id'],
{'host': host})
updates)
# NOTE(ameade): Do not cast to driver if creating from cgsnapshot
return

75
manila/tests/api/v2/test_consistency_groups.py
View File

@ -75,7 +75,11 @@ class CGApiTest(test.TestCase):
return cg, req
def _get_fake_cg(self, **values):
def _get_fake_cg(self, ctxt=None, **values):
if ctxt is None:
ctxt = self.context
cg = {
'id': 'fake_id',
'user_id': 'fakeuser',
@ -85,8 +89,8 @@ class CGApiTest(test.TestCase):
'description': None,
'host': None,
'source_cgsnapshot_id': None,
'share_network_id': None,
'share_server_id': None,
'share_network_id': uuid.uuid4(),
'share_server_id': uuid.uuid4(),
'share_types': [],
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
}
@ -95,7 +99,8 @@ class CGApiTest(test.TestCase):
expected_cg = copy.deepcopy(cg)
del expected_cg['user_id']
del expected_cg['share_server_id']
if not ctxt.is_admin:
del expected_cg['share_server_id']
expected_cg['links'] = mock.ANY
expected_cg['share_types'] = [st['share_type_id']
for st in cg.get('share_types')]
@ -120,11 +125,10 @@ class CGApiTest(test.TestCase):
mock.Mock(return_value=fake_cg))
body = {"consistency_group": {}}
context = self.request.environ['manila.context']
res_dict = self.controller.create(self.request, body)
self.controller.cg_api.create.assert_called_once_with(
context, share_type_ids=[self.fake_share_type['id']])
self.context, share_type_ids=[self.fake_share_type['id']])
self.assertEqual(expected_cg, res_dict['consistency_group'])
self.mock_policy_check.assert_called_once_with(
self.context, self.resource_name, 'create')
@ -209,6 +213,27 @@ class CGApiTest(test.TestCase):
self.mock_policy_check.assert_called_once_with(
self.context, self.resource_name, 'create')
def test_cg_create_with_source_cgsnapshot_id_and_share_network(self):
fake_snap_id = six.text_type(uuid.uuid4())
fake_net_id = six.text_type(uuid.uuid4())
self.mock_object(share_types, 'get_default_share_type',
mock.Mock(return_value=self.fake_share_type))
mock_api_call = self.mock_object(self.controller.cg_api, 'create')
body = {
"consistency_group": {
"source_cgsnapshot_id": fake_snap_id,
"share_network_id": fake_net_id,
}
}
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller.create,
self.request, body)
self.assertFalse(mock_api_call.called)
self.mock_policy_check.assert_called_once_with(
self.context, self.resource_name, 'create')
def test_cg_create_with_source_cgsnapshot_id(self):
fake_snap_id = six.text_type(uuid.uuid4())
fake_cg, expected_cg = self._get_fake_cg(
@ -513,14 +538,15 @@ class CGApiTest(test.TestCase):
self.context, self.resource_name, 'get_all')
def test_cg_list_detail_with_limit(self):
fake_cg, expected_cg = self._get_fake_cg()
fake_cg2, expected_cg2 = self._get_fake_cg(id="fake_id2")
self.mock_object(cg_api.API, 'get_all',
mock.Mock(return_value=[fake_cg, fake_cg2]))
req = fakes.HTTPRequest.blank('/consistency_groups?limit=1',
version=self.api_version,
experimental=True)
req_context = req.environ['manila.context']
fake_cg, expected_cg = self._get_fake_cg(ctxt=req_context)
fake_cg2, expected_cg2 = self._get_fake_cg(ctxt=req_context,
id="fake_id2")
self.mock_object(cg_api.API, 'get_all',
mock.Mock(return_value=[fake_cg, fake_cg2]))
res_dict = self.controller.detail(req)
@ -530,14 +556,15 @@ class CGApiTest(test.TestCase):
req_context, self.resource_name, 'get_all')
def test_cg_list_detail_with_limit_and_offset(self):
fake_cg, expected_cg = self._get_fake_cg()
fake_cg2, expected_cg2 = self._get_fake_cg(id="fake_id2")
self.mock_object(cg_api.API, 'get_all',
mock.Mock(return_value=[fake_cg, fake_cg2]))
req = fakes.HTTPRequest.blank('/consistency_groups?limit=1&offset=1',
version=self.api_version,
experimental=True)
req_context = req.environ['manila.context']
fake_cg, expected_cg = self._get_fake_cg(ctxt=req_context)
fake_cg2, expected_cg2 = self._get_fake_cg(
id="fake_id2", ctxt=req_context)
self.mock_object(cg_api.API, 'get_all',
mock.Mock(return_value=[fake_cg, fake_cg2]))
res_dict = self.controller.detail(req)
@ -596,30 +623,32 @@ class CGApiTest(test.TestCase):
req_context, self.resource_name, 'get')
def test_cg_show_as_admin(self):
fake_cg, expected_cg = self._get_fake_cg()
expected_cg['share_server_id'] = None
self.mock_object(cg_api.API, 'get',
mock.Mock(return_value=fake_cg))
req = fakes.HTTPRequest.blank(
'/consistency_groups/%s' % fake_cg['id'],
'/consistency_groups/my_cg_id',
version=self.api_version, experimental=True)
admin_context = req.environ['manila.context'].elevated()
req.environ['manila.context'] = admin_context
fake_cg, expected_cg = self._get_fake_cg(
ctxt=admin_context, id='my_cg_id')
self.mock_object(cg_api.API, 'get',
mock.Mock(return_value=fake_cg))
res_dict = self.controller.show(req, fake_cg['id'])
self.assertEqual(expected_cg, res_dict['consistency_group'])
self.assertIsNotNone(res_dict['consistency_group']['share_server_id'])
self.mock_policy_check.assert_called_once_with(
admin_context, self.resource_name, 'get')
def test_cg_show_cg_not_found(self):
fake_cg, expected_cg = self._get_fake_cg()
self.mock_object(cg_api.API, 'get',
mock.Mock(side_effect=exception.NotFound))
req = fakes.HTTPRequest.blank(
'/consistency_groups/%s' % fake_cg['id'],
'/consistency_groups/myfakecg',
version=self.api_version, experimental=True)
req_context = req.environ['manila.context']
fake_cg, expected_cg = self._get_fake_cg(
ctxt=req_context, id='myfakecg')
self.mock_object(cg_api.API, 'get',
mock.Mock(side_effect=exception.NotFound))
self.assertRaises(webob.exc.HTTPNotFound, self.controller.show,
req, fake_cg['id'])

46
manila/tests/consistency_group/test_api.py
View File

@ -45,9 +45,15 @@ def fake_cg(id, **kwargs):
'host': None,
'source_cgsnapshot_id': None,
'share_network_id': None,
'share_server_id': None,
'share_types': None,
'created_at': datetime.datetime(1, 1, 1, 1, 1, 1),
}
if 'source_cgsnapshot_id' in kwargs:
cg['share_network_id'] = 'fake_share_network_id'
cg['share_server_id'] = 'fake_share_server_id'
cg.update(kwargs)
return cg
@ -89,6 +95,7 @@ class CGAPITestCase(test.TestCase):
user_id=self.context.user_id,
project_id=self.context.project_id,
status=constants.STATUS_CREATING)
expected_values = cg.copy()
for name in ('id', 'host', 'created_at'):
expected_values.pop(name, None)
@ -283,19 +290,26 @@ class CGAPITestCase(test.TestCase):
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_AVAILABLE,
host='fake_original_host')
host='fake_original_host',
share_network_id='fake_network_id',
share_server_id='fake_server_id')
cg = fake_cg('fakeid',
user_id=self.context.user_id,
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_CREATING,
host='fake_original_host')
host='fake_original_host',
share_network_id='fake_network_id',
share_server_id='fake_server_id')
expected_values = cg.copy()
for name in ('id', 'created_at'):
for name in ('id', 'created_at', 'share_network_id',
'share_server_id'):
expected_values.pop(name, None)
expected_values['source_cgsnapshot_id'] = snap['id']
expected_values['share_types'] = ["fake_share_type_id"]
expected_values['share_network_id'] = 'fake_network_id'
expected_values['share_server_id'] = 'fake_server_id'
self.mock_object(db_driver, 'cgsnapshot_get',
mock.Mock(return_value=snap))
@ -327,18 +341,25 @@ class CGAPITestCase(test.TestCase):
user_id=self.context.user_id,
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_AVAILABLE)
status=constants.STATUS_AVAILABLE,
share_network_id='fake_network_id',
share_server_id='fake_server_id')
cg = fake_cg('fakeid',
user_id=self.context.user_id,
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_CREATING)
status=constants.STATUS_CREATING,
share_network_id='fake_network_id',
share_server_id='fake_server_id')
expected_values = cg.copy()
for name in ('id', 'created_at'):
for name in ('id', 'created_at', 'fake_network_id',
'fake_share_server_id'):
expected_values.pop(name, None)
expected_values['source_cgsnapshot_id'] = snap['id']
expected_values['share_types'] = ["fake_share_type_id"]
expected_values['share_network_id'] = 'fake_network_id'
expected_values['share_server_id'] = 'fake_server_id'
self.mock_object(db_driver, 'cgsnapshot_get',
mock.Mock(return_value=snap))
@ -375,18 +396,25 @@ class CGAPITestCase(test.TestCase):
user_id=self.context.user_id,
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_AVAILABLE)
status=constants.STATUS_AVAILABLE,
share_network_id='fake_network_id',
share_server_id='fake_server_id')
cg = fake_cg('fakeid',
user_id=self.context.user_id,
project_id=self.context.project_id,
share_types=[fake_share_type_mapping],
status=constants.STATUS_CREATING)
status=constants.STATUS_CREATING,
share_network_id='fake_network_id',
share_server_id='fake_server_id')
expected_values = cg.copy()
for name in ('id', 'created_at'):
for name in ('id', 'created_at', 'share_network_id',
'share_server_id'):
expected_values.pop(name, None)
expected_values['source_cgsnapshot_id'] = snap['id']
expected_values['share_types'] = ["fake_share_type_id"]
expected_values['share_network_id'] = 'fake_network_id'
expected_values['share_server_id'] = 'fake_server_id'
self.mock_object(db_driver, 'cgsnapshot_get',
mock.Mock(return_value=snap))

5
manila/tests/fake_share.py
View File

@ -54,11 +54,16 @@ def fake_share_instance(base_share=None, **kwargs):
'share_id': share['id'],
'id': "fakeinstanceid",
'status': "active",
'host': 'fakehost',
'share_network_id': 'fakesharenetworkid',
'share_server_id': 'fakeshareserverid',
}
for attr in models.ShareInstance._proxified_properties:
share_instance[attr] = getattr(share, attr, None)
share_instance.update(kwargs)
return db_fakes.FakeModel(share_instance)

37
manila/tests/share/test_api.py
View File

@ -29,6 +29,7 @@ from manila.data import rpcapi as data_rpc
from manila import db as db_api
from manila.db.sqlalchemy import models
from manila import exception
from manila import policy
from manila import quota
from manila import share
from manila.share import api as share_api
@ -728,6 +729,42 @@ class ShareAPITestCase(test.TestCase):
self.context, request_spec=mock.ANY, filter_properties={})
self.assertFalse(self.api.share_rpcapi.create_share_instance.called)
def test_create_instance_cgsnapshot_member(self):
fake_req_spec = {
'share_properties': 'fake_share_properties',
'share_instance_properties': 'fake_share_instance_properties',
}
share = fakes.fake_share()
member_info = {
'host': 'host',
'share_network_id': 'share_network_id',
'share_server_id': 'share_server_id',
}
fake_instance = fakes.fake_share_instance(
share_id=share['id'], **member_info)
cgsnapmember = {'share_instance': fake_instance}
self.mock_policy_check = self.mock_object(
policy, 'check_policy', mock.Mock(return_value=True))
mock_share_rpcapi_call = self.mock_object(self.share_rpcapi,
'create_share_instance')
mock_scheduler_rpcapi_call = self.mock_object(self.scheduler_rpcapi,
'create_share_instance')
mock_db_share_instance_update = self.mock_object(
db_api, 'share_instance_update')
self.mock_object(
share_api.API, '_create_share_instance_and_get_request_spec',
mock.Mock(return_value=(fake_req_spec, fake_instance)))
retval = self.api.create_instance(self.context, fake_share,
cgsnapshot_member=cgsnapmember)
self.assertIsNone(retval)
mock_db_share_instance_update.assert_called_once_with(
self.context, fake_instance['id'], member_info)
self.assertFalse(mock_scheduler_rpcapi_call.called)
self.assertFalse(mock_share_rpcapi_call.called)
@ddt.data('dr', 'readable', None)
def test_manage_new(self, replication_type):
share_data = {

38
manila_tempest_tests/tests/api/admin/test_consistency_group_actions.py
View File

@ -43,6 +43,8 @@ class ConsistencyGroupActionsTest(base.BaseSharesAdminTest):
# Create a consistency group
cls.consistency_group = cls.create_consistency_group(
share_type_ids=[cls.share_type['id'], cls.share_type2['id']])
cls.consistency_group = cls.shares_v2_client.get_consistency_group(
cls.consistency_group['id'])
@test.attr(type=["gate", ])
def test_create_cg_from_cgsnapshot_with_multiple_share_types_v2_4(self):
@ -103,9 +105,13 @@ class ConsistencyGroupActionsTest(base.BaseSharesAdminTest):
version='2.4',
)
self.create_consistency_group(cleanup_in_class=False,
source_cgsnapshot_id=cgsnapshot['id'],
version='2.4')
new_cg = self.create_consistency_group(
cleanup_in_class=False, source_cgsnapshot_id=cgsnapshot['id'],
version='2.4')
new_cg_shares = self.shares_v2_client.list_shares(
detailed=True,
params={'consistency_group_id': new_cg['id']},
version='2.4')
# TODO(akerr): Skip until bug 1483886 is resolved
# Verify that the new shares correspond to correct share types
@ -117,3 +123,29 @@ class ConsistencyGroupActionsTest(base.BaseSharesAdminTest):
# 'Expected shares of types %s, got %s.' % (
# sorted(expected_share_types),
# sorted(actual_share_types)))
# Ensure that share_server information of the child CG and associated
# shares match with that of the parent CG
self.assertEqual(self.consistency_group['share_network_id'],
new_cg['share_network_id'])
self.assertEqual(self.consistency_group['share_server_id'],
new_cg['share_server_id'])
for share in new_cg_shares:
msg = ('Share %(share)s has %(attr)s=%(value)s and it does not '
'match that of the parent CG where %(attr)s=%(orig)s.')
payload = {
'share': share['id'],
'attr': 'share_network_id',
'value': share['share_network_id'],
'orig': self.consistency_group['share_network_id'],
}
self.assertEqual(self.consistency_group['share_network_id'],
share['share_network_id'], msg % payload)
payload.update({'attr': 'share_server_id',
'value': share['share_server_id'],
'orig': self.consistency_group['share_server_id'],
})
self.assertEqual(self.consistency_group['share_server_id'],
share['share_server_id'], msg % payload)

30
manila_tempest_tests/tests/api/admin/test_consistency_groups.py
View File

@ -66,3 +66,33 @@ class ConsistencyGroupsTest(base.BaseSharesAdminTest):
'%s. Expected %s, got %s' % (consistency_group['id'],
expected_share_types,
actual_share_types))
@testtools.skipIf(
not CONF.share.multitenancy_enabled, "Only for multitenancy.")
def test_create_cg_from_cgsnapshot_verify_share_server_information(self):
# Create a consistency group
orig_consistency_group = self.create_consistency_group(
cleanup_in_class=False,
share_type_ids=[self.share_type['id']],
version='2.4')
# Get latest CG information
orig_consistency_group = self.shares_v2_client.get_consistency_group(
orig_consistency_group['id'], version='2.4')
# Assert share server information
self.assertIsNotNone(orig_consistency_group['share_network_id'])
self.assertIsNotNone(orig_consistency_group['share_server_id'])
cg_snapshot = self.create_cgsnapshot_wait_for_active(
orig_consistency_group['id'], cleanup_in_class=False,
version='2.4')
new_consistency_group = self.create_consistency_group(
cleanup_in_class=False, version='2.4',
source_cgsnapshot_id=cg_snapshot['id'])
# Assert share server information
self.assertEqual(orig_consistency_group['share_network_id'],
new_consistency_group['share_network_id'])
self.assertEqual(orig_consistency_group['share_server_id'],
new_consistency_group['share_server_id'])

5
manila_tempest_tests/tests/api/base.py
View File

@ -431,8 +431,9 @@ class BaseSharesTest(test.BaseTestCase):
def create_consistency_group(cls, client=None, cleanup_in_class=True,
share_network_id=None, **kwargs):
client = client or cls.shares_v2_client
kwargs['share_network_id'] = (share_network_id or
client.share_network_id or None)
if kwargs.get('source_cgsnapshot_id') is None:
kwargs['share_network_id'] = (share_network_id or
client.share_network_id or None)
consistency_group = client.create_consistency_group(**kwargs)
resource = {
"type": "consistency_group",

9
manila_tempest_tests/tests/api/test_consistency_group_actions.py
View File

@ -263,6 +263,13 @@ class ConsistencyGroupActionsTest(base.BaseSharesTest):
version='2.4'
)
new_consistency_group = self.shares_v2_client.get_consistency_group(
new_consistency_group['id'], version='2.4')
# Verify that share_network information matches source CG
self.assertEqual(self.cg['share_network_id'],
new_consistency_group['share_network_id'])
new_shares = self.shares_v2_client.list_shares(
params={'consistency_group_id': new_consistency_group['id']},
detailed=True,
@ -292,6 +299,8 @@ class ConsistencyGroupActionsTest(base.BaseSharesTest):
# TODO(akerr): Add back assert when bug 1483886 is fixed
# self.assertEqual(member['share_type_id'],
# share['share_type'])
self.assertEqual(self.cg['share_network_id'],
share['share_network_id'])
@testtools.skipUnless(CONF.share.run_consistency_group_tests,

10
manila_tempest_tests/tests/api/test_consistency_groups.py
View File

@ -125,7 +125,7 @@ class ConsistencyGroupsTest(base.BaseSharesTest):
self.assertEmpty(new_shares,
'Expected 0 new shares, got %s' % len(new_shares))
msg = 'Expected cgsnapshot_id %s as source of share %s' % (
msg = 'Expected cgsnapshot_id %s as source of consistency group %s' % (
cgsnapshot['id'], new_consistency_group['source_cgsnapshot_id'])
self.assertEqual(new_consistency_group['source_cgsnapshot_id'],
cgsnapshot['id'], msg)
@ -135,3 +135,11 @@ class ConsistencyGroupsTest(base.BaseSharesTest):
new_consistency_group['share_types']))
self.assertEqual(sorted(consistency_group['share_types']),
sorted(new_consistency_group['share_types']), msg)
# Assert the share_network information is the same
msg = 'Expected share_network %s as share_network of cg %s' % (
consistency_group['share_network_id'],
new_consistency_group['share_network_id'])
self.assertEqual(consistency_group['share_network_id'],
new_consistency_group['share_network_id'],
msg)

7
releasenotes/notes/fix-consistency-groups-api-dd9b5b99138e22eb.yaml Normal file
View File

@ -0,0 +1,7 @@
---
fixes:
- Consistency Group APIs return share_server_id information correctly to
administrators.
- When using a consistency group snapshot to create another consistency
group, share server and network information is persisted from the source
consistency group to the new consistency group.