openstack
/
cinder
Code Issues Proposed changes

Set replication_status on volume creation 

The scheduler is currently always setting `replication_status` to
"disabled" regardles of the real replication status.

Some drivers are updating it on volume creation, but even in that case
the user will have received the API REST response with the
`replication_status` set to "disabled" and will have to do a `show` of
the volume once the creation has been completed to confirm if the volume
is actually replicated or not.

This patch changes this and allows the scheduler to correctly set the
`replication_status` on volume creation based on the standard property
`replication_enable`.

This doesn't solve 100% of the cases, as a Cloud Administrator may not
be using the `replication_enable` extra specs in volume types to set
replication because it's using a backed that is replicating at
backend/pool level.

In that case there will be a discrepancy, just like we have now, until
the driver completes the creation and returns the model update for the
volume.

Closes-Bug: #1643883
Change-Id: Id4df2b7ad55e9b5def91329f5437da9caa185c30
This commit is contained in:
Gorka Eguileor 2016-11-17 18:24:30 +01:00
parent 36735e244a
commit 7647f994f1
5 changed files with 95 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
cinder/tests/unit/test_quota.py
View File

@ -48,9 +48,14 @@ class QuotaIntegrationTestCase(test.TestCase):
objects.register_all()
super(QuotaIntegrationTestCase, self).setUp()
self.volume_type_name = CONF.default_volume_type
self.volume_type = db.volume_type_create(
context.get_admin_context(),
dict(name=self.volume_type_name))
self.volume_type = objects.VolumeType(context.get_admin_context(),
name=self.volume_type_name,
description='',
is_public=False,
projects=[],
extra_specs={})
self.volume_type.create()
self.addCleanup(db.volume_type_destroy, context.get_admin_context(),
self.volume_type['id'])

17
cinder/tests/unit/test_volume_utils.py
View File

@ -21,6 +21,7 @@ import io
import mock
import six
import ddt
from oslo_concurrency import processutils
from oslo_config import cfg
from oslo_utils import units
@ -688,6 +689,7 @@ class CopyVolumeTestCase(test.TestCase):
1073741824, mock.ANY)
@ddt.ddt
class VolumeUtilsTestCase(test.TestCase):
def test_null_safe_str(self):
self.assertEqual('', volume_utils.null_safe_str(None))
@ -1000,3 +1002,18 @@ class VolumeUtilsTestCase(test.TestCase):
create_key.assert_called_once_with(ctxt,
algorithm='aes',
length=256)
@ddt.data('<is> True', '<is> true', '<is> yes')
def test_is_replicated_spec_true(self, enabled):
res = volume_utils.is_replicated_spec({'replication_enabled': enabled})
self.assertTrue(res)
@ddt.data({}, None, {'key': 'value'})
def test_is_replicated_no_specs(self, extra_specs):
res = volume_utils.is_replicated_spec(extra_specs)
self.assertFalse(res)
@ddt.data('<is> False', '<is> false', '<is> f', 'baddata', 'bad data')
def test_is_replicated_spec_false(self, enabled):
res = volume_utils.is_replicated_spec({'replication_enabled': enabled})
self.assertFalse(res)

57
cinder/tests/unit/volume/flows/test_create_volume_flow.py
View File

@ -48,6 +48,9 @@ class CreateVolumeFlowTestCase(test.TestCase):
# Ensure that time.time() always returns more than the last time it was
# called to avoid div by zero errors.
self.counter = float(0)
self.get_extra_specs = self.patch(
'cinder.volume.volume_types.get_volume_type_extra_specs',
return_value={})
@mock.patch('cinder.objects.Volume.get_by_id')
@mock.patch('cinder.volume.utils.extract_host')
@ -101,6 +104,42 @@ class CreateVolumeFlowTestCase(test.TestCase):
consistencygroup_get_by_id.assert_called_once_with(self.ctxt, 5)
mock_extract_host.assert_called_once_with('host@backend#pool')
@ddt.data(('enabled', {'replication_enabled': '<is> True'}),
('disabled', {'replication_enabled': '<is> False'}),
('disabled', {}))
@ddt.unpack
@mock.patch('cinder.volume.flows.api.create_volume.'
'ExtractVolumeRequestTask.'
'_get_encryption_key_id', mock.Mock())
@mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs')
def test_extract_volume_request_replication_status(self,
replication_status,
extra_specs,
fake_get_qos):
self.get_extra_specs.return_value = extra_specs
fake_image_service = fake_image.FakeImageService()
fake_key_manager = mock_key_manager.MockKeyManager()
task = create_volume.ExtractVolumeRequestTask(fake_image_service,
{'nova'})
result = task.execute(self.ctxt,
size=1,
snapshot=None,
image_id=None,
source_volume=None,
availability_zone='nova',
volume_type={'id': fakes.VOLUME_TYPE_ID,
'size': 1},
metadata=None,
key_manager=fake_key_manager,
source_replica=None,
consistencygroup=None,
cgsnapshot=None,
group=None)
self.assertEqual(replication_status, result['replication_status'],
extra_specs)
@mock.patch('cinder.volume.volume_types.is_encrypted')
@mock.patch('cinder.volume.flows.api.create_volume.'
'ExtractVolumeRequestTask.'
@ -199,7 +238,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.volume.volume_types.is_encrypted')
@ -299,7 +339,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.volume.volume_types.is_encrypted')
@ -355,7 +396,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.volume.volume_types.is_encrypted')
@ -418,7 +460,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.db.volume_type_get_by_name')
@ -482,7 +525,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.db.volume_type_get_by_name')
@ -545,7 +589,8 @@ class CreateVolumeFlowTestCase(test.TestCase):
'source_replicaid': None,
'consistencygroup_id': None,
'cgsnapshot_id': None,
'group_id': None, }
'group_id': None,
'replication_status': 'disabled'}
self.assertEqual(expected_result, result)
@mock.patch('cinder.db.volume_type_get_by_name')

12
cinder/volume/flows/api/create_volume.py
View File

@ -458,9 +458,19 @@ class ExtractVolumeRequestTask(flow_utils.CinderTask):
qos_specs = volume_types.get_volume_type_qos_specs(volume_type_id)
if qos_specs['qos_specs']:
specs = qos_specs['qos_specs'].get('specs', {})
# Determine default replication status
extra_specs = volume_types.get_volume_type_extra_specs(
volume_type_id)
if not specs:
# to make sure we don't pass empty dict
specs = None
extra_specs = None
if vol_utils.is_replicated_spec(extra_specs):
replication_status = fields.ReplicationStatus.ENABLED
else:
replication_status = fields.ReplicationStatus.DISABLED
return {
'size': size,
@ -475,6 +485,7 @@ class ExtractVolumeRequestTask(flow_utils.CinderTask):
'consistencygroup_id': consistencygroup_id,
'cgsnapshot_id': cgsnapshot_id,
'group_id': group_id,
'replication_status': replication_status,
}
@ -516,7 +527,6 @@ class EntryCreateTask(flow_utils.CinderTask):
# Rename these to the internal name.
'display_description': kwargs.pop('description'),
'display_name': kwargs.pop('name'),
'replication_status': 'disabled',
'multiattach': kwargs.pop('multiattach'),
}

8
cinder/volume/utils.py
View File

@ -885,3 +885,11 @@ def create_encryption_key(context, key_manager, volume_type_id):
algorithm=algorithm,
length=length)
return encryption_key_id
def is_replicated_spec(extra_specs):
if not extra_specs:
return False
spec = extra_specs.get('replication_enabled', '').split()
return (len(spec) == 2 and
spec[0] == '<is>' and strutils.bool_from_string(spec[1]))