From 12ef157c3b71124e91ccb9fdba04ec9e3c3f0fd3 Mon Sep 17 00:00:00 2001 From: haixin Date: Fri, 13 Oct 2023 16:12:07 +0800 Subject: [PATCH] Add disabled reason field to service. update micversion to 2.83 user can set disabled reason for service. Closes-Bug: #2037700 Change-Id: I3d7c46945366ac9e1d305c2f6de2233859259bf7 --- manila/api/openstack/api_version_request.py | 3 +- .../openstack/rest_api_version_history.rst | 6 ++ manila/api/v2/services.py | 42 ++++++++++-- manila/api/views/services.py | 33 +++++++-- ...328f0a3d2_add_disable_reason_to_service.py | 49 +++++++++++++ manila/db/sqlalchemy/models.py | 1 + manila/tests/api/v2/test_services.py | 68 ++++++++++++++++++- .../alembic/migrations_data_checks.py | 32 +++++++++ ...d_reason_to_services-8369aaa2985ada25.yaml | 8 +++ 9 files changed, 225 insertions(+), 17 deletions(-) create mode 100644 manila/db/migrations/alembic/versions/99d328f0a3d2_add_disable_reason_to_service.py create mode 100644 releasenotes/notes/add_disabled_reason_to_services-8369aaa2985ada25.yaml diff --git a/manila/api/openstack/api_version_request.py b/manila/api/openstack/api_version_request.py index 228cfe6757..95f6cc1323 100644 --- a/manila/api/openstack/api_version_request.py +++ b/manila/api/openstack/api_version_request.py @@ -200,13 +200,14 @@ REST_API_VERSION_HISTORY = """ * 2.80 - Added share backup APIs. * 2.81 - Added API methods, endpoint /resource-locks. * 2.82 - Added lock and restriction to share access rules. + * 2.83 - Added 'disabled_reason' field to services. """ # The minimum and maximum versions of the API supported # The default api version request is defined to be the # minimum version of the API supported. _MIN_API_VERSION = "2.0" -_MAX_API_VERSION = "2.82" +_MAX_API_VERSION = "2.83" DEFAULT_API_VERSION = _MIN_API_VERSION diff --git a/manila/api/openstack/rest_api_version_history.rst b/manila/api/openstack/rest_api_version_history.rst index 5ad30efa97..5484bb0a14 100644 --- a/manila/api/openstack/rest_api_version_history.rst +++ b/manila/api/openstack/rest_api_version_history.rst @@ -444,3 +444,9 @@ user documentation. ------------------------------- Introduce the ability to lock access rules and restrict the visibility of sensitive fields. + +2.83 +---- + The ``disabled_reason`` field was added to the service to mark the reason why + the user disabled the service. ``disabled`` field will be replaced by + ``status`` field. diff --git a/manila/api/v2/services.py b/manila/api/v2/services.py index 337a1ded5a..953a780ba2 100644 --- a/manila/api/v2/services.py +++ b/manila/api/v2/services.py @@ -14,11 +14,13 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_utils import strutils import webob.exc from manila.api.openstack import wsgi from manila.api.views import services as services_views from manila import db +from manila.i18n import _ class ServiceMixin(object): @@ -46,6 +48,7 @@ class ServiceMixin(object): 'host': service['host'], 'zone': service['availability_zone']['name'], 'status': 'disabled' if service['disabled'] else 'enabled', + 'disabled_reason': service.get('disabled_reason'), 'state': service['state'], 'updated_at': service['updated_at'], } @@ -65,17 +68,38 @@ class ServiceMixin(object): if len(services) == 0: break - return self._view_builder.detail_list(services) + return self._view_builder.detail_list(req, services) @wsgi.Controller.authorize("update") - def _update(self, req, id, body): + def _update(self, req, id, body, support_disabled_reason=True): """Enable/Disable scheduling for a service.""" context = req.environ['manila.context'] + update_dict = {} if id == "enable": data = {'disabled': False} + if support_disabled_reason: + update_dict['disabled_reason'] = None elif id == "disable": data = {'disabled': True} + disabled_reason = body.get('disabled_reason') + if disabled_reason and not support_disabled_reason: + msg = _("'disabled_reason' option is not supported by this " + "microversion. Use 2.83 or greater microversion to " + "be able to set 'disabled_reason'.") + raise webob.exc.HTTPBadRequest(explanation=msg) + if disabled_reason: + try: + strutils.check_string_length(disabled_reason.strip(), + name='disabled_reason', + min_length=1, + max_length=255) + except (ValueError, TypeError): + msg = _('Disabled reason contains invalid characters ' + 'or is too long') + raise webob.exc.HTTPBadRequest(explanation=msg) + update_dict['disabled_reason'] = disabled_reason.strip() + data['disabled_reason'] = disabled_reason.strip() else: raise webob.exc.HTTPNotFound("Unknown action '%s'" % id) @@ -86,10 +110,12 @@ class ServiceMixin(object): raise webob.exc.HTTPBadRequest() svc = db.service_get_by_args(context, data['host'], data['binary']) - db.service_update( - context, svc['id'], {'disabled': data['disabled']}) + update_dict['disabled'] = data['disabled'] - return self._view_builder.summary(data) + db.service_update(context, svc['id'], update_dict) + data['status'] = 'disabled' if id == "disable" else 'enabled' + + return self._view_builder.summary(req, data) class ServiceControllerLegacy(ServiceMixin, wsgi.Controller): @@ -119,8 +145,12 @@ class ServiceController(ServiceMixin, wsgi.Controller): def index(self, req): return self._index(req) - @wsgi.Controller.api_version('2.7') + @wsgi.Controller.api_version('2.7', '2.82') def update(self, req, id, body): + return self._update(req, id, body, support_disabled_reason=False) + + @wsgi.Controller.api_version('2.83') # noqa + def update(self, req, id, body): # pylint: disable=function-redefined # noqa F811 return self._update(req, id, body) diff --git a/manila/api/views/services.py b/manila/api/views/services.py index 759ffd0c1c..d0db7df3c9 100644 --- a/manila/api/views/services.py +++ b/manila/api/views/services.py @@ -19,14 +19,33 @@ from manila.api import common class ViewBuilder(common.ViewBuilder): _collection_name = "services" + _detail_version_modifiers = [ + "add_disabled_reason_field", + ] - def summary(self, service): + def summary(self, request, service): """Summary view of a single service.""" - keys = 'host', 'binary', 'disabled' - return {key: service.get(key) for key in keys} + keys = 'host', 'binary', 'status', + service_dict = {key: service.get(key) for key in keys} + self.update_versioned_resource_dict(request, service_dict, service) + return service_dict - def detail_list(self, services): + def detail(self, request, service): + """Detailed view of a single service.""" + keys = ('id', 'binary', 'host', 'zone', 'status', + 'state', 'updated_at') + service_dict = {key: service.get(key) for key in keys} + self.update_versioned_resource_dict(request, service_dict, service) + return service_dict + + def detail_list(self, request, services): """Detailed view of a list of services.""" - keys = 'id', 'binary', 'host', 'zone', 'status', 'state', 'updated_at' - views = [{key: s.get(key) for key in keys} for s in services] - return {self._collection_name: views} + services_list = [self.detail(request, s) for s in services] + services_dict = dict(services=services_list) + return services_dict + + @common.ViewBuilder.versioned_method("2.83") + def add_disabled_reason_field(self, context, service_dict, service): + service_dict.pop('disabled', None) + service_dict['status'] = service.get('status') + service_dict['disabled_reason'] = service.get('disabled_reason') diff --git a/manila/db/migrations/alembic/versions/99d328f0a3d2_add_disable_reason_to_service.py b/manila/db/migrations/alembic/versions/99d328f0a3d2_add_disable_reason_to_service.py new file mode 100644 index 0000000000..dce7e9970c --- /dev/null +++ b/manila/db/migrations/alembic/versions/99d328f0a3d2_add_disable_reason_to_service.py @@ -0,0 +1,49 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""add_disable_reason_to_service + +Revision ID: 99d328f0a3d2 +Revises: 2d708a9a3ba9 +Create Date: 2023-10-13 10:50:29.311032 + +""" + +# revision identifiers, used by Alembic. +revision = '99d328f0a3d2' +down_revision = '2d708a9a3ba9' + +from alembic import op +from oslo_log import log +import sqlalchemy as sa + + +LOG = log.getLogger(__name__) + + +def upgrade(): + try: + op.add_column( + 'services', sa.Column( + 'disabled_reason', sa.String(length=255), + nullable=True)) + except Exception: + LOG.error("Column services.disabled_reason not created!") + raise + + +def downgrade(): + try: + op.drop_column('services', 'disabled_reason') + except Exception: + LOG.error("Column services.disabled_reason not dropped!") + raise diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index d7c1f12a62..96ef205c85 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -68,6 +68,7 @@ class Service(BASE, ManilaBase): state = Column(String(36)) report_count = Column(Integer, nullable=False, default=0) disabled = Column(Boolean, default=False) + disabled_reason = Column(String(255)) availability_zone_id = Column(String(36), ForeignKey('availability_zones.id'), nullable=True) diff --git a/manila/tests/api/v2/test_services.py b/manila/tests/api/v2/test_services.py index eb56bcbdb9..97141d637c 100644 --- a/manila/tests/api/v2/test_services.py +++ b/manila/tests/api/v2/test_services.py @@ -21,6 +21,7 @@ from unittest import mock import ddt from oslo_utils import timeutils +from manila.api.openstack import api_version_request as api_version from manila.api.v2 import services from manila import context from manila import db @@ -37,6 +38,7 @@ fake_services_list = [ 'availability_zone': {'name': 'manila1'}, 'id': 1, 'disabled': True, + 'disabled_reason': 'test1', 'state': 'up', 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 2), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 27), @@ -47,6 +49,7 @@ fake_services_list = [ 'availability_zone': {'name': 'manila1'}, 'id': 2, 'disabled': True, + 'disabled_reason': 'test2', 'state': 'up', 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 5), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 27)}, @@ -56,6 +59,7 @@ fake_services_list = [ 'availability_zone': {'name': 'manila2'}, 'id': 3, 'disabled': False, + 'disabled_reason': '', 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 19, 6, 55, 34), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28)}, @@ -65,6 +69,7 @@ fake_services_list = [ 'availability_zone': {'name': 'manila2'}, 'id': 4, 'disabled': True, + 'disabled_reason': 'test4', 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28), @@ -111,6 +116,49 @@ fake_response_service_list = {'services': [ }, ]} +fake_response_service_list_with_disabled_reason = {'services': [ + { + 'id': 1, + 'binary': 'manila-scheduler', + 'host': 'host1', + 'zone': 'manila1', + 'status': 'disabled', + 'disabled_reason': 'test1', + 'state': 'up', + 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 2), + }, + { + 'id': 2, + 'binary': 'manila-share', + 'host': 'host1', + 'zone': 'manila1', + 'status': 'disabled', + 'disabled_reason': 'test2', + 'state': 'up', + 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 5), + }, + { + 'id': 3, + 'binary': 'manila-scheduler', + 'host': 'host2', + 'zone': 'manila2', + 'status': 'enabled', + 'disabled_reason': '', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 19, 6, 55, 34), + }, + { + 'id': 4, + 'binary': 'manila-share', + 'host': 'host2', + 'zone': 'manila2', + 'status': 'disabled', + 'disabled_reason': 'test4', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), + }, +]} + def fake_service_get_all(context): return fake_services_list @@ -165,6 +213,7 @@ class ServicesTest(test.TestCase): ('os-services', '1.0', services.ServiceControllerLegacy), ('os-services', '2.6', services.ServiceControllerLegacy), ('services', '2.7', services.ServiceController), + ('services', '2.83', services.ServiceController), ) @ddt.unpack def test_services_list(self, url, version, controller): @@ -173,7 +222,12 @@ class ServicesTest(test.TestCase): res_dict = controller().index(req) - self.assertEqual(fake_response_service_list, res_dict) + if (api_version.APIVersionRequest(version) >= + api_version.APIVersionRequest('2.83')): + self.assertEqual(fake_response_service_list_with_disabled_reason, + res_dict) + else: + self.assertEqual(fake_response_service_list, res_dict) self.mock_policy_check.assert_called_once_with( req.environ['manila.context'], self.resource_name, 'index') @@ -273,7 +327,7 @@ class ServicesTest(test.TestCase): res_dict = controller().update(req, "enable", body) - self.assertFalse(res_dict['disabled']) + self.assertEqual('enabled', res_dict['status']) self.mock_policy_check.assert_called_once_with( req.environ['manila.context'], self.resource_name, 'update') @@ -281,16 +335,24 @@ class ServicesTest(test.TestCase): ('os-services', '1.0', services.ServiceControllerLegacy), ('os-services', '2.6', services.ServiceControllerLegacy), ('services', '2.7', services.ServiceController), + ('services', '2.83', services.ServiceController), ) @ddt.unpack def test_services_disable(self, url, version, controller): req = fakes.HTTPRequest.blank( '/fooproject/%s/disable' % url, version=version) body = {'host': 'host1', 'binary': 'manila-share'} + if (api_version.APIVersionRequest(version) > + api_version.APIVersionRequest("2.83")): + body['disabled_reason'] = 'test1' res_dict = controller().update(req, "disable", body) - self.assertTrue(res_dict['disabled']) + self.assertEqual('disabled', res_dict['status']) + if (api_version.APIVersionRequest(version) > + api_version.APIVersionRequest("2.83")): + self.assertEqual(res_dict['disabled_reason'], 'test1') + self.mock_policy_check.assert_called_once_with( req.environ['manila.context'], self.resource_name, 'update') diff --git a/manila/tests/db/migrations/alembic/migrations_data_checks.py b/manila/tests/db/migrations/alembic/migrations_data_checks.py index 8793253153..0abee4ea2d 100644 --- a/manila/tests/db/migrations/alembic/migrations_data_checks.py +++ b/manila/tests/db/migrations/alembic/migrations_data_checks.py @@ -3372,3 +3372,35 @@ class AddResourceLocks(BaseMigrationChecks): self.test_case.assertRaises(sa_exc.NoSuchTableError, utils.load_table, 'resource_locks', engine) + + +@map_to_migration('99d328f0a3d2') +class ServiceDisabledReason(BaseMigrationChecks): + def _get_service_data(self, options): + base_dict = { + 'binary': 'manila-share', + 'topic': 'share', + 'disabled': False, + 'report_count': '100', + } + base_dict.update(options) + return base_dict + + def setup_upgrade_data(self, engine): + service_fixture = [ + self._get_service_data({'host': 'fake1'}), + self._get_service_data({'host': 'fake2'}), + ] + services_table = utils.load_table('services', engine) + for fixture in service_fixture: + engine.execute(services_table.insert(fixture)) + + def check_upgrade(self, engine, data): + service_table = utils.load_table('services', engine) + for s in engine.execute(service_table.select()): + self.test_case.assertTrue(hasattr(s, 'disabled_reason')) + + def check_downgrade(self, engine): + service_table = utils.load_table('services', engine) + for s in engine.execute(service_table.select()): + self.test_case.assertFalse(hasattr(s, 'disabled_reason')) diff --git a/releasenotes/notes/add_disabled_reason_to_services-8369aaa2985ada25.yaml b/releasenotes/notes/add_disabled_reason_to_services-8369aaa2985ada25.yaml new file mode 100644 index 0000000000..e06cde3dd4 --- /dev/null +++ b/releasenotes/notes/add_disabled_reason_to_services-8369aaa2985ada25.yaml @@ -0,0 +1,8 @@ +--- +features: + - Added a new field 'disabled_reason' to services table. Users can set + 'disabled_reason' when disabling a service, and query the disabled + reason while listing services. When re-enabling the service, the + disabled reason will be cleared. + See `bug 2037700 `_ + for more details.