Open Stack

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.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|