Running of unit tests fails if SQLAlchemy >= 0.8.3 is used

[Roman Podoliaka]

If SQLAlchemy >= 0.8.3 is used, running of unit tests fails. The following error is printed to stderr multiple times:

DBError: (IntegrityError) constraint failed u'UPDATE instance_system_metadata SET updated_at=?, deleted_at=?, deleted=? WHERE instance_system_metadata.id = ?' ('2013- 11-19 10:37:44.378444', '2013-11-19 10:37:44.377819', 11, 11)

A few of our migrations change the type of deleted column from boolean to int. MySQL and SQLite don't have native boolean data type. SQLAlchemy uses int columns (e.g. in case of MySQL - tinyint) + CHECK constraint (something like CHECK (deleted in (0, 1))) to emulate boolean data type.

In our migrations when the type of column `deleted` is changed from boolean to int, the corresponding CHECK constraint is dropped too. But starting from SQLAlchemy version 0.8.3, those CHECK constraints aren't dropped anymore. So despite the fact that column deleted is of type int now, we still restrict its values to be either 0 or 1.

Migrations changing the data type of deleted columns rely on SQL rendered for CHECK constraints (e.g. https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/migrate_repo/versions/152_change_type_of_deleted_column.py#L172). There was a patch in SQLAlchemy 0.8.3 release that slightly changed the way DDL statements are rendered ((http://docs.sqlalchemy.org/en/latest/changelog/changelog_08.html#change-487183f04e6da9aa27d8817bca9906d1)). Unfortunately, due to the fact that nova migrations depend on such implementation details, this change to SQLAlchemy broke our code.

We must fix our migrations to work properly with new SQLAlchemy versions too (0.8.3+).

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/57493

[Viktor Serhieiev (vsergeyev)]

We have a similar code in Oslo-incubator, so we should fix it there as well

[Doug Hellmann (doug-hellmann)]

This sounds like a bug in sqlalchemy. Has it been reported upstream?

[Roman Podoliaka (rpodolyaka)]

Doug, actually it's not.

This bug is caused by limitations of SQLite and sqlalchemy-migrate library: SQLite doesn't have a native implementation of BOOLEAN data type, so SQLAlchemy emulates it by creating an INTEGER column and setting a CHECK constraint which ensures that values can be either 1 or 0 only. And SQLite doesn't have a 'normal' ALTER, so when we ask sqlalchemy-migrate to drop a column, it actually recreates the table without that column. Though, it knows nothing about constraints which must be dropped.

We've been carrying a work around for this which relies on the way CHECK constraints are rendered in SQLAlchemy. The new version of SQLAlchemy slightly changed the way constraints are rendered and broke our work around. But we can't blame SQLAlchemy here, because we've been relying on the implementation detail rather than on public API (as there is no public API for this, because no one cares about migrations in SQLite...).

So we can improve our existing work around here to make it work with new versions of SQLAlchemy. But I hope we'll drop all this stuff as soon as we migrate to using of alembic and stop running migration scripts on SQLite.

[Doug Hellmann (doug-hellmann)]

The change in behavior for SQLAlchemy (not dropping the CHECK constraint) still sounds like a regression, but if the work-around was already in our code base then I agree we need to fix it here.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/57493
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=c1be8381e19390b0edca28c8ab3f77e6226f25e0
Submitter: Jenkins
Branch: master

commit c1be8381e19390b0edca28c8ab3f77e6226f25e0
Author: Ilya Pekelny <email address hidden>
Date: Wed Nov 20 18:51:39 2013 +0200

    Fix migrations changing the type of deleted column

    SQLite doesn't provide a native implementation of BOOLEAN data type.
    SQLAlchemy emulates BOOLEAN data type for SQLite using INT column +
    CHECK constraint.
    We have a few migrations changing the type of column 'deleted' from
    BOOLEAN to INT. Due to limitations of ALTER in SQLite, in order to do
    that, we omit the original 'deleted' column as well as the corresponding
    CHECK constraint when recreating the table. Omitting of the constraint
    is more tricky. It's implemented by analyzing the SQL text used for its
    rendering. SQLAlchemy versions 0.8.3+ slightly changed the way
    constraints are rendered, so we have to update our migrations changing
    the type of 'deleted' column to work with SQLAlchemy 0.8.3+ on SQLite
    backend (we aren't using SQLite in production, but we still need it for
    running of unit tests).

    Closes-Bug: #1252693
    Change-Id: I52f2b5f90a2f9191767fadc7b1eacae236c30e98

[Doug Hellmann (doug-hellmann)]

Is there still work to do here for Oslo?

[Ilya Pekelny (i159)]

Thanks! I forgot about the task! I'm sorry...

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/72419

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/93414

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/93414
Committed: https://git.openstack.org/cgit/openstack/oslo.db/commit/?id=528e406053ac9dfc5ee1348cf1ee64220ad0c1ce
Submitter: Jenkins
Branch: master

commit 528e406053ac9dfc5ee1348cf1ee64220ad0c1ce
Author: Ilya Pekelny <email address hidden>
Date: Tue May 13 14:16:44 2014 +0300

    Fix changing the type of column deleted

    SQLite doesn't provide a native implementation of BOOLEAN data type.
    SQLAlchemy emulates BOOLEAN data type for SQLite using INT column +
    a CHECK constraint.

    We've been providing a helper util to change the type of column
    'deleted' from BOOLEAN to INT. Due to limitations of ALTER in SQLite,
    in order to do that, we omit the original 'deleted' column as well as
    the corresponding CHECK constraint when recreating the table.
    Omitting of the constraint is more tricky, though. It's implemented
    by analyzing the SQL text used for its rendering. SQLAlchemy
    versions 0.8.3+ slightly changed the way constraints are rendered, so
    we have to update our util changing the type of 'deleted' column to
    work with SQLAlchemy 0.8.3+ on SQLite backend (we aren't using SQLite
    in production, but we still need it for running of unit tests).

    Closes-Bug: #1252693

    Change-Id: Ie92caff005217250de17461f4f87692ad8e96a09