Add share-server-delete API

This API is useful when share-server autodeletion is disabled, which is default behaviour.

Changes:
- added new api method with deletion of share server
- added db method, that takes list of shares by share server id
- changed logic of share manager, that is related to concurrency issues
- added unit and tempest tests to cover changes

Partially-implements blueprint add-admin-api-delete-share-server

Change-Id: I03b452de2cd4fe34c648b2434dab1b9244b1b005
This commit is contained in:
Valeriy Ponomaryov 2014-07-01 07:47:14 -04:00
parent b98d3ae6f7
commit 83e94d735e
14 changed files with 310 additions and 34 deletions

View File

@ -72,12 +72,13 @@ class ShareServersAdminTest(base.BaseSharesAdminTest):
# Project id is not empty
self.assertTrue(len(server["project_id"]) > 0)
# Server we used is present
# allow 'creating' status too, because there can be
# another server with same requirements.
# Server we used is present.
# Use 'allowed_statuses' to cover possible statuses of share servers
# in general, because we get info for whole cluster.
allowed_statuses = ["active", "creating", "deleting"]
any((s["share_network_name"] in self.sn_name_and_id and
self.assertIn(s["status"].lower(),
["active", "creating"])) for s in servers)
allowed_statuses)) for s in servers)
@test.attr(type=["gate", "smoke", ])
def test_list_share_servers_with_host_filter(self):
@ -207,3 +208,33 @@ class ShareServersAdminTest(base.BaseSharesAdminTest):
for k, v in details.iteritems():
self.assertTrue(isinstance(k, basestring))
self.assertTrue(isinstance(v, basestring))
@test.attr(type=["gate", "smoke", ])
def test_delete_share_server(self):
# Get client with isolated creds
client = self.get_client_with_isolated_creds()
# Create server with share
__, share = self.create_share(client=client)
# Get share to be able to get its share_network_id
__, share = client.get_share(share["id"])
# Delete share, so we will have share server without shares
client.delete_share(share["id"])
# Wait for share deletion
client.wait_for_resource_deletion(share_id=share["id"])
# List share servers, filtered by share_network_id,
# list with only one item is expected - our share server.
search_opts = {"share_network": share["share_network_id"]}
__, servers = client.list_share_servers(search_opts)
self.assertEqual(len(servers), 1)
# Delete share server
resp, server = client.delete_share_server(servers[0]["id"])
self.assertIn(int(resp["status"]), test.HTTP_SUCCESS)
# Wait for share server deletion
client.wait_for_resource_deletion(server_id=servers[0]["id"])

View File

@ -92,3 +92,15 @@ class ShareServersNegativeAdminTest(base.BaseSharesAdminTest):
}
__, servers = self.shares_client.list_share_servers(search_opts)
self.assertEqual(len(servers), 0)
@test.attr(type=["gate", "smoke", "negative", ])
def test_delete_share_server_with_nonexistent_id(self):
self.assertRaises(exceptions.NotFound,
self.shares_client.delete_share_server,
"fake_nonexistent_share_server_id")
@test.attr(type=["gate", "smoke", "negative", ])
def test_delete_share_server_with_member(self):
self.assertRaises(exceptions.Unauthorized,
self.member_shares_client.delete_share_server,
"fake_nonexistent_share_server_id")

View File

@ -270,7 +270,7 @@ class SharesClient(rest_client.RestClient):
"""Verifies deleted resource or not.
:param kwargs: expected keys are 'share_id', 'rule_id',
:param kwargs: 'snapshot_id', 'sn_id', 'ss_id' and 'vt_id'
:param kwargs: 'snapshot_id', 'sn_id', 'ss_id', 'vt_id' and 'server_id'
:raises share_exceptions.InvalidResource
"""
@ -315,6 +315,12 @@ class SharesClient(rest_client.RestClient):
self.get_volume_type(kwargs.get("vt_id"))
except exceptions.NotFound:
return True
elif "server_id" in kwargs:
# Whether share server deleted or not
try:
self.show_share_server(kwargs.get("server_id"))
except exceptions.NotFound:
return True
else:
raise share_exceptions.InvalidResource(message=str(kwargs))
return False
@ -626,6 +632,11 @@ class SharesClient(rest_client.RestClient):
resp, body = self.get(uri)
return resp, self._parse_resp(body)
def delete_share_server(self, share_server_id):
"""Delete share server by its ID."""
uri = "share-servers/%s" % share_server_id
return self.delete(uri)
def show_share_server(self, share_server_id):
"""Get share server info."""
uri = "share-servers/%s" % share_server_id

View File

@ -32,6 +32,7 @@
"share_server:index": [["rule:admin_api"]],
"share_server:show": [["rule:admin_api"]],
"share_server:details": [["rule:admin_api"]],
"share_server:delete": [["rule:admin_api"]],
"share_network:create": [],
"share_network:delete": [],

View File

@ -14,15 +14,18 @@
# under the License.
import six
import webob
from webob import exc
from manila.api.openstack import wsgi
from manila.api.views import share_servers as share_servers_views
from manila.api import xmlutil
from manila.common import constants
from manila.db import api as db_api
from manila import exception
from manila.openstack.common import log as logging
from manila import policy
from manila import share
RESOURCE_NAME = 'share_server'
RESOURCES_NAME = 'share_servers'
@ -63,7 +66,10 @@ class ShareServersTemplate(xmlutil.TemplateBuilder):
class ShareServerController(wsgi.Controller):
"""The Share Server API controller for the OpenStack API."""
_view_builder_class = share_servers_views.ViewBuilder
def __init__(self):
self.share_api = share.API()
self._view_builder_class = share_servers_views.ViewBuilder
super(ShareServerController, self).__init__()
@wsgi.serializers(xml=ShareServersTemplate)
def index(self, req):
@ -118,6 +124,30 @@ class ShareServerController(wsgi.Controller):
details = db_api.share_server_backend_details_get(context, id)
return self._view_builder.build_share_server_details(details)
def delete(self, req, id):
"""Delete specified share server."""
context = req.environ['manila.context']
policy.check_policy(context, RESOURCE_NAME, 'delete')
try:
share_server = db_api.share_server_get(context, id)
except exception.ShareServerNotFound as e:
raise exc.HTTPNotFound(explanation=str(e))
allowed_statuses = [constants.STATUS_ERROR, constants.STATUS_ACTIVE]
if share_server['status'] not in allowed_statuses:
data = {
'status': share_server['status'],
'allowed_statuses': allowed_statuses,
}
msg = _("Share server's actual status is %(status)s, allowed "
"statuses for deletion are %(allowed_statuses)s.") % (data)
raise exc.HTTPForbidden(explanation=msg)
LOG.debug("Deleting share server with id: %s." % id)
try:
self.share_api.delete_share_server(context, share_server)
except exception.ShareServerInUse as e:
raise exc.HTTPConflict(explanation=str(e))
return webob.Response(status_int=202)
def create_resource():
return wsgi.Resource(ShareServerController())

View File

@ -341,6 +341,11 @@ def share_get_all_by_project(context, project_id):
return IMPL.share_get_all_by_project(context, project_id)
def share_get_all_by_share_server(context, share_server_id):
"""Returns all shares with given share server."""
return IMPL.share_get_all_by_share_server(context, share_server_id)
def share_delete(context, share_id):
"""Delete share."""
return IMPL.share_delete(context, share_id)

View File

@ -1146,6 +1146,13 @@ def share_get_all_by_project(context, project_id):
return _share_get_query(context).filter_by(project_id=project_id).all()
@require_context
def share_get_all_by_share_server(context, share_server_id):
"""Returns list of shares with given share server."""
return _share_get_query(context).filter_by(
share_server_id=share_server_id).all()
@require_context
def share_delete(context, share_id):
session = get_session()

View File

@ -209,6 +209,19 @@ class API(base.Base):
self.share_rpcapi.delete_share(context, share)
def delete_share_server(self, context, server):
"""Delete share server."""
policy.check_policy(context, 'share_server', 'delete', server)
shares = self.db.share_get_all_by_share_server(context, server['id'])
if shares:
raise exception.ShareServerInUse(share_server_id=server['id'])
# NOTE(vponomaryov): There is no share_server status update here,
# it is intentional.
# Status will be changed in manila.share.manager after verification
# for race condition between share creation on server
# and server deletion.
self.share_rpcapi.delete_share_server(context, server)
def create_snapshot(self, context, share, name, description,
force=False):
policy.check_policy(context, 'share', 'create_snapshot', share)

View File

@ -98,10 +98,26 @@ class ShareManager(manager.SchedulerDependentManager):
self.publish_service_capabilities(ctxt)
def _share_server_get_or_create(self, context, share_network_id):
def _provide_share_server_for_share(self, context, share_network_id,
share_id):
"""Gets or creates share_server and updates share with its id.
Active share_server can be deleted if there are no dependent shares
on it.
So we need avoid possibility to delete share_server in time gap
between reaching active state for share_server and setting up
share_server_id for share. It is possible, for example, with first
share creation, which starts share_server creation.
For this purpose used shared lock between this method and the one
with deletion of share_server.
:returns: dict, dict -- first value is share_server, that
has been choosen for share schedule. Second value is
share updated with share_server_id.
"""
@lockutils.synchronized(share_network_id)
def _get_or_create_server():
def _provide_share_server_for_share():
try:
share_server = \
self.db.share_server_get_by_host_and_share_net_valid(
@ -111,9 +127,16 @@ class ShareManager(manager.SchedulerDependentManager):
context, share_network_id)
share_server = self._setup_server(context, share_network)
LOG.info(_("Share server created successfully."))
return share_server
LOG.debug("Using share_server "
"%s for share %s" % (share_server['id'], share_id))
share_ref = self.db.share_update(
context,
share_id,
{'share_server_id': share_server['id']},
)
return share_server, share_ref
return _get_or_create_server()
return _provide_share_server_for_share()
def _get_share_server(self, context, share):
if share['share_server_id']:
@ -143,6 +166,10 @@ class ShareManager(manager.SchedulerDependentManager):
try:
share_server = self.db.share_server_get(context,
parent_share_server_id)
LOG.debug("Using share_server "
"%s for share %s" % (share_server['id'], share_id))
share_ref = self.db.share_update(
context, share_id, {'share_server_id': share_server['id']})
except exception.ShareServerNotFound:
with excutils.save_and_reraise_exception():
LOG.error(_("Share server %s does not exist.")
@ -151,8 +178,8 @@ class ShareManager(manager.SchedulerDependentManager):
{'status': 'error'})
elif share_network_id:
try:
share_server = self._share_server_get_or_create(
context, share_network_id)
share_server, share_ref = self._provide_share_server_for_share(
context, share_network_id, share_id)
except Exception:
with excutils.save_and_reraise_exception():
LOG.error(_("Failed to get share server"
@ -162,11 +189,6 @@ class ShareManager(manager.SchedulerDependentManager):
else:
share_server = None
if share_server:
LOG.debug("Using share server %s" % share_server['id'])
share_ref = self.db.share_update(
context, share_id, {'share_server_id': share_server['id']})
try:
if snapshot_ref:
export_location = self.driver.create_share_from_snapshot(
@ -225,7 +247,10 @@ class ShareManager(manager.SchedulerDependentManager):
if CONF.delete_share_server_with_last_share:
share_server = self._get_share_server(context, share_ref)
if share_server and not share_server.shares:
self._teardown_server(context, share_server)
LOG.debug("Scheduled deletion of share-server "
"with id '%s' automatically by "
"deletion of last share." % share_server['id'])
self.delete_share_server(context, share_server)
def create_snapshot(self, context, share_id, snapshot_id):
"""Create snapshot for share."""
@ -388,10 +413,23 @@ class ShareManager(manager.SchedulerDependentManager):
{'status': constants.STATUS_ERROR})
self.network_api.deallocate_network(context, share_network)
def _teardown_server(self, context, share_server):
def delete_share_server(self, context, share_server):
@lockutils.synchronized(share_server['share_network_id'])
def _teardown_server():
# NOTE(vponomaryov): Verify that there are no dependent shares.
# Without this verification we can get here exception in next case:
# share-server-delete API was called after share creation scheduled
# and share_server reached ACTIVE status, but before update
# of share_server_id field for share. If so, after lock realese
# this method starts executing when amount of dependent shares
# has been changed.
shares = self.db.share_get_all_by_share_server(
context, share_server['id'])
if shares:
raise exception.ShareServerInUse(
share_server_id=share_server['id'])
self.db.share_server_update(context, share_server['id'],
{'status': constants.STATUS_DELETING})
sec_services = self.db.share_network_get(

View File

@ -59,6 +59,10 @@ class ShareAPI(object):
cctxt = self.client.prepare(server=share['host'], version='1.0')
cctxt.cast(ctxt, 'delete_share', share_id=share['id'])
def delete_share_server(self, ctxt, share_server):
cctxt = self.client.prepare(server=share_server['host'], version='1.0')
cctxt.cast(ctxt, 'delete_share_server', share_server=share_server)
def create_snapshot(self, ctxt, share, snapshot):
cctxt = self.client.prepare(server=share['host'])
cctxt.cast(

View File

@ -14,11 +14,13 @@
# under the License.
import mock
from webob import exc
from manila.api.v1 import share_servers
from manila.common import constants
from manila import context
from manila.db import api as db_api
from manila import exception
from manila import policy
from manila import test
@ -242,3 +244,94 @@ class ShareServerAPITest(test.TestCase):
CONTEXT, fake_share_server_get_result['share_server']['id'])
self.assertEqual(result,
fake_share_server_backend_details_get_result)
def test_delete_active_server(self):
share_server = FakeShareServer(status=constants.STATUS_ACTIVE)
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(return_value=share_server))
self.stubs.Set(self.controller.share_api, 'delete_share_server',
mock.Mock())
result = self.controller.delete(FakeRequestAdmin,
fake_share_server_get_result['share_server']['id'])
policy.check_policy.assert_called_once_with(CONTEXT,
share_servers.RESOURCE_NAME, 'delete')
db_api.share_server_get.assert_called_once_with(CONTEXT,
fake_share_server_get_result['share_server']['id'])
self.controller.share_api.delete_share_server.assert_called_once_with(
CONTEXT, share_server)
def test_delete_error_server(self):
share_server = FakeShareServer(status=constants.STATUS_ERROR)
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(return_value=share_server))
self.stubs.Set(self.controller.share_api, 'delete_share_server',
mock.Mock())
result = self.controller.delete(FakeRequestAdmin,
fake_share_server_get_result['share_server']['id'])
policy.check_policy.assert_called_once_with(CONTEXT,
share_servers.RESOURCE_NAME, 'delete')
db_api.share_server_get.assert_called_once_with(CONTEXT,
fake_share_server_get_result['share_server']['id'])
self.controller.share_api.delete_share_server.assert_called_once_with(
CONTEXT, share_server)
def test_delete_used_server(self):
share_server_id = fake_share_server_get_result['share_server']['id']
def raise_not_share_server_in_use(*args, **kwargs):
raise exception.ShareServerInUse(share_server_id=share_server_id)
share_server = fake_share_server_get()
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(return_value=share_server))
self.stubs.Set(self.controller.share_api, 'delete_share_server',
mock.Mock(side_effect=raise_not_share_server_in_use))
self.assertRaises(exc.HTTPConflict,
self.controller.delete,
FakeRequestAdmin,
share_server_id)
db_api.share_server_get.assert_called_once_with(CONTEXT,
share_server_id)
self.controller.share_api.delete_share_server.assert_called_once_with(
CONTEXT, share_server)
def test_delete_not_found(self):
share_server_id = fake_share_server_get_result['share_server']['id']
def raise_not_found(*args, **kwargs):
raise exception.ShareServerNotFound(
share_server_id=share_server_id)
share_server = fake_share_server_get()
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(side_effect=raise_not_found))
self.assertRaises(exc.HTTPNotFound,
self.controller.delete,
FakeRequestAdmin,
share_server_id)
db_api.share_server_get.assert_called_once_with(
CONTEXT, share_server_id)
policy.check_policy.assert_called_once_with(CONTEXT,
share_servers.RESOURCE_NAME, 'delete')
def test_delete_creating_server(self):
share_server = FakeShareServer(status=constants.STATUS_CREATING)
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(return_value=share_server))
self.assertRaises(exc.HTTPForbidden,
self.controller.delete,
FakeRequestAdmin,
share_server['id'])
policy.check_policy.assert_called_once_with(CONTEXT,
share_servers.RESOURCE_NAME, 'delete')
def test_delete_deleting_server(self):
share_server = FakeShareServer(status=constants.STATUS_DELETING)
self.stubs.Set(db_api, 'share_server_get',
mock.Mock(return_value=share_server))
self.assertRaises(exc.HTTPForbidden,
self.controller.delete,
FakeRequestAdmin,
share_server['id'])
policy.check_policy.assert_called_once_with(CONTEXT,
share_servers.RESOURCE_NAME, 'delete')

View File

@ -16,6 +16,7 @@
"share_server:index": [["rule:admin_api"]],
"share_server:show": [["rule:admin_api"]],
"share_server:details": [["rule:admin_api"]],
"share_server:delete": [["rule:admin_api"]],
"share:get_share_metadata": [],
"share:delete_share_metadata": [],

View File

@ -185,6 +185,32 @@ class ShareAPITestCase(test.TestCase):
db_driver.share_snapshot_create.assert_called_once_with(
self.context, options)
@mock.patch.object(db_driver, 'share_get_all_by_share_server',
mock.Mock(return_value=[]))
def test_delete_share_server_no_dependent_shares(self):
server = {'id': 'fake_share_server_id'}
server_returned = {
'id': 'fake_share_server_id',
}
self.stubs.Set(db_driver, 'share_server_update',
mock.Mock(return_value=server_returned))
self.api.delete_share_server(self.context, server)
db_driver.share_get_all_by_share_server.assert_called_once_with(
self.context, server['id'])
self.share_rpcapi.delete_share_server.assert_called_once_with(
self.context, server_returned)
@mock.patch.object(db_driver, 'share_get_all_by_share_server',
mock.Mock(return_value=['fake_share', ]))
def test_delete_share_server_dependent_share_exists(self):
server = {'id': 'fake_share_server_id'}
self.assertRaises(exception.ShareServerInUse,
self.api.delete_share_server,
self.context,
server)
db_driver.share_get_all_by_share_server.assert_called_once_with(
self.context, server['id'])
@mock.patch.object(db_driver, 'share_snapshot_update', mock.Mock())
def test_delete_snapshot(self):
date = datetime.datetime(1, 1, 1, 1, 1, 1)

View File

@ -47,27 +47,24 @@ class ShareRpcAPITestCase(test.TestCase):
snap = {}
snap['share_id'] = share['id']
snapshot = db.share_snapshot_create(self.context, snap)
server = {'id': 'fake_share_server_id', 'host': 'fake_host'}
share_server = db.share_server_create(self.context, server)
self.fake_share = jsonutils.to_primitive(share)
self.fake_access = jsonutils.to_primitive(access)
self.fake_snapshot = jsonutils.to_primitive(snapshot)
self.fake_share_server = jsonutils.to_primitive(share_server)
super(ShareRpcAPITestCase, self).setUp()
self.ctxt = context.RequestContext('fake_user', 'fake_project')
self.rpcapi = share_rpcapi.ShareAPI()
def test_serialized_share_has_id(self):
self.assertTrue('id' in self.fake_share)
def _test_share_api(self, method, rpc_method, **kwargs):
ctxt = context.RequestContext('fake_user', 'fake_project')
if 'rpcapi_class' in kwargs:
rpcapi_class = kwargs['rpcapi_class']
del kwargs['rpcapi_class']
else:
rpcapi_class = share_rpcapi.ShareAPI
rpcapi = rpcapi_class()
expected_retval = 'foo' if method == 'call' else None
target = {
"version": kwargs.pop('version', rpcapi.BASE_RPC_API_VERSION)
"version": kwargs.pop('version', self.rpcapi.BASE_RPC_API_VERSION)
}
expected_msg = copy.deepcopy(kwargs)
if 'share' in expected_msg:
@ -88,6 +85,8 @@ class ShareRpcAPITestCase(test.TestCase):
if 'host' in kwargs:
host = kwargs['host']
elif 'share_server' in kwargs:
host = kwargs['share_server']['host']
else:
host = kwargs['share']['host']
target['server'] = host
@ -96,12 +95,12 @@ class ShareRpcAPITestCase(test.TestCase):
self.fake_args = None
self.fake_kwargs = None
real_prepare = rpcapi.client.prepare
real_prepare = self.rpcapi.client.prepare
def _fake_prepare_method(*args, **kwds):
for kwd in kwds:
self.assertEqual(kwds[kwd], target[kwd])
return rpcapi.client
return self.rpcapi.client
def _fake_rpc_method(*args, **kwargs):
self.fake_args = args
@ -109,13 +108,13 @@ class ShareRpcAPITestCase(test.TestCase):
if expected_retval:
return expected_retval
self.stubs.Set(rpcapi.client, "prepare", _fake_prepare_method)
self.stubs.Set(rpcapi.client, rpc_method, _fake_rpc_method)
self.stubs.Set(self.rpcapi.client, "prepare", _fake_prepare_method)
self.stubs.Set(self.rpcapi.client, rpc_method, _fake_rpc_method)
retval = getattr(rpcapi, method)(ctxt, **kwargs)
retval = getattr(self.rpcapi, method)(self.ctxt, **kwargs)
self.assertEqual(retval, expected_retval)
expected_args = [ctxt, method]
expected_args = [self.ctxt, method]
for arg, expected_arg in zip(self.fake_args, expected_args):
self.assertEqual(arg, expected_arg)
@ -159,3 +158,8 @@ class ShareRpcAPITestCase(test.TestCase):
rpc_method='cast',
snapshot=self.fake_snapshot,
host='fake_host')
def test_delete_share_server(self):
self._test_share_api('delete_share_server',
rpc_method='cast',
share_server=self.fake_share_server)