| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-06 | |||
| 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 | |
| 18:38:50 | dansmith | efried: https://review.opendev.org/#/c/706331/ | |
| 18:38:56 | dansmith | just for YI if nothing else | |
| 18:42:06 | efried | ack | |
| 18:45:24 | mnaser | catching up on buffer... | |
| 18:45:55 | mnaser | dansmith: ~220kish it didnt fail, ~340kish it failed, mariadb 10.3.something i believe | |
| 18:46:31 | dansmith | mnaser: okay thanks. I guess if anyone asks me I'll say "somewhere around 330k" | |
| 18:46:54 | mnaser | yeah and in this case it would be nice if we purged records so | |
| 18:46:57 | dansmith | er, "300k" I mean | |
| 18:47:07 | mnaser | important note that this doesnt mean "300k active instance" but "300k db records" | |
| 18:47:11 | dansmith | yup | |
| 18:51:29 | mnaser | dansmith: i wonder if this might start a whack-a-mole of references to hidden=false or hidden=null | |
| 18:51:56 | efried | mnaser: I was just leaving a comment to that effect. | |
| 18:52:12 | dansmith | mnaser: maybe except I think these are the only two places :) | |
| 18:52:31 | efried | for now | |
| 18:52:46 | mnaser | yeah, thing is i have no better solution | |
| 18:52:48 | dansmith | not really, because everything else sits above the db layer | |
| 18:52:58 | sean-k-mooney | we porably should be normalising that when we load if form the db | |
| 18:53:00 | dansmith | this is what the db and object apis are *for* | |
| 18:53:09 | dansmith | sean-k-mooney: we are, that's what I'm doing | |
| 18:53:31 | dansmith | the second case is specifically doing a very targeted non-ORM query | |
| 18:53:31 | sean-k-mooney | dansmith: in https://review.opendev.org/#/c/706331/ | |
| 18:53:32 | dansmith | otherwise this would be a single place | |
| 19:01:22 | dansmith | mnaser: added comments to the test to mirror your self-discovery | |
| 19:01:29 | dansmith | efried: added the hidden=False case to the test | |
| 19:01:39 | dansmith | gonna wait to push it up so we can get a run on it | |
| 19:01:45 | dansmith | efried: also added that ref to the commit message | |
| 19:02:16 | efried | ack. dansmith when I re-add the default in the poison patch and run just that test, it works, but when I run the whole suite, the exception is weird | |
| 19:02:23 | efried | AttributeError: hidden | |
| 19:02:47 | dansmith | hmm, that's weird | |
| 19:02:56 | dansmith | I was running just one | |
| 19:02:56 | mriedem | dansmith: question in that test | |
| 19:05:05 | dansmith | mriedem: replied | |
| 19:05:16 | 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) | |
| 19:05:16 | openstackgerrit | Merged openstack/nova stable/queens: Add functional recreate revert resize test for bug 1852610 https://review.opendev.org/699708 | |
| 19:05:49 | efried | fails with AttributeError: stestr run -n nova.tests.unit.db.test_migrations | |
| 19:05:49 | efried | fails with the expected message: stestr run -n nova.tests.unit.db.test_migrations.TestNovaMigrationsSQLite.test_walk_versions | |
| 19:05:49 | efried | activate the venv | |
| 19:05:49 | efried | dansmith: repro is: | |
| 19:06:11 | efried | ...venv is py36 | |
| 19:06:23 | dansmith | efried: oh I was running the whole test_migrations module myself and wasn't seeing the error, but let me try again | |
| 19:06:28 | dansmith | I'm on py37 | |
| 19:07:53 | efried | dansmith: zuul agrees with me | |
| 19:09:40 | efried | but it also fails despite the migration having been 'fixed'. | |
| 19:10:39 | dansmith | hrm | |
| 19:10:55 | dansmith | what version of sqla is in your venv? | |
| 19:11:23 | efried | I just repro'd with a fresh py37 env, so whatever's in requirements. | |
| 19:11:31 | dansmith | can you look so I can compare? | |
| 19:11:45 | efried | SQLAlchemy==1.3.13,sqlalchemy-migrate==0.13.0,sqlparse==0.3.0 | |
| 19:11:51 | sean-k-mooney | tox -e py37 -- nova.tests.unit.db.test_migrations.TestNovaMigrationsSQLite.test_walk_versions | |
| 19:11:59 | sean-k-mooney | passes form me with dans change | |
| 19:12:01 | dansmith | okay I just did a fuuuul run and that failed like yours but doesn't fail for me on just the module | |
| 19:12:12 | dansmith | so must be an ordering thing or something? | |
| 19:12:16 | efried | or something. | |
| 19:12:19 | mriedem | shouldn't column_default_banned default to False for everything before 399? | |
| 19:12:31 | efried | I skimmed looking for other monkey patches of Column, but didn't see any at a glance. | |
| 19:12:35 | mriedem | i guess the default maybe doesn't matter since the test is always passing a value | |
| 19:12:39 | dansmith | mriedem: it does.. set it to zero and it will fail for 216_havana | |
| 19:13:29 | mriedem | yeah it was the default in the fixture that i was thinking about - the default isn't used since the test always overwrites it | |
| 19:13:44 | dansmith | correct | |
| 19:13:54 | efried | if I'm being super picky, either remove the default or specify the kwarg name in the call. | |
| 19:14:14 | efried | but that's so nittish for a test I won't even put it in the review | |
| 19:14:14 | dansmith | efried: you see that the original doesn't specify either I assume | |
| 19:14:24 | dansmith | the existing argument I mean | |
| 19:14:35 | efried | hah, the number of times we've busted Sundar for following a precedent that was wrong... | |
| 19:14:43 | dansmith | python2 used to not allow multi-line with statements so it had to be short | |
| 19:14:46 | mriedem | only other comment on the actual fix is do you want a reno to advertise it since it's going to be backported | |
| 19:15:03 | dansmith | mriedem: yep | |
| 19:15:12 | mriedem | can i -1 for it to get stats!? | |
| 19:15:44 | mriedem | after that i'd be +2 on the fix | |
| 19:16:16 | mriedem | well i guess there is no test for the quota thing but i'll leave that up to you guys, | |
| 19:16:38 | mriedem | in my new job tests are a low concern for people so i'm getting used to not asking for them. | |
| 19:16:41 | sean-k-mooney | efried: dansmith ya i also can repoduce the error with "tox -e py37 -- nova.tests.unit.db.test_migration" so this is like due to incorrect mocking | |
| 19:16:46 | dansmith | mriedem: I didn't because we kinda already test the non-null site, and didn't want to have to replicate the raw-sql create of a null-having record for that too, but I can | |
| 19:17:01 | mriedem | up to you, like i said, any tests are good :) | |
| 19:19:52 | mriedem | mnaser: now that you're getting to train does this mean you're going to start cross-cell resizing like a mad man? | |
| 19:20:15 | mriedem | oh wait, did that land in ussuri? | |
| 19:20:35 | mriedem | ah right it's available in ussuri, nvm me | |
| 19:21:13 | dansmith | mriedem: he would need multiple cells to cross :) | |
| 19:21:24 | mriedem | i know, it was implied as a nudge | |