Open Stack

On 2013-11-01 06:28, Daniel P. Berrange wrote:
> On Fri, Nov 01, 2013 at 07:20:19AM -0400, Sean Dague wrote:
>> On 11/01/2013 06:27 AM, John Garbutt wrote:
>> >On 31 October 2013 16:57, Johannes Erdfelt <johannes at erdfelt.com> wrote:
>> >>On Thu, Oct 31, 2013, Sean Dague <sean at dague.net> wrote:
>> >>>So there is a series of patches starting with -
>> >>>https://review.openstack.org/#/c/53417/ that go back and radically
>> >>>change existing migration files.
>> >
>> >I initially agreed with the -2, but actually I like this change, but I
>> >will get to that later.
>> >
>> >>>This is really a no-no, unless there is a critical bug fix that
>> >>>absolutely requires it. Changing past migrations should be
>> >>>considered with the same level of weight as an N-2 backport, only
>> >>>done when there is huge upside to the change.
>> >>>
>> >>>I've -2ed the first 2 patches in the series, though that review
>> >>>applies to all of them (I figured a mailing list thread was probably
>> >>>more useful than -2ing everything in the series).
>> >>>
>> >>>There needs to be really solid discussion about the trade offs here
>> >>>before contemplating something as dangerous as this.
>> >>
>> >>The most important thing for DB migrations is that they remain
>> >>functionality identical.
>> >
>> >+1
>> >
>> >We really should never change what the migrations functionally do.
>> >
>> >Admittedly we should ensure we don't change something "by accident",
>> >so I agree with minimizing the changes in those files also.
>> >
>> >>Historically we have allowed many changes to DB migrations that kept
>> >>them functionally identical to how they were before.
>> >>
>> >>Looking through the commit history, here's a sampling of changes:
>> >>
>> >>- _ was no longer monkey patched, necessitating a new import added
>> >>- fix bugs causing testing problems
>> >>- change copyright headers
>> >>- remove unused code (creating logger, imports, etc)
>> >>- fix bugs causing the migrations to fail to function (on PostgreSQL,
>> >>   downgrade bugs, etc)
>> >>- style changes (removing use of locals(), whitespace, etc)
>> >>- make migrations faster
>> >>- add comments to clarify code
>> >>- improve compatibility with newer versions of SQLAlchemy
>> >>
>> >>The reviews you're referencing seem to fall into what we have
>> >>historically allowed.
>> >
>> >+1 The patch is really just refactoring.
>> >
>> >I think we should move to the more descriptive field names, so we
>> >remove the risk of cut and paste errors in string length, etc.
>> >
>> >Now, if we don't go back and add those into the migrations, people
>> >will just cut and paste examples from the old migrations, and
>> >everything will start getting quite confusing. I would love to say
>> >that wasn't true, be we know that's how it goes.
>> 
>> It's trading one source of bugs for another. I'd love to say we can
>> have our cake and eat it to, but we really can't. And I very much
>> fall on the side of "getting migrations is hard, updating past
>> migrations without ever forking the universe is really really hard,
>> and we've completely screwed it up in the past, so lets not do it."
>> 
>> >>That said, I do agree there needs to be a higher burden of proof that
>> >>the change being made is functionally identical to before.
>> >
>> >+1 and Rick said he has inspected the MySQL and PostgreSQL tables to
>> >ensure he didn't change anything.
>> 
>> So I'm going to call a straight BS on that. In at least one of the
>> cases columns were shortened from 256 to 255. In the average case
>> would that be an issue? Probably not. However that's a truncation,
>> and a completely working system at 256 length for those fields could
>> go to non working with data truncation. Data loads matter. And we
>> can't assume anything about the data in those fields that isn't
>> enforced by the DB schema itself.
>> 
>> I've watched us mess this up multiple times in the past when we were
>> *sure* it was good. And as has been noticed recently, one of the
>> collapses changes a fk name (by accident), which broke upgrades to
>> havana for a whole class of people.
>> 
>> So I think that we really should put a moratorium on touching past
>> migrations until there is some sort of automatic validation that the
>> new and old path are the same, with sufficiently complicated data
>> that pushes the limits of those fields.
> 
> Agreed, automated validation should be a mandatory pre-requisite
> for this kind of change. I've done enough "mechanical no-op
> refactoring" changes in the past to know that humans always screw
> something up - we're just not good at identifying the needle in a
> haystack. For data model upgrade changes this risk is too serious to
> ignore.

FWIW, there's work going on in Oslo around validating that our 
migrations result in a schema that matches the intended model.  IIUC, 
that should help catch a lot of errors in changes for both old and new 
migrations.

-Ben