Add unique constraint to service_statuses

In the current implementation, if for any reason a duplicate
service entry gets created, the call to update that service
will fail endlessly, and instead cause the service to create
new entries everytime update_service_status gets called. Causing
it to fill the database with duplicate entries.

This patch adds a unique constraint to the service_statuses
table based on the service_name and hostname, to ensure that
this cannot happen.

In addition we add a new test to the storage driver and further
expanded the central service test coverage.

Change-Id: I307a8f7dd8b8a83effa447a846db3288efa32dba
Closes-Bug: #1768824

designate/central/service.py

@@ -2894,5 +2894,13 @@ class Service(service.RPCService, service.Service):
 
             return self.storage.update_service_status(context, db_status)
         except exceptions.ServiceStatusNotFound:
+            LOG.info(
+                "Creating new service status entry for %(service_name)s "
+                "at %(hostname)s",
+                {
+                    'service_name': service_status.service_name,
+                    'hostname': service_status.hostname
+                }
+            )
             return self.storage.create_service_status(
                 context, service_status)

designate/storage/impl_sqlalchemy/migrate_repo/versions/100_unique_service_status.py

@@ -0,0 +1,47 @@
+# 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 Unique constraint on ('service_name', 'hostname') in the
+service_statuses table for bug #1768824"""
+
+import sys
+
+from migrate.changeset.constraint import UniqueConstraint
+from oslo_log import log as logging
+from sqlalchemy import exc
+from sqlalchemy.schema import MetaData
+from sqlalchemy.schema import Table
+
+LOG = logging.getLogger()
+EXPLANATION = """
+You need to manually remove duplicate entries from the database.
+
+The error message was:
+%s
+"""
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+    service_statuses_table = Table('service_statuses', meta, autoload=True)
+
+    # Add UniqueConstraint based on service_name and hostname.
+    constraint = UniqueConstraint('service_name', 'hostname',
+                                  table=service_statuses_table,
+                                  name="unique_service_status")
+    try:
+        constraint.create()
+    except exc.IntegrityError as e:
+        LOG.error(EXPLANATION, e)
+        # Use sys.exit so we don't blow up with a huge trace
+        sys.exit(1)

designate/tests/__init__.py

@@ -76,6 +76,13 @@ class TestCase(base.BaseTestCase):
         'status': "UP",
         'stats': {},
         'capabilities': {},
+    }, {
+        'id': 'c326f735-eecc-4968-969f-355a43c4ae27',
+        'service_name': 'baz',
+        'hostname': 'qux',
+        'status': "UP",
+        'stats': {},
+        'capabilities': {},
     }]
 
     quota_fixtures = [{

designate/tests/test_central/test_service.py

@@ -32,6 +32,7 @@ from oslo_messaging.notify import notifier
 from designate import exceptions
 from designate import objects
 from designate.mdns import rpcapi as mdns_api
+from designate.tests import fixtures
 from designate.tests.test_central import CentralTestCase
 from designate.storage.impl_sqlalchemy import tables
 
@@ -39,6 +40,11 @@ LOG = logging.getLogger(__name__)
 
 
 class CentralServiceTest(CentralTestCase):
+    def setUp(self):
+        super(CentralServiceTest, self).setUp()
+        self.stdlog = fixtures.StandardLogging()
+        self.useFixture(self.stdlog)
+
     def test_stop(self):
         # Test stopping the service
         self.central_service.stop()
@@ -3119,6 +3125,48 @@ class CentralServiceTest(CentralTestCase):
 
         self.assertEqual(zone_serial, new_zone_serial)
 
+    def test_create_new_service_status_entry(self):
+        values = self.get_service_status_fixture()
+
+        service_status = self.central_service.update_service_status(
+            self.admin_context, objects.ServiceStatus.from_dict(values))
+
+        self.assertIn("Creating new service status entry for foo at bar",
+                      self.stdlog.logger.output)
+
+        # Make sure this was never updated.
+        self.assertIsNone(service_status.updated_at)
+
+    def test_update_existing_service_status_entry(self):
+        values = self.get_service_status_fixture()
+
+        new_service_status = objects.ServiceStatus.from_dict(values)
+        self.storage.create_service_status(
+            self.admin_context, new_service_status)
+
+        service_status = self.central_service.update_service_status(
+            self.admin_context, objects.ServiceStatus.from_dict(values))
+
+        self.assertEqual(new_service_status.id, service_status.id)
+
+        # Make sure the entry was updated.
+        self.assertIsNotNone(service_status.updated_at)
+
+    def test_update_existing_service_status_entry_with_id_provided(self):
+        values = self.get_service_status_fixture(fixture=1)
+
+        self.storage.create_service_status(
+            self.admin_context, objects.ServiceStatus.from_dict(values))
+
+        service_status = self.central_service.update_service_status(
+            self.admin_context, objects.ServiceStatus.from_dict(values))
+
+        self.assertEqual('c326f735-eecc-4968-969f-355a43c4ae27',
+                         service_status.id)
+
+        # Make sure the entry was updated.
+        self.assertIsNotNone(service_status.updated_at)
+
     def test_create_zone_transfer_request(self):
         zone = self.create_zone()
         zone_transfer_request = self.create_zone_transfer_request(zone)

designate/tests/test_storage/__init__.py

@@ -2882,6 +2882,16 @@ class StorageTestCase(object):
             uuid = '97f57960-f41b-4e93-8e22-8fd6c7e2c183'
             self.storage.delete_pool_also_notify(self.admin_context, uuid)
 
+    def test_create_service_status_duplicate(self):
+        values = self.get_service_status_fixture(fixture=0)
+
+        self.storage.create_service_status(
+            self.admin_context, objects.ServiceStatus.from_dict(values))
+
+        with testtools.ExpectedException(exceptions.DuplicateServiceStatus):
+            self.storage.create_service_status(
+                self.admin_context, objects.ServiceStatus.from_dict(values))
+
     # Zone Transfer Accept tests
     def test_create_zone_transfer_request(self):
         zone = self.create_zone()

designate/tests/unit/test_service_status.py

@@ -83,7 +83,7 @@ class RpcEmitterTest(oslotest.base.BaseTestCase):
 
         status_factory = mock.Mock(return_value=(status, stats, capabilities,))
         emitter = service_status.RpcEmitter("svc", self.mock_tg,
-                                           status_factory=status_factory)
+                                            status_factory=status_factory)
         emitter.start()
 
         central = mock.Mock()

releasenotes/notes/bug-1768824-service_statuses-constraint-7a30eb78dc63b86f.yaml

@@ -0,0 +1,15 @@
+---
+upgrade:
+  - |
+    If there are duplicate service entries in the service_statuses table
+    and the db sync command fails, you may need to truncate the
+    service_statuses table.
+fixes:
+  - |
+    Fixed `bug 1768824`_ which could cause the service_statuses table
+    to be flooded with duplicate service entries.
+
+    We fixed this by introducing a new unique constraint to the
+    service_statuses table.
+
+    .. _Bug 1768824: https://bugs.launchpad.net/designate/+bug/1768824