From 70afc0d5408aaae8beb587682fe26c124c0cacee Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Sun, 5 Feb 2017 16:32:44 -0500 Subject: [PATCH] Handle conflicts for os-assisted-volume-snapshots Since a guest-assisted disk snapshot is performed on the compute that the instance is running on, there are only certain states that the instance can be in to perform this operation. For example, if the instance is shelved_offloaded then the instance does not have a host and we can't cast to a compute to perform the snapshot. Given how unrestrictive this API was before, the only restriction we place on the state is that the instance does not have a task_state set. We allow any vm_state for performing the operation as long as there is a host and no task_state. As noted in the code, we'd normally return a 409 in this case but according to our microversion docs [1] that would be a new error code and require a version bump, so this change just uses 400 and leaves a TODO to make this 409 in a later mass return code update microversion. [1] https://docs.openstack.org/developer/nova/api_microversion_dev.html#f1 Change-Id: I1dc54a38f02bb48921bcbc4c2fdcc2c946e783c1 Closes-Bug: #1657585 --- .../compute/assisted_volume_snapshots.py | 14 ++++ nova/compute/api.py | 36 +++++++--- .../api/openstack/compute/test_volumes.py | 54 ++++++++++++++ nova/tests/unit/compute/test_compute_api.py | 72 ++++++++++++++++++- .../notes/bug-1657585-99b7eddc40c71e5a.yaml | 7 ++ 5 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-1657585-99b7eddc40c71e5a.yaml diff --git a/nova/api/openstack/compute/assisted_volume_snapshots.py b/nova/api/openstack/compute/assisted_volume_snapshots.py index 7ea1f88f9668..97052e346b71 100644 --- a/nova/api/openstack/compute/assisted_volume_snapshots.py +++ b/nova/api/openstack/compute/assisted_volume_snapshots.py @@ -57,6 +57,13 @@ class AssistedVolumeSnapshotsController(wsgi.Controller): exception.VolumeBDMIsMultiAttach, exception.InvalidVolume) as error: raise exc.HTTPBadRequest(explanation=error.format_message()) + except (exception.InstanceInvalidState, + exception.InstanceNotReady) as e: + # TODO(mriedem) InstanceInvalidState and InstanceNotReady would + # normally result in a 409 but that would require bumping the + # microversion, which we should just do in a single microversion + # across all APIs when we fix status code wrinkles. + raise exc.HTTPBadRequest(explanation=e.format_message()) @wsgi.response(204) @extensions.expected_errors((400, 404)) @@ -82,6 +89,13 @@ class AssistedVolumeSnapshotsController(wsgi.Controller): raise exc.HTTPBadRequest(explanation=error.format_message()) except exception.NotFound as e: return exc.HTTPNotFound(explanation=e.format_message()) + except (exception.InstanceInvalidState, + exception.InstanceNotReady) as e: + # TODO(mriedem) InstanceInvalidState and InstanceNotReady would + # normally result in a 409 but that would require bumping the + # microversion, which we should just do in a single microversion + # across all APIs when we fix status code wrinkles. + raise exc.HTTPBadRequest(explanation=e.format_message()) class AssistedVolumeSnapshots(extensions.V21APIExtensionBase): diff --git a/nova/compute/api.py b/nova/compute/api.py index 534f0c0c2cdd..d79ff4223446 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -4038,22 +4038,38 @@ class API(base.Base): def volume_snapshot_create(self, context, volume_id, create_info): bdm = objects.BlockDeviceMapping.get_by_volume( context, volume_id, expected_attrs=['instance']) - self.compute_rpcapi.volume_snapshot_create(context, bdm.instance, - volume_id, create_info) - snapshot = { - 'snapshot': { - 'id': create_info.get('id'), - 'volumeId': volume_id + + # We allow creating the snapshot in any vm_state as long as there is + # no task being performed on the instance and it has a host. + @check_instance_host + @check_instance_state(vm_state=None) + def do_volume_snapshot_create(self, context, instance): + self.compute_rpcapi.volume_snapshot_create(context, instance, + volume_id, create_info) + snapshot = { + 'snapshot': { + 'id': create_info.get('id'), + 'volumeId': volume_id + } } - } - return snapshot + return snapshot + + return do_volume_snapshot_create(self, context, bdm.instance) def volume_snapshot_delete(self, context, volume_id, snapshot_id, delete_info): bdm = objects.BlockDeviceMapping.get_by_volume( context, volume_id, expected_attrs=['instance']) - self.compute_rpcapi.volume_snapshot_delete(context, bdm.instance, - volume_id, snapshot_id, delete_info) + + # We allow deleting the snapshot in any vm_state as long as there is + # no task being performed on the instance and it has a host. + @check_instance_host + @check_instance_state(vm_state=None) + def do_volume_snapshot_delete(self, context, instance): + self.compute_rpcapi.volume_snapshot_delete(context, instance, + volume_id, snapshot_id, delete_info) + + do_volume_snapshot_delete(self, context, bdm.instance) def external_instance_event(self, context, instances, mappings, events): # NOTE(danms): The external API consumer just provides events, diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index ad73c7ecd1ee..d38d94049e52 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -29,6 +29,7 @@ from nova.api.openstack.compute import assisted_volume_snapshots \ from nova.api.openstack.compute import volumes as volumes_v21 from nova.compute import api as compute_api from nova.compute import flavors +from nova.compute import task_states from nova.compute import vm_states import nova.conf from nova import context @@ -796,6 +797,33 @@ class AssistedSnapshotCreateTestCaseV21(test.NoDBTestCase): self.assertRaises( webob.exc.HTTPBadRequest, self.controller.create, req, body=body) + def _test_assisted_create_instance_conflict(self, api_error): + # unset the stub on volume_snaphost_create from setUp + self.mox.UnsetStubs() + req = fakes.HTTPRequest.blank('/v2/fake/os-assisted-volume-snapshots') + body = {'snapshot': + {'volume_id': '1', + 'create_info': {'type': 'qcow2', + 'new_file': 'new_file', + 'snapshot_id': 'snapshot_id'}}} + req.method = 'POST' + with mock.patch.object(compute_api.API, 'volume_snapshot_create', + side_effect=api_error): + self.assertRaises( + webob.exc.HTTPBadRequest, self.controller.create, + req, body=body) + + def test_assisted_create_instance_invalid_state(self): + api_error = exception.InstanceInvalidState( + instance_uuid=FAKE_UUID, attr='task_state', + state=task_states.SHELVING_OFFLOADING, + method='volume_snapshot_create') + self._test_assisted_create_instance_conflict(api_error) + + def test_assisted_create_instance_not_ready(self): + api_error = exception.InstanceNotReady(instance_id=FAKE_UUID) + self._test_assisted_create_instance_conflict(api_error) + class AssistedSnapshotDeleteTestCaseV21(test.NoDBTestCase): assisted_snaps = assisted_snaps_v21 @@ -828,6 +856,32 @@ class AssistedSnapshotDeleteTestCaseV21(test.NoDBTestCase): self.assertRaises(webob.exc.HTTPBadRequest, self.controller.delete, req, '5') + def _test_assisted_delete_instance_conflict(self, api_error): + # unset the stub on volume_snapshot_delete from setUp + self.mox.UnsetStubs() + params = { + 'delete_info': jsonutils.dumps({'volume_id': '1'}), + } + req = fakes.HTTPRequest.blank( + '/v2/fake/os-assisted-volume-snapshots?%s' % + urllib.parse.urlencode(params)) + req.method = 'DELETE' + with mock.patch.object(compute_api.API, 'volume_snapshot_delete', + side_effect=api_error): + self.assertRaises( + webob.exc.HTTPBadRequest, self.controller.delete, req, '5') + + def test_assisted_delete_instance_invalid_state(self): + api_error = exception.InstanceInvalidState( + instance_uuid=FAKE_UUID, attr='task_state', + state=task_states.UNSHELVING, + method='volume_snapshot_delete') + self._test_assisted_delete_instance_conflict(api_error) + + def test_assisted_delete_instance_not_ready(self): + api_error = exception.InstanceNotReady(instance_id=FAKE_UUID) + self._test_assisted_delete_instance_conflict(api_error) + class TestAssistedVolumeSnapshotsPolicyEnforcementV21(test.NoDBTestCase): diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index eac42621421b..fbb8231c0878 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -2793,7 +2793,9 @@ class _ComputeAPIUnitTestMixIn(object): 'connection_info': "{'fake': 'connection_info'}", 'volume_id': 1, 'boot_index': -1}) - fake_bdm['instance'] = fake_instance.fake_db_instance() + fake_bdm['instance'] = fake_instance.fake_db_instance( + launched_at=timeutils.utcnow(), + vm_state=vm_states.ACTIVE) fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid'] fake_bdm = objects.BlockDeviceMapping._from_db_object( self.context, objects.BlockDeviceMapping(), @@ -2823,6 +2825,38 @@ class _ComputeAPIUnitTestMixIn(object): } self.assertEqual(snapshot, expected_snapshot) + @mock.patch.object( + objects.BlockDeviceMapping, 'get_by_volume', + return_value=objects.BlockDeviceMapping( + instance=objects.Instance( + launched_at=timeutils.utcnow(), uuid=uuids.instance_uuid, + vm_state=vm_states.ACTIVE, task_state=task_states.SHELVING, + host='fake_host'))) + def test_volume_snapshot_create_shelving(self, bdm_get_by_volume): + """Tests a negative scenario where the instance task_state is not + accepted for creating a guest-assisted volume snapshot. + """ + self.assertRaises(exception.InstanceInvalidState, + self.compute_api.volume_snapshot_create, + self.context, mock.sentinel.volume_id, + mock.sentinel.create_info) + + @mock.patch.object( + objects.BlockDeviceMapping, 'get_by_volume', + return_value=objects.BlockDeviceMapping( + instance=objects.Instance( + launched_at=timeutils.utcnow(), uuid=uuids.instance_uuid, + vm_state=vm_states.SHELVED_OFFLOADED, task_state=None, + host=None))) + def test_volume_snapshot_create_shelved_offloaded(self, bdm_get_by_volume): + """Tests a negative scenario where the instance is shelved offloaded + so we don't have a host to cast to for the guest-assisted snapshot. + """ + self.assertRaises(exception.InstanceNotReady, + self.compute_api.volume_snapshot_create, + self.context, mock.sentinel.volume_id, + mock.sentinel.create_info) + def test_volume_snapshot_delete(self): volume_id = '1' snapshot_id = '2' @@ -2834,7 +2868,9 @@ class _ComputeAPIUnitTestMixIn(object): 'connection_info': "{'fake': 'connection_info'}", 'volume_id': 1, 'boot_index': -1}) - fake_bdm['instance'] = fake_instance.fake_db_instance() + fake_bdm['instance'] = fake_instance.fake_db_instance( + launched_at=timeutils.utcnow(), + vm_state=vm_states.STOPPED) fake_bdm['instance_uuid'] = fake_bdm['instance']['uuid'] fake_bdm = objects.BlockDeviceMapping._from_db_object( self.context, objects.BlockDeviceMapping(), @@ -2856,6 +2892,38 @@ class _ComputeAPIUnitTestMixIn(object): self.compute_api.volume_snapshot_delete(self.context, volume_id, snapshot_id, {}) + @mock.patch.object( + objects.BlockDeviceMapping, 'get_by_volume', + return_value=objects.BlockDeviceMapping( + instance=objects.Instance( + launched_at=timeutils.utcnow(), uuid=uuids.instance_uuid, + vm_state=vm_states.ACTIVE, task_state=task_states.SHELVING, + host='fake_host'))) + def test_volume_snapshot_delete_shelving(self, bdm_get_by_volume): + """Tests a negative scenario where the instance is shelving and the + task_state is set so we can't perform the guest-assisted snapshot. + """ + self.assertRaises(exception.InstanceInvalidState, + self.compute_api.volume_snapshot_delete, + self.context, mock.sentinel.volume_id, + mock.sentinel.snapshot_id, mock.sentinel.delete_info) + + @mock.patch.object( + objects.BlockDeviceMapping, 'get_by_volume', + return_value=objects.BlockDeviceMapping( + instance=objects.Instance( + launched_at=timeutils.utcnow(), uuid=uuids.instance_uuid, + vm_state=vm_states.SHELVED_OFFLOADED, task_state=None, + host=None))) + def test_volume_snapshot_delete_shelved_offloaded(self, bdm_get_by_volume): + """Tests a negative scenario where the instance is shelved offloaded + so there is no host to cast to for the guest-assisted snapshot delete. + """ + self.assertRaises(exception.InstanceNotReady, + self.compute_api.volume_snapshot_delete, + self.context, mock.sentinel.volume_id, + mock.sentinel.snapshot_id, mock.sentinel.delete_info) + def _test_boot_volume_bootable(self, is_bootable=False): def get_vol_data(*args, **kwargs): return {'bootable': is_bootable} diff --git a/releasenotes/notes/bug-1657585-99b7eddc40c71e5a.yaml b/releasenotes/notes/bug-1657585-99b7eddc40c71e5a.yaml new file mode 100644 index 000000000000..439179d1c0eb --- /dev/null +++ b/releasenotes/notes/bug-1657585-99b7eddc40c71e5a.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The ``POST`` and ``DELETE`` operations on the + ``os-assisted-volume-snapshots`` API will now fail with a 400 error if the + related instance is undergoing a task state transition or does not have a + host, i.e. is shelved offloaded.