openstack/nova
Code Issues Proposed changes

Handle conflicts for os-assisted-volume-snapshots

Browse Source 
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
This commit is contained in:
Matt Riedemann 2017-02-05 16:32:44 -05:00 committed by Matt Riedemann
parent 68e808623e
commit 70afc0d540
5 changed files with 171 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
nova/api/openstack/compute/assisted_volume_snapshots.py
View File

@ -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):

36
nova/compute/api.py
View File

@ -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,

54
nova/tests/unit/api/openstack/compute/test_volumes.py
View File

@ -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):

72
nova/tests/unit/compute/test_compute_api.py
View File

@ -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}

7
releasenotes/notes/bug-1657585-99b7eddc40c71e5a.yaml Normal file
View File

@ -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.