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
This commit is contained in:
Erik Olof Gunnar Andersson 2018-05-14 13:50:03 -07:00
parent cb4e34d348
commit 1924abff40
7 changed files with 136 additions and 1 deletions

View File

@ -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)

View File

@ -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)

View File

@ -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 = [{

View File

@ -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)

View File

@ -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()

View File

@ -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()

View File

@ -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