From ee2a0c890f5f62b4da24a0bae4918f7a1d0ad2d2 Mon Sep 17 00:00:00 2001 From: Harsh Mishra Date: Sat, 28 Feb 2015 06:54:43 +0000 Subject: [PATCH] Fix for inconsistent cinder-services state change MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This bug was introduced to cinder as a side effect of fix : 18365 (https://review.openstack.org/#/c/18365/). API “service_update” is used by periodic and manual service-enable/ disable to update cinder-services table. Due to this ‘updated_at’ column of cinder-services table receives an update for both periodic updates and manual service-enable/disable. Since state of any cinder service is calculated based on the ‘updated_at’ column of the service, we get inconsistent state transitions if a service is manually enabled/disabled and a service-list command is launched before “CONF.service_down_time”, for services which are already in a “down” state. This fix introduces a new column: “modified_at” in cinder-services table. As per the fix all the periodic updates will be updated in ‘updated_at’ column and manual service-enable/disable in “modified_at” column. This will result in correct state transitions as ‘updated_at’ column will only contains time value for periodic updates which in turn would be used to calculate the state of the cinder-service if it is ‘up’/’down’. Also, the time for any service receiving an update will be displayed as “updated_at” or “modified_at”, which ever is latest. Closes-bug: 1397167 Change-Id: Iaee4a9cec035dd60c9de04699b1135ee306f6257 --- cinder/api/contrib/services.py | 10 +- cinder/db/sqlalchemy/api.py | 3 + .../041_add_modified_at_column_to_service.py | 42 ++++++ cinder/db/sqlalchemy/models.py | 5 + cinder/tests/api/contrib/test_services.py | 123 +++++++++++++++++- cinder/tests/test_migrations.py | 10 ++ 6 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 cinder/db/sqlalchemy/migrate_repo/versions/041_add_modified_at_column_to_service.py diff --git a/cinder/api/contrib/services.py b/cinder/api/contrib/services.py index 915ce1f4e..ae716bb7f 100644 --- a/cinder/api/contrib/services.py +++ b/cinder/api/contrib/services.py @@ -106,8 +106,14 @@ class ServiceController(wsgi.Controller): svcs = [] for svc in services: + updated_at = svc['updated_at'] delta = now - (svc['updated_at'] or svc['created_at']) - alive = abs(delta.total_seconds()) <= CONF.service_down_time + delta_sec = delta.total_seconds() + if svc['modified_at']: + delta_mod = now - svc['modified_at'] + if abs(delta_sec) >= abs(delta_mod.total_seconds()): + updated_at = svc['modified_at'] + alive = abs(delta_sec) <= CONF.service_down_time art = (alive and "up") or "down" active = 'enabled' if svc['disabled']: @@ -115,7 +121,7 @@ class ServiceController(wsgi.Controller): ret_fields = {'binary': svc['binary'], 'host': svc['host'], 'zone': svc['availability_zone'], 'status': active, 'state': art, - 'updated_at': svc['updated_at']} + 'updated_at': updated_at} if detailed: ret_fields['disabled_reason'] = svc['disabled_reason'] svcs.append(ret_fields) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 2adf45628..fd4fb02eb 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -433,6 +433,9 @@ def service_update(context, service_id, values): session = get_session() with session.begin(): service_ref = _service_get(context, service_id, session=session) + if ('disabled' in values): + service_ref['modified_at'] = timeutils.utcnow() + service_ref['updated_at'] = literal_column('updated_at') service_ref.update(values) return service_ref diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/041_add_modified_at_column_to_service.py b/cinder/db/sqlalchemy/migrate_repo/versions/041_add_modified_at_column_to_service.py new file mode 100644 index 000000000..be2d33e09 --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/041_add_modified_at_column_to_service.py @@ -0,0 +1,42 @@ +# 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. + +from oslo_log import log as logging +from sqlalchemy import Column, MetaData, DateTime, Table + +from cinder.i18n import _LE + +LOG = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + services = Table('services', meta, autoload=True) + modified_at = Column('modified_at', DateTime(timezone=False)) + try: + services.create_column(modified_at) + except Exception: + LOG.error(_LE("Adding modified_at column to services table failed.")) + raise + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + services = Table('services', meta, autoload=True) + try: + services.drop_column('modified_at') + except Exception: + LOG.error(_LE("Unable to drop modified_at column from services" + "table.")) + raise diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index 77f7105c5..989fd8401 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -63,6 +63,11 @@ class Service(BASE, CinderBase): disabled = Column(Boolean, default=False) availability_zone = Column(String(255), default='cinder') disabled_reason = Column(String(255)) + # adding column modified_at to contain timestamp + # for manual enable/disable of cinder services + # updated_at column will now contain timestamps for + # periodic updates + modified_at = Column(DateTime) class ConsistencyGroup(BASE, CinderBase): diff --git a/cinder/tests/api/contrib/test_services.py b/cinder/tests/api/contrib/test_services.py index 6504fd6ed..d8aeb1ee2 100644 --- a/cinder/tests/api/contrib/test_services.py +++ b/cinder/tests/api/contrib/test_services.py @@ -37,7 +37,8 @@ fake_services_list = [ 'disabled': True, 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 2), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 27), - 'disabled_reason': 'test1'}, + 'disabled_reason': 'test1', + 'modified_at': ''}, {'binary': 'cinder-volume', 'host': 'host1', 'availability_zone': 'cinder', @@ -45,7 +46,8 @@ fake_services_list = [ 'disabled': True, 'updated_at': datetime.datetime(2012, 10, 29, 13, 42, 5), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 27), - 'disabled_reason': 'test2'}, + 'disabled_reason': 'test2', + 'modified_at': ''}, {'binary': 'cinder-scheduler', 'host': 'host2', 'availability_zone': 'cinder', @@ -53,7 +55,8 @@ fake_services_list = [ 'disabled': False, 'updated_at': datetime.datetime(2012, 9, 19, 6, 55, 34), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28), - 'disabled_reason': ''}, + 'disabled_reason': '', + 'modified_at': ''}, {'binary': 'cinder-volume', 'host': 'host2', 'availability_zone': 'cinder', @@ -61,7 +64,27 @@ fake_services_list = [ 'disabled': True, 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28), - 'disabled_reason': 'test4'}, ] + 'disabled_reason': 'test4', + 'modified_at': ''}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'availability_zone': 'cinder', + 'id': 5, + 'disabled': True, + 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), + 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28), + 'disabled_reason': 'test5', + 'modified_at': datetime.datetime(2012, 10, 29, 13, 42, 5)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'availability_zone': 'cinder', + 'id': 6, + 'disabled': False, + 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), + 'created_at': datetime.datetime(2012, 9, 18, 2, 46, 28), + 'disabled_reason': '', + 'modified_at': datetime.datetime(2012, 9, 18, 8, 1, 38)}, +] class FakeRequest(object): @@ -174,6 +197,18 @@ class ServicesTest(test.TestCase): 'host': 'host2', 'zone': 'cinder', 'status': 'disabled', 'state': 'down', + 'updated_at': datetime.datetime( + 2012, 9, 18, 8, 3, 38)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', 'state': 'down', + 'updated_at': datetime.datetime( + 2012, 10, 29, 13, 42, 5)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', 'state': 'down', 'updated_at': datetime.datetime( 2012, 9, 18, 8, 3, 38)}]} self.assertEqual(res_dict, response) @@ -209,7 +244,21 @@ class ServicesTest(test.TestCase): 'status': 'disabled', 'state': 'down', 'updated_at': datetime.datetime( 2012, 9, 18, 8, 3, 38), - 'disabled_reason': 'test4'}]} + 'disabled_reason': 'test4'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', 'state': 'down', + 'updated_at': datetime.datetime( + 2012, 10, 29, 13, 42, 5), + 'disabled_reason': 'test5'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', 'state': 'down', + 'updated_at': datetime.datetime( + 2012, 9, 18, 8, 3, 38), + 'disabled_reason': ''}]} self.assertEqual(res_dict, response) def test_services_list_with_host(self): @@ -271,6 +320,20 @@ class ServicesTest(test.TestCase): 'zone': 'cinder', 'status': 'disabled', 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 18, + 8, 3, 38)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 10, 29, + 13, 42, 5)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', + 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38)}]} self.assertEqual(res_dict, response) @@ -297,7 +360,23 @@ class ServicesTest(test.TestCase): 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), - 'disabled_reason': 'test4'}]} + 'disabled_reason': 'test4'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 10, 29, + 13, 42, 5), + 'disabled_reason': 'test5'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 18, + 8, 3, 38), + 'disabled_reason': ''}]} self.assertEqual(res_dict, response) def test_services_list_with_binary(self): @@ -317,6 +396,20 @@ class ServicesTest(test.TestCase): 'zone': 'cinder', 'status': 'disabled', 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 18, + 8, 3, 38)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 10, 29, + 13, 42, 5)}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', + 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38)}]} self.assertEqual(res_dict, response) @@ -343,7 +436,23 @@ class ServicesTest(test.TestCase): 'state': 'down', 'updated_at': datetime.datetime(2012, 9, 18, 8, 3, 38), - 'disabled_reason': 'test4'}]} + 'disabled_reason': 'test4'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'disabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 10, 29, + 13, 42, 5), + 'disabled_reason': 'test5'}, + {'binary': 'cinder-volume', + 'host': 'host2', + 'zone': 'cinder', + 'status': 'enabled', + 'state': 'down', + 'updated_at': datetime.datetime(2012, 9, 18, + 8, 3, 38), + 'disabled_reason': ''}]} self.assertEqual(res_dict, response) def test_services_list_with_host_service(self): diff --git a/cinder/tests/test_migrations.py b/cinder/tests/test_migrations.py index 95c1fa8be..7028777a2 100644 --- a/cinder/tests/test_migrations.py +++ b/cinder/tests/test_migrations.py @@ -807,6 +807,16 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): self.assertIsInstance(volumes.c.mountpoint.type, sqlalchemy.types.VARCHAR) + def _check_041(self, engine, data): + """Test that adding modified_at column works correctly.""" + services = db_utils.get_table(engine, 'services') + self.assertIsInstance(services.c.modified_at.type, + self.TIME_TYPE) + + def _post_downgrade_041(self, engine): + services = db_utils.get_table(engine, 'services') + self.assertNotIn('modified_at', services.c) + def test_walk_versions(self): self.walk_versions(True, False) -- 2.45.2