Fix for inconsistent cinder-services state change

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

diff --git a/cinder/api/contrib/services.py b/cinder/api/contrib/services.py
index 915ce1f4e9c32afa6ac6b66bcd95f7195377e201..ae716bb7f8a196668330d6bf43ca3ad52352bc70 100644 (file)
--- 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 2adf456281a45ac7955a26ffe24ff4a655e2eaef..fd4fb02eb7862b3ffba57cdeb75ce19b92c82110 100644 (file)
--- 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 (file)
index 0000000..be2d33e
--- /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 77f7105c538b41f1265561e7420199d8cf52505c..989fd8401b5616f56d034c03ac1ca4988610b46b 100644 (file)
--- 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 6504fd6ed430e1038e53348b08686b2a668ff5ab..d8aeb1ee2b0ec66b87977288bda5da04597169ca 100644 (file)
--- 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 95c1fa8bec183190e8818a62364270f70fe28c6f..7028777a29a5f44375d5c5701227c60837afde55 100644 (file)
--- 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)