Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
catch InstanceInvalidState in more places
Further fixes to bug 911879

500s or 400s are returned in the OS API when actions are denied due
to being in an invalid state.  409 should be returned, instead.  A
previous review (2846) fixed the delete case and this fixes more.

When writing tests, I found a number of exceptions that are not raised
anymore, and they were being incorrectly used in tests still.  I fixed
those up.

Change-Id: I0d5b1ed52e0cc9766be8e2a7de84c8601f4bdf26
  • Loading branch information
comstud committed Jan 12, 2012
1 parent 6ece432 commit 475691a
Show file tree
Hide file tree
Showing 10 changed files with 324 additions and 44 deletions.
15 changes: 15 additions & 0 deletions nova/api/openstack/common.py
Expand Up @@ -334,6 +334,21 @@ def _emit_addr(ip, version):
return networks


def raise_http_conflict_for_instance_invalid_state(exc, action):
"""Return a webob.exc.HTTPConflict instance containing a message
appropriate to return via the API based on the original
InstanceInvalidState exception.
"""
attr = exc.kwargs.get('attr')
state = exc.kwargs.get('state')
if attr and state:
msg = _("Cannot '%(action)s' while instance is in %(attr)s %(state)s")
else:
# At least give some meaningful message
msg = _("Instance is in an invalid state for '%(action)s'")
raise webob.exc.HTTPConflict(explanation=msg % locals())


class MetadataDeserializer(wsgi.MetadataXMLDeserializer):
def deserialize(self, text):
dom = minidom.parseString(text)
Expand Down
27 changes: 21 additions & 6 deletions nova/api/openstack/v2/contrib/admin_actions.py
Expand Up @@ -56,6 +56,9 @@ def _pause(self, input_dict, req, id):
try:
server = self.compute_api.get(ctxt, id)
self.compute_api.pause(ctxt, server)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'pause')
except Exception:
readable = traceback.format_exc()
LOG.exception(_("Compute.api::pause %s"), readable)
Expand All @@ -70,6 +73,9 @@ def _unpause(self, input_dict, req, id):
try:
server = self.compute_api.get(ctxt, id)
self.compute_api.unpause(ctxt, server)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'unpause')
except Exception:
readable = traceback.format_exc()
LOG.exception(_("Compute.api::unpause %s"), readable)
Expand All @@ -84,6 +90,9 @@ def _suspend(self, input_dict, req, id):
try:
server = self.compute_api.get(context, id)
self.compute_api.suspend(context, server)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'suspend')
except Exception:
readable = traceback.format_exc()
LOG.exception(_("compute.api::suspend %s"), readable)
Expand All @@ -98,6 +107,9 @@ def _resume(self, input_dict, req, id):
try:
server = self.compute_api.get(context, id)
self.compute_api.resume(context, server)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'resume')
except Exception:
readable = traceback.format_exc()
LOG.exception(_("compute.api::resume %s"), readable)
Expand All @@ -112,6 +124,9 @@ def _migrate(self, input_dict, req, id):
try:
instance = self.compute_api.get(context, id)
self.compute_api.resize(req.environ['nova.context'], instance)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'migrate')
except Exception, e:
LOG.exception(_("Error in migrate %s"), e)
raise exc.HTTPBadRequest()
Expand Down Expand Up @@ -233,12 +248,12 @@ def _create_backup(self, input_dict, req, instance_id):
except exception.NotFound:
raise exc.HTTPNotFound(_("Instance not found"))

image = self.compute_api.backup(context,
instance,
image_name,
backup_type,
rotation,
extra_properties=props)
try:
image = self.compute_api.backup(context, instance, image_name,
backup_type, rotation, extra_properties=props)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'createBackup')

# build location of newly-created image entity
image_id = str(image['id'])
Expand Down
14 changes: 12 additions & 2 deletions nova/api/openstack/v2/contrib/deferred_delete.py
Expand Up @@ -17,9 +17,11 @@

import webob

from nova.api.openstack import common
from nova.api.openstack.v2 import extensions
from nova.api.openstack.v2 import servers
from nova import compute
from nova import exception
from nova import log as logging


Expand All @@ -44,15 +46,23 @@ def _restore(self, input_dict, req, instance_id):

context = req.environ["nova.context"]
instance = self.compute_api.get(context, instance_id)
self.compute_api.restore(context, instance)
try:
self.compute_api.restore(context, instance)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'restore')
return webob.Response(status_int=202)

def _force_delete(self, input_dict, req, instance_id):
"""Force delete of instance before deferred cleanup."""

context = req.environ["nova.context"]
instance = self.compute_api.get(context, instance_id)
self.compute_api.force_delete(context, instance)
try:
self.compute_api.force_delete(context, instance)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'forceDelete')
return webob.Response(status_int=202)

def get_actions(self):
Expand Down
30 changes: 20 additions & 10 deletions nova/api/openstack/v2/servers.py
Expand Up @@ -826,6 +826,9 @@ def _action_confirm_resize(self, input_dict, req, id):
except exception.MigrationNotFound:
msg = _("Instance has not been resized.")
raise exc.HTTPBadRequest(explanation=msg)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'confirmResize')
except Exception, e:
LOG.exception(_("Error in confirm-resize %s"), e)
raise exc.HTTPBadRequest()
Expand All @@ -839,6 +842,9 @@ def _action_revert_resize(self, input_dict, req, id):
except exception.MigrationNotFound:
msg = _("Instance has not been resized.")
raise exc.HTTPBadRequest(explanation=msg)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'revertResize')
except Exception, e:
LOG.exception(_("Error in revert-resize %s"), e)
raise exc.HTTPBadRequest()
Expand All @@ -862,6 +868,9 @@ def _action_reboot(self, input_dict, req, id):

try:
self.compute_api.reboot(context, instance, reboot_type)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'reboot')
except Exception, e:
LOG.exception(_("Error in reboot %s"), e)
raise exc.HTTPUnprocessableEntity()
Expand All @@ -880,6 +889,9 @@ def _resize(self, req, instance_id, flavor_id):
except exception.CannotResizeToSameSize:
msg = _("Resize requires a change in size.")
raise exc.HTTPBadRequest(explanation=msg)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'resize')

return webob.Response(status_int=202)

Expand All @@ -893,9 +905,8 @@ def delete(self, req, id):
except exception.NotFound:
raise exc.HTTPNotFound()
except exception.InstanceInvalidState as state_error:
state = state_error.kwargs.get("state")
msg = _("Unable to delete instance when %s") % state
raise exc.HTTPConflict(explanation=msg)
common.raise_http_conflict_for_instance_invalid_state(state_error,
'delete')

def _get_key_name(self, req, body):
if 'server' in body:
Expand Down Expand Up @@ -1009,10 +1020,9 @@ def _action_rebuild(self, info, request, instance_id):
image_href,
password,
**kwargs)

except exception.RebuildRequiresActiveInstance:
msg = _("Instance must be active to rebuild.")
raise exc.HTTPConflict(explanation=msg)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'rebuild')
except exception.InstanceNotFound:
msg = _("Instance could not be found")
raise exc.HTTPNotFound(explanation=msg)
Expand Down Expand Up @@ -1064,9 +1074,9 @@ def _action_create_image(self, input_dict, req, instance_id):
instance,
image_name,
extra_properties=props)
except exception.InstanceBusy:
msg = _("Server is currently creating an image. Please wait.")
raise webob.exc.HTTPConflict(explanation=msg)
except exception.InstanceInvalidState as state_error:
common.raise_http_conflict_for_instance_invalid_state(state_error,
'createImage')

# build location of newly-created image entity
image_id = str(image['id'])
Expand Down
2 changes: 1 addition & 1 deletion nova/api/openstack/wsgi.py
Expand Up @@ -919,7 +919,7 @@ class Fault(webob.exc.HTTPException):
403: "resizeNotAllowed",
404: "itemNotFound",
405: "badMethod",
409: "inProgress",
409: "inProgress", # FIXME(comstud): This doesn't seem right
413: "overLimit",
415: "badMediaType",
501: "notImplemented",
Expand Down
16 changes: 0 additions & 16 deletions nova/exception.py
Expand Up @@ -91,10 +91,6 @@ def __init__(self, message='Unknown', code=None):
super(ApiError, self).__init__(outstr)


class RebuildRequiresActiveInstance(Error):
pass


class DBError(Error):
"""Wraps an implementation specific exception."""
def __init__(self, inner_exception=None):
Expand Down Expand Up @@ -211,18 +207,6 @@ class PolicyNotAllowed(NotAuthorized):
message = _("Policy Doesn't allow %(action)s to be performed.")


class InstanceBusy(NovaException):
message = _("Instance %(instance_id)s is busy. (%(task_state)s)")


class InstanceSnapshotting(InstanceBusy):
message = _("Instance %(instance_uuid)s is currently snapshotting.")


class InstanceBackingUp(InstanceBusy):
message = _("Instance %(instance_uuid)s is currently being backed up.")


class Invalid(NovaException):
message = _("Unacceptable parameters.")

Expand Down
33 changes: 31 additions & 2 deletions nova/tests/api/openstack/test_common.py
Expand Up @@ -21,10 +21,11 @@

from lxml import etree
import webob.exc
import xml.dom.minidom as minidom

# FIXME(comstud): Don't import classes (HACKING)
from webob import Request
import xml.dom.minidom as minidom

from nova import exception
from nova import test
from nova.api.openstack import common
from nova.api.openstack import xmlutil
Expand Down Expand Up @@ -299,6 +300,34 @@ def test_get_version_from_href_default(self):
actual = common.get_version_from_href(fixture)
self.assertEqual(actual, expected)

def test_raise_http_conflict_for_instance_invalid_state(self):
# Correct args
exc = exception.InstanceInvalidState(attr='fake_attr',
state='fake_state', method='fake_method')
try:
common.raise_http_conflict_for_instance_invalid_state(exc,
'meow')
except Exception, e:
self.assertTrue(isinstance(e, webob.exc.HTTPConflict))
msg = str(e)
self.assertEqual(msg,
"Cannot 'meow' while instance is in fake_attr fake_state")
else:
self.fail("webob.exc.HTTPConflict was not raised")

# Incorrect args
exc = exception.InstanceInvalidState()
try:
common.raise_http_conflict_for_instance_invalid_state(exc,
'meow')
except Exception, e:
self.assertTrue(isinstance(e, webob.exc.HTTPConflict))
msg = str(e)
self.assertEqual(msg,
"Instance is in an invalid state for 'meow'")
else:
self.fail("webob.exc.HTTPConflict was not raised")


class MetadataXMLDeserializationTest(test.TestCase):

Expand Down
52 changes: 50 additions & 2 deletions nova/tests/api/openstack/v2/contrib/test_admin_actions.py
Expand Up @@ -21,6 +21,7 @@
from nova.api.openstack.v2 import extensions
from nova.api.openstack import wsgi
from nova import compute
from nova import exception
from nova import flags
from nova import test
from nova import utils
Expand All @@ -46,10 +47,14 @@
}


def fake_compute_api(cls, req, id):
def fake_compute_api(*args, **kwargs):
return True


def fake_compute_api_raises_invalid_state(*args, **kwargs):
raise exception.InstanceInvalidState


def fake_compute_api_get(self, context, instance_id):
return {'id': 1, 'uuid': instance_id}

Expand All @@ -62,6 +67,14 @@ class AdminActionsTest(test.TestCase):
_methods = ('pause', 'unpause', 'suspend', 'resume', 'resize',
'reset_network', 'inject_network_info', 'lock', 'unlock')

_actions_that_check_state = (
# action, method
('pause', 'pause'),
('unpause', 'unpause'),
('suspend', 'suspend'),
('resume', 'resume'),
('migrate', 'resize'))

def setUp(self):
super(AdminActionsTest, self).setUp()
self.stubs.Set(compute.API, 'get', fake_compute_api_get)
Expand All @@ -74,13 +87,32 @@ def test_admin_api_actions(self):
self.maxDiff = None
app = fakes.wsgi_app()
for _action in self._actions:
req = webob.Request.blank('/v2/fake/servers/%s/action' % self.UUID)
req = webob.Request.blank('/v2/fake/servers/%s/action' %
self.UUID)
req.method = 'POST'
req.body = json.dumps({_action: None})
req.content_type = 'application/json'
res = req.get_response(app)
self.assertEqual(res.status_int, 202)

def test_admin_api_actions_raise_conflict_on_invalid_state(self):
self.maxDiff = None
app = fakes.wsgi_app()

for _action, _method in self._actions_that_check_state:
self.stubs.Set(compute.API, _method,
fake_compute_api_raises_invalid_state)

req = webob.Request.blank('/v2/fake/servers/%s/action' %
self.UUID)
req.method = 'POST'
req.body = json.dumps({_action: None})
req.content_type = 'application/json'
res = req.get_response(app)
self.assertEqual(res.status_int, 409)
self.assertIn("invalid state for '%(_action)s'" % locals(),
res.body)


class CreateBackupTests(test.TestCase):

Expand Down Expand Up @@ -200,3 +232,19 @@ def test_create_backup(self):
instance_ref = self.backup_stubs.extra_props_last_call['instance_ref']
expected_server_location = 'http://localhost/v2/servers/%s' % self.uuid
self.assertEqual(expected_server_location, instance_ref)

def test_create_backup_raises_conflict_on_invalid_state(self):
body = {
'createBackup': {
'name': 'Backup 1',
'backup_type': 'daily',
'rotation': 1,
},
}

self.stubs.Set(compute.API, 'backup',
fake_compute_api_raises_invalid_state)

request = self._get_request(body)
response = request.get_response(self.app)
self.assertEqual(response.status_int, 409)

0 comments on commit 475691a

Please sign in to comment.