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 <johan...@erdfelt.com> wrote: > >>On Thu, Oct 31, 2013, Sean Dague <s...@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 :| _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev