From 541168b86ec5b42db34524b3946b701ed8500737 Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Thu, 13 Dec 2018 08:42:59 -0500 Subject: [PATCH] Fix A/A 'resource_backend' when scheduling volumes Fix an issue with the 'resource_backend' included in the scheduler spec for creating a volume associated with another volume, snapshot, or group/cg. When running A/A, the 'resource_backend' must reference the cluster, not the host. Enhance the unit tests that cover this area. This includes fixing the 'expected_spec' so it copies a dictionary rather than referencing it, so that external changes to the dictionary don't inadvertently update the unit test's expected results. Closes-Bug: #1808343 Change-Id: I7d414844d094945b55a094a8426687595f22de28 --- cinder/objects/base.py | 3 ++ .../unit/volume/flows/fake_volume_api.py | 4 +- .../volume/flows/test_create_volume_flow.py | 40 ++++++++++++++++--- cinder/volume/flows/api/create_volume.py | 9 +++-- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/cinder/objects/base.py b/cinder/objects/base.py index f6200fd2df5..f8c02ab32f0 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -528,6 +528,9 @@ class ClusteredObject(object): 'storage backends.') raise exception.InvalidInput(reason=msg) + # The object's resource backend depends on whether it's clustered. + resource_backend = service_topic_queue + class CinderObjectSerializer(base.VersionedObjectSerializer): OBJ_BASE_CLASS = CinderObject diff --git a/cinder/tests/unit/volume/flows/fake_volume_api.py b/cinder/tests/unit/volume/flows/fake_volume_api.py index 83aec482e73..94ab0962f2b 100644 --- a/cinder/tests/unit/volume/flows/fake_volume_api.py +++ b/cinder/tests/unit/volume/flows/fake_volume_api.py @@ -13,7 +13,7 @@ class FakeVolumeAPI(object): def __init__(self, expected_spec, test_inst): - self.expected_spec = expected_spec + self.expected_spec = expected_spec.copy() self.test_inst = test_inst def create_volume(self, ctxt, volume, host, @@ -31,7 +31,7 @@ class FakeVolumeAPI(object): class FakeSchedulerRpcAPI(object): def __init__(self, expected_spec, test_inst): - self.expected_spec = expected_spec + self.expected_spec = expected_spec.copy() self.test_inst = test_inst def create_volume(self, ctxt, volume, snapshot_id=None, image_id=None, diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py index 5c405a2de73..b043832d338 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -67,12 +67,18 @@ class CreateVolumeFlowTestCase(test.TestCase): mock_time, mock_extract_host, volume_get_by_id): mock_time.side_effect = self.time_inc - volume = fake_volume.fake_volume_obj(self.ctxt, - host='host@backend#pool') + volume = fake_volume.fake_volume_obj( + self.ctxt, + host='host@backend#pool', + cluster_name='cluster@backend#pool') volume_get_by_id.return_value = volume + # This is the spec for a volume created from another resource. It + # includes the 'resource_backend'. When the volume is associated + # with a cluster the 'resource_backend' should use the cluster name. spec = {'volume_id': volume.id, 'volume': volume, + 'resource_backend': 'cluster@backend#pool', 'source_volid': volume.id, 'snapshot_id': None, 'image_id': 4, @@ -87,7 +93,13 @@ class CreateVolumeFlowTestCase(test.TestCase): fake_volume_api.FakeVolumeAPI(spec, self), fake_volume_api.FakeDb()) + # Remove 'resource_backend' prior to calling task._cast_create_volume + # (the point of the test is to confirm that it adds it to the spec + # sent to the scheduler). + spec.pop('resource_backend') + task._cast_create_volume(self.ctxt, spec, {}) + mock_snapshot_get.assert_not_called() mock_extract_host.assert_not_called() snapshot = fake_snapshot.fake_snapshot_obj(self.ctxt, @@ -96,6 +108,7 @@ class CreateVolumeFlowTestCase(test.TestCase): spec = {'volume_id': volume.id, 'volume': volume, + 'resource_backend': 'cluster@backend#pool', 'source_volid': None, 'snapshot_id': snapshot.id, 'image_id': 4, @@ -110,6 +123,7 @@ class CreateVolumeFlowTestCase(test.TestCase): fake_volume_api.FakeVolumeAPI(spec, self), fake_volume_api.FakeDb()) + spec.pop('resource_backend') task._cast_create_volume(self.ctxt, spec, {}) mock_snapshot_get.assert_called_once_with(self.ctxt, snapshot.id) mock_extract_host.assert_not_called() @@ -124,10 +138,14 @@ class CreateVolumeFlowTestCase(test.TestCase): volume = fake_volume.fake_volume_obj(self.ctxt) volume_get_by_id.return_value = volume props = {} - cg_obj = (fake_consistencygroup. - fake_consistencyobject_obj(self.ctxt, consistencygroup_id=1, - host='host@backend#pool')) + cg_obj = fake_consistencygroup.fake_consistencyobject_obj( + self.ctxt, + consistencygroup_id=1, + host='host@backend#pool', + cluster_name='cluster@backend#pool') consistencygroup_get_by_id.return_value = cg_obj + mock_extract_host.return_value = 'cluster@backend' + spec = {'volume_id': None, 'volume': None, 'source_volid': None, @@ -145,9 +163,14 @@ class CreateVolumeFlowTestCase(test.TestCase): fake_volume_api.FakeDb()) task._cast_create_volume(self.ctxt, spec, props) + consistencygroup_get_by_id.assert_not_called() + mock_extract_host.assert_not_called() + # This is the spec for a volume created from a consistency group. It + # includes the 'resource_backend'. spec = {'volume_id': volume.id, 'volume': volume, + 'resource_backend': 'cluster@backend', 'source_volid': 2, 'snapshot_id': 3, 'image_id': 4, @@ -162,9 +185,14 @@ class CreateVolumeFlowTestCase(test.TestCase): fake_volume_api.FakeVolumeAPI(spec, self), fake_volume_api.FakeDb()) + # Remove 'resource_backend' prior to calling task._cast_create_volume + # (the point of the test is to confirm that it adds it to the spec + # sent to the scheduler). + spec.pop('resource_backend') + task._cast_create_volume(self.ctxt, spec, props) consistencygroup_get_by_id.assert_called_once_with(self.ctxt, 5) - mock_extract_host.assert_called_once_with('host@backend#pool') + mock_extract_host.assert_called_once_with('cluster@backend#pool') @mock.patch('cinder.db.volume_create') @mock.patch('cinder.objects.Volume.get_by_id') diff --git a/cinder/volume/flows/api/create_volume.py b/cinder/volume/flows/api/create_volume.py index 77dd0d49a53..12569f59c10 100644 --- a/cinder/volume/flows/api/create_volume.py +++ b/cinder/volume/flows/api/create_volume.py @@ -741,13 +741,13 @@ class VolumeCastTask(flow_utils.CinderTask): # to choose a proper pool whose backend is same as CG's backend. cgroup = objects.ConsistencyGroup.get_by_id(context, cgroup_id) request_spec['resource_backend'] = vol_utils.extract_host( - cgroup.host) + cgroup.resource_backend) elif group_id: # If group_id exists, we should cast volume to the scheduler # to choose a proper pool whose backend is same as group's backend. group = objects.Group.get_by_id(context, group_id) request_spec['resource_backend'] = vol_utils.extract_host( - group.host) + group.resource_backend) elif snapshot_id and CONF.snapshot_same_host: # NOTE(Rongze Zhu): A simple solution for bug 1008866. # @@ -759,10 +759,11 @@ class VolumeCastTask(flow_utils.CinderTask): # before creating volume, we schedule this request to scheduler # service with the desired backend information. snapshot = objects.Snapshot.get_by_id(context, snapshot_id) - request_spec['resource_backend'] = snapshot.volume.host + request_spec['resource_backend'] = snapshot.volume.resource_backend elif source_volid: source_volume_ref = objects.Volume.get_by_id(context, source_volid) - request_spec['resource_backend'] = source_volume_ref.host + request_spec['resource_backend'] = ( + source_volume_ref.resource_backend) self.scheduler_rpcapi.create_volume( context,