Block subtractive operations in migrations for Kilo and beyond
In Kilo, we made a point to not allow any subtractive DB migrations. That gave us a lot of flexibility for upgrades from Juno. Reviewers did the awesome job of keeping to that plan. However, we need a mechanical filter to prevent regressions. This adds that in the form of a banned-ops fixture in test_migrations. It excludes migrations from pre-Kilo times, as well as excludes a few migrations that are already in place and do reaspnable things. In general, we need to be extremely careful about letting migrations put themselves on the exclusion list, so I added scary words around it. Change-Id: Iefe678d0aeb1ad898fc5a0cee10fd55ccb7e06db
This commit is contained in:
parent
1294422be6
commit
43ccfc84f5
|
@ -29,6 +29,7 @@ import six
|
|||
|
||||
from nova.db import migration
|
||||
from nova.db.sqlalchemy import api as session
|
||||
from nova import exception
|
||||
from nova.objects import base as obj_base
|
||||
from nova import rpc
|
||||
from nova import service
|
||||
|
@ -412,3 +413,26 @@ class SpawnIsSynchronousFixture(fixtures.Fixture):
|
|||
super(SpawnIsSynchronousFixture, self).setUp()
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'nova.utils.spawn_n', lambda f, *a, **k: f(*a, **k)))
|
||||
|
||||
|
||||
class BannedDBSchemaOperations(fixtures.Fixture):
|
||||
"""Ban some operations for migrations"""
|
||||
def __init__(self, banned_resources=None):
|
||||
super(BannedDBSchemaOperations, self).__init__()
|
||||
self._banned_resources = banned_resources or []
|
||||
|
||||
@staticmethod
|
||||
def _explode(resource, op):
|
||||
raise exception.DBNotAllowed(
|
||||
'Operation %s.%s() is not allowed in a database migration' % (
|
||||
resource, op))
|
||||
|
||||
def setUp(self):
|
||||
super(BannedDBSchemaOperations, self).setUp()
|
||||
for thing in self._banned_resources:
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'sqlalchemy.%s.drop' % thing,
|
||||
lambda *a, **k: self._explode(thing, 'drop')))
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'sqlalchemy.%s.alter' % thing,
|
||||
lambda *a, **k: self._explode(thing, 'alter')))
|
||||
|
|
|
@ -189,7 +189,40 @@ class NovaMigrationsCheckers(test_migrations.ModelsMigrationsSync,
|
|||
('DB Migration %i does not have a '
|
||||
'test. Please add one!') % version)
|
||||
|
||||
super(NovaMigrationsCheckers, self).migrate_up(version, with_data)
|
||||
# NOTE(danms): This is a list of migrations where we allow dropping
|
||||
# things. The rules for adding things here are very very specific.
|
||||
# Chances are you don't meet the critera.
|
||||
# Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE
|
||||
exceptions = [
|
||||
# 267 enforces non-nullable instance.uuid. This was mostly
|
||||
# a special case because instance.uuid shouldn't be able
|
||||
# to be nullable
|
||||
267,
|
||||
|
||||
# 278 removes a FK restriction, so it's an alter operation
|
||||
# that doesn't break existing users
|
||||
278,
|
||||
|
||||
# 280 enforces non-null keypair name. This is really not
|
||||
# something we should allow, but it's in the past
|
||||
280,
|
||||
|
||||
# 292 drops completely orphaned tables with no users, so
|
||||
# it can be done without affecting anything.
|
||||
292,
|
||||
]
|
||||
# Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE
|
||||
|
||||
# NOTE(danms): We only started requiring things be additive in
|
||||
# kilo, so ignore all migrations before that point.
|
||||
KILO_START = 265
|
||||
|
||||
if version >= KILO_START and version not in exceptions:
|
||||
banned = ['Table', 'Column']
|
||||
else:
|
||||
banned = None
|
||||
with nova_fixtures.BannedDBSchemaOperations(banned):
|
||||
super(NovaMigrationsCheckers, self).migrate_up(version, with_data)
|
||||
|
||||
def test_walk_versions(self):
|
||||
self.walk_versions(snake_walk=False, downgrade=False)
|
||||
|
|
|
@ -14,6 +14,7 @@
|
|||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
import sqlalchemy
|
||||
import sys
|
||||
|
||||
import fixtures as fx
|
||||
|
@ -24,6 +25,7 @@ from oslo_utils import uuidutils
|
|||
import testtools
|
||||
|
||||
from nova.db.sqlalchemy import api as session
|
||||
from nova import exception
|
||||
from nova.objects import base as obj_base
|
||||
from nova.tests import fixtures
|
||||
from nova.tests.unit import conf_fixture
|
||||
|
@ -309,3 +311,21 @@ class TestSpawnIsSynchronousFixture(testtools.TestCase):
|
|||
tester = mock.MagicMock()
|
||||
utils.spawn_n(tester.function, 'foo', bar='bar')
|
||||
tester.function.assert_called_once_with('foo', bar='bar')
|
||||
|
||||
|
||||
class TestBannedDBSchemaOperations(testtools.TestCase):
|
||||
def test_column(self):
|
||||
column = sqlalchemy.Column()
|
||||
with fixtures.BannedDBSchemaOperations(['Column']):
|
||||
self.assertRaises(exception.DBNotAllowed,
|
||||
column.drop)
|
||||
self.assertRaises(exception.DBNotAllowed,
|
||||
column.alter)
|
||||
|
||||
def test_table(self):
|
||||
table = sqlalchemy.Table()
|
||||
with fixtures.BannedDBSchemaOperations(['Table']):
|
||||
self.assertRaises(exception.DBNotAllowed,
|
||||
table.drop)
|
||||
self.assertRaises(exception.DBNotAllowed,
|
||||
table.alter)
|
||||
|
|
Loading…
Reference in New Issue