Open Stack

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.

Manual inspection by one person that their environment looks fine has 
never been a sufficient threshold for merging code.

	-Sean

-- 
Sean Dague
http://dague.net