| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-06 | |||
| 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 | sean-k-mooney | dansmith: in https://review.opendev.org/#/c/706331/ | |
| 18:53:31 | dansmith | the second case is specifically doing a very targeted non-ORM query | |
| 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 | mriedem | dansmith: question in that test | |
| 19:02:56 | dansmith | I was running just one | |
| 19:05:05 | dansmith | mriedem: replied | |
| 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: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:49 | efried | dansmith: repro is: | |
| 19:05:49 | efried | activate the venv | |
| 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 | fails with AttributeError: stestr run -n nova.tests.unit.db.test_migrations | |
| 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 | dansmith | efried: you see that the original doesn't specify either I assume | |
| 19:14:14 | efried | but that's so nittish for a test I won't even put it in the review | |
| 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 | |
| 19:21:34 | mriedem | because there has been some vexxhost multi-cell chatter for awhile | |
| 19:21:49 | mriedem | does cern still upgrade nova? | |
| 19:37:18 | dansmith | ah I see the problem | |
| 19:37:37 | dansmith | I dunno why it doesn't always happen, but it's also not going to be an easy fix :/ | |
| 19:39:49 | mnaser | mriedem: hah. yeah,ii think for cross-cell there's a few mountains to climb first | |
| 19:40:09 | mnaser | like figuring out glance with multiple backends and nova cells with different ceph backends in each one | |
| 19:40:24 | mnaser | and time | |
| 19:40:46 | dansmith | mnaser: that's in the works fwiw | |
| 19:40:58 | dansmith | glance has to do a thing first and then I plan to get on the nova side | |
| 19:46:41 | dansmith | efried: mriedem: the reno for this should just have an item in "fixes:" ? or critical? or upgrade? | |
| 19:47:34 | mriedem | either fixes or upgrade, or both i guess? | |
| 19:48:27 | mriedem | so i guess if you do upgrade, you can say if you haven't rolled to this point yet make sure you do first rather than like train GA, but if you have already upgraded to train GA and hit this issue, you can manually update the records (maybe after an archive/purge)? | |