| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-06 | |||
| 16:21:58 | dansmith | mnaser: okay it shouldn't be doing that | |
| 16:21:59 | mnaser | so its almost like once it does the schema update, it bumps the version, and then tries to backfill default values | |
| 16:22:08 | mnaser | let me check the value of hidden in the other train deployemnt | |
| 16:22:11 | dansmith | yeah, that's not the intent of that migration for sure | |
| 16:22:13 | openstackgerrit | Merged openstack/nova-specs master: FUP: Fixed the invalid index in References https://review.opendev.org/705933 | |
| 16:23:03 | dansmith | maybe in some situations sqla-migrate actually tries to patch up data for you on existing tables, which is a real problem | |
| 16:23:18 | mnaser | ok yeah | |
| 16:23:26 | mnaser | the other deployment has hidden=0 for every single record | |
| 16:23:30 | dansmith | and if so, this is a good example of why we don't do data transformation (intentionally) in schema migrations | |
| 16:23:31 | dansmith | okay damn | |
| 16:23:57 | dansmith | so we need to change that migration retroactively, and patch the load code to properly interpret the null as the default (which sqla does not do with default=) | |
| 16:24:10 | mnaser | so i think what happened is: schema updated and column added, db version bumped, backfill values to 0 | |
| 16:24:22 | mnaser | and then i failed in the backfill, so the next run it just started from the next migration | |
| 16:24:25 | dansmith | yeah, and on your big cloud, that timed out | |
| 16:24:39 | dansmith | yeah, probably because the backfill wasn't technically a schema fail | |
| 16:24:41 | dansmith | which is also scary | |
| 16:25:18 | mnaser | i think your theory is best | |
| 16:25:23 | mnaser | s/theory/solution/ | |
| 16:25:23 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: Add MigrationTypeField https://review.opendev.org/706013 | |
| 16:25:24 | dansmith | mnaser: can you file a bug for me to write this up against? | |
| 16:25:39 | mnaser | sure | |
| 16:25:47 | dansmith | thanks | |
| 16:26:57 | mriedem | fwiw i count 5 non-api db schema migrations that add a new boolean column with default=False, | |
| 16:27:05 | dansmith | mriedem: yup | |
| 16:27:12 | mriedem | if it's actually backfilling, then maybe just not an issue for those b/c they are smaller tables | |
| 16:27:13 | dansmith | mriedem: we've been doing this without realizing | |
| 16:27:17 | dansmith | yu[ | |
| 16:27:22 | dansmith | will need a poison test too | |
| 16:27:29 | dansmith | like the others I have for alter operations | |
| 16:29:23 | mriedem | i sort of remember this discussion with jaypipes on https://review.opendev.org/#/c/557958/ | |
| 16:29:26 | mriedem | which uses server_default | |
| 16:30:09 | dansmith | I guess server_default would be one option, but it involves another schema change which I think is less good since we have to backport this | |
| 16:34:30 | mriedem | https://docs.sqlalchemy.org/en/13/core/metadata.html#sqlalchemy.schema.Column.params.default | |
| 16:34:41 | mriedem | "which will be invoked upon insert if this column is otherwise not specified in the VALUES clause of the insert" | |
| 16:34:45 | mriedem | didn't realize it would backfill though | |
| 16:35:03 | dansmith | right, I always assume it fills on read too, but just insert | |
| 16:35:07 | mriedem | https://docs.sqlalchemy.org/en/13/core/metadata.html#sqlalchemy.schema.Column.params.server_default - "A FetchedValue instance, str, Unicode or text() construct representing the DDL DEFAULT value for the column." | |
| 16:35:23 | mriedem | so yeah, i guess never use default in migrations | |
| 16:35:31 | dansmith | right | |
| 16:35:39 | mriedem | but is default in the models OK? | |
| 16:35:40 | mriedem | i thought it was | |
| 16:36:00 | dansmith | it is,m | |
| 16:36:10 | mriedem | mnaser: just purge your db :) | |
| 16:36:13 | dansmith | and I just tested that the model sync test does not fail you for having default on the column and not the migration | |
| 16:36:31 | mnaser | mriedem: you're not wrong | |
| 16:36:36 | mnaser | dansmith: https://bugs.launchpad.net/nova/+bug/1862205 | |
| 16:36:36 | openstack | Launchpad bug 1862205 in OpenStack Compute (nova) "Instances not visible when hidden=NULL" [Undecided,New] | |
| 16:37:08 | dansmith | mnaser: thanks | |
| 16:37:30 | mnaser | dansmith: if we come up/discuss a solution, i'm happy to work out a fix, i have bigger clouds to upgrade ahead of this sooo | |
| 16:38:01 | dansmith | mnaser: I've got a patch started, but yeah, will appreciate real testing for sure | |
| 16:39:34 | mnaser | dansmith: ok cool, i can try and help reviewing to my ability and id be able to provide some feedback on a cloud with 1.5 times the # of records (but only in 2 weeks :X) | |
| 16:39:48 | mriedem | i've still got core if you need to poke someone later | |
| 16:39:50 | dansmith | cool | |
| 16:41:55 | melwitt | I can help with review too if needed | |
| 16:52:40 | gibi | dansmith, efried, Sundar: I agree with the discussed direction in https://review.opendev.org/#/c/631243/55 See details in my review | |
| 16:54:33 | gibi | efried: is there other patches in the series that need my eyes? I can spend some extra time on the series tomorrow if needed | |
| 17:12:20 | openstackgerrit | Merged openstack/nova master: Don't error out on floating IPs without associated ports https://review.opendev.org/706213 | |
| 17:12:26 | openstackgerrit | Merged openstack/nova master: nova-net: Update API reference guide https://review.opendev.org/703796 | |
| 17:12:34 | openstackgerrit | Merged openstack/nova stable/queens: Add functional recreate test for bug 1852610 https://review.opendev.org/699705 | |
| 17:12:34 | openstack | bug 1852610 in OpenStack Compute (nova) queens "API allows source compute service/node deletion while instances are pending a resize confirm/revert" [Low,In progress] https://launchpad.net/bugs/1852610 - Assigned to Matt Riedemann (mriedem) | |
| 17:16:11 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Remove native LUKS compat code https://review.opendev.org/669121 | |
| 17:25:38 | efried | gibi: Well, I've been hoping that I would get the chance to review the series from top to bottom and be the second +2 (with dansmith) but I'm no longer sure I'm going to be able to get to it soon. So if you have time, your help would be much appreciated. But I would say you shouldn't push other stuff out of the way for it. | |
| 17:30:47 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix instance.hidden migration and querying https://review.opendev.org/706331 | |
| 17:31:16 | dansmith | mnaser: mriedem melwitt ^ haven't run all tests yet, so it might break other things, but that's what I'm thinking as a backportable change | |
| 17:31:34 | melwitt | in da club | |
| 17:34:04 | dansmith | gross. | |
| 17:34:55 | melwitt | lol wtf. never heard that before | |
| 17:35:40 | sean-k-mooney | in the club to mean pregnant ya ist an old old way of saying that in polite company when gossiping | |
| 17:36:23 | melwitt | interesting. here it means like at the dance club, in a bunch of rap songs | |
| 17:36:31 | dansmith | as someone who feels like society favors those who procreate unfairly, I definitely see it as "a club" in the negative sense | |
| 17:38:20 | sean-k-mooney | yes as i said it was used when gossiping about someone behind there back so it had a negitive over tone in many cases | |
| 17:38:52 | dansmith | that's not really what I meant :) | |
| 17:38:59 | melwitt | lol | |
| 17:39:00 | mriedem | i'm in da costco club | |
| 17:39:09 | dansmith | mnaser: how many instances in the database that failed, and how many in the one that didn't? | |
| 17:39:13 | mriedem | if you need a years supply of toilet paper i'm your guy | |
| 17:39:23 | sean-k-mooney | lol | |
| 17:39:24 | melwitt | me too, Costco Executive Club | |
| 17:47:03 | melwitt | maybe this is too out there but I wonder if this might be related to the cell timeout gate bug (affecting only grenade jobs) | |
| 17:48:04 | dansmith | the first query after upgrade works, so ... I don't think so | |
| 17:48:34 | dansmith | and also, mnaser said it worked fine for his smaller cluster, which I'm sure is waaaay larger than a grenade setup with a tempest-run worth of instances in the db | |
| 17:48:38 | melwitt | ok. I wondered if there could be something to do with the table rewriting in the background or something | |
| 17:49:15 | dansmith | surely hope not :) but it seems like it was synchronous in mnaser's case | |
| 17:49:25 | melwitt | because when I dug into that, it seemed like mysql just overloaded somehow and logging errors "reading communication packets" | |
| 17:49:39 | melwitt | ok | |
| 17:59:58 | sean-k-mooney | any table updates or data migration should be taking a full table write lock. reads should see the old data until the transaction completes | |
| 18:00:14 | sean-k-mooney | at least if you are using the innodb schema for mysql | |
| 18:14:56 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix instance.hidden migration and querying https://review.opendev.org/706331 | |
| 18:28:51 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix instance.hidden migration and querying https://review.opendev.org/706331 | |
| 18:34:09 | openstackgerrit | Dan Smith proposed openstack/nova master: Disallow specifying a default= on new Columns in schema changes https://review.opendev.org/706351 | |
| 18:34:28 | dansmith | ^ makes it impossible to use Column(default=) in migrations after 399 | |
| 18:35:08 | dansmith | efried: did you catch the earlier discussion about the relatively serious upgrade-corrupts-database bug in train? | |
| 18:35:23 | efried | no | |
| 18:35:54 | dansmith | efried: on largeish deployments, upgrading to Train can make all your instances hidden silently | |
| 18:36:24 | dansmith | we're going to want to backport my fix and release a train update with definitive action at some point here soon | |
| 18:36:54 | sean-k-mooney | dansmith: do we have that fix downstream by the way | |
| 18:37:20 | dansmith | sean-k-mooney: we don't have it upstream yet, so..no. | |
| 18:37:55 | sean-k-mooney | ok cause you know what day it is :) | |
| 18:38:11 | efried | dansmith: do I need to review things? | |
| 18:38:43 | dansmith | efried: you're welcome to.. I think we're looking for a test run, a report from mnaser and melwitt/mriedem are also going to review | |