Re: [openstack-dev] [nova] changing old migrations is verboten
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. 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. Cheers, John ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
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. 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 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
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
Re: [openstack-dev] [nova] changing old migrations is verboten
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 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. 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 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
On Fri, Nov 01, 2013, Sean Dague s...@dague.net wrote: 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. I understand what you're saying, but if the result of it is that we're never going to touch old migrations, we're going to slowly build technical debt. I don't think it's an acceptable solution to throw up our hands and deal with the pain. We need to come up with a solution that allows us to stay agile while also ensuring we don't break things. 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 assume this is the review you're talking about? https://review.openstack.org/#/c/53471/3 FWIW, the old migrations *are* functionally identical. Those strings are still 256 characters long. It's the new migration that truncates data. That said, I'm not sure I see the value in this particular cleanup considering the fact it does truncate data (even if it's unlikely to cause problems). 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. I can get completely on board with that. Does that mean you're softening your stance that migrations should never be touched? JE ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
On 11/01/2013 12:20 PM, Johannes Erdfelt wrote: snip 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. I can get completely on board with that. Does that mean you're softening your stance that migrations should never be touched? If we have a way to automatically validate the new final results vs. the old final results, including carrying interesting edge condition data through it, yes, absolutely. Many things move from verboten to acceptable with sufficient test harness. -Sean -- Sean Dague http://dague.net signature.asc Description: OpenPGP digital signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [nova] changing old migrations is verboten
So there is a series of patches starting with - https://review.openstack.org/#/c/53417/ that go back and radically change existing migration files. 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. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
On 10/31/2013 08:01 AM, Sean Dague 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. 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. +1 -jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
On 10/31/2013 11:23 AM, Jay Pipes wrote: On 10/31/2013 08:01 AM, Sean Dague 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. 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. +1 There is a very real reason why we have a firm stance on this. There are a huge number of OpenStack instances out in the field, at all sorts of different past versions. We really try to promise that you can always forward upgrade your database. If you go back and change an old migration, you have not forked the past. Some people will have already taken that migration, and they have one view of the world, others haven't yet, they hit your updated version, and they now have different database. So 2 people with Havana would no longer be guaranteed to have the same data model set up by us. It's easy to believe that this change is really straight forward, it will be exactly the same model, but if it isn't, in any way, exactly the same (even in a way that we didn't realize yet that it mattered), you've forked the past. And that makes supporting users in these various forked versions of the world impossible. Migrations are basically idempotent. If you want to clean things up, do them in a new migration. Don't touch an old one unless it is causing corruption to someone's data so that fixing it with a future migration is not an option. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
On 10/31/2013 11:56 AM, Sean Dague wrote: On 10/31/2013 11:23 AM, Jay Pipes wrote: On 10/31/2013 08:01 AM, Sean Dague 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. 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. +1 There is a very real reason why we have a firm stance on this. There are a huge number of OpenStack instances out in the field, at all sorts of different past versions. We really try to promise that you can always forward upgrade your database. If you go back and change an old migration, you have not forked the past. Some people will have already taken that migration, and they have one view of the world, others haven't yet, they hit your updated version, and they now have different database. So 2 people with Havana would no longer be guaranteed to have the same data model set up by us. It's easy to believe that this change is really straight forward, it will be exactly the same model, but if it isn't, in any way, exactly the same (even in a way that we didn't realize yet that it mattered), you've forked the past. And that makes supporting users in these various forked versions of the world impossible. Migrations are basically idempotent. If you want to clean things up, do them in a new migration. Don't touch an old one unless it is causing corruption to someone's data so that fixing it with a future migration is not an option. LOL, I was +1'ing your thoughts, not +1'ing the proposal to have a solid discussion about the trade-offs :) Sorry for the confusion! -jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
Actually no confusion. :-) Joe Gordon just made me realize that I didn't really explain why we had that policy. That really should have been a follow up to my own post, not yours. Sorry if I made it look like I was arguing with you, which I wasn't.. :-) We're all good. Sean Dague http://dague.net On Oct 31, 2013 12:12 PM, Jay Pipes jaypi...@gmail.com wrote: On 10/31/2013 11:56 AM, Sean Dague wrote: On 10/31/2013 11:23 AM, Jay Pipes wrote: On 10/31/2013 08:01 AM, Sean Dague wrote: So there is a series of patches starting with - https://review.openstack.org/#**/c/53417/https://review.openstack.org/#/c/53417/that go back and radically change existing migration files. 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. +1 There is a very real reason why we have a firm stance on this. There are a huge number of OpenStack instances out in the field, at all sorts of different past versions. We really try to promise that you can always forward upgrade your database. If you go back and change an old migration, you have not forked the past. Some people will have already taken that migration, and they have one view of the world, others haven't yet, they hit your updated version, and they now have different database. So 2 people with Havana would no longer be guaranteed to have the same data model set up by us. It's easy to believe that this change is really straight forward, it will be exactly the same model, but if it isn't, in any way, exactly the same (even in a way that we didn't realize yet that it mattered), you've forked the past. And that makes supporting users in these various forked versions of the world impossible. Migrations are basically idempotent. If you want to clean things up, do them in a new migration. Don't touch an old one unless it is causing corruption to someone's data so that fixing it with a future migration is not an option. LOL, I was +1'ing your thoughts, not +1'ing the proposal to have a solid discussion about the trade-offs :) Sorry for the confusion! -jay __**_ OpenStack-dev mailing list OpenStack-dev@lists.openstack.**org OpenStack-dev@lists.openstack.org http://lists.openstack.org/**cgi-bin/mailman/listinfo/**openstack-devhttp://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] changing old migrations is verboten
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. 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. 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. That said, I do agree there needs to be a higher burden of proof that the change being made is functionally identical to before. JE ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
