| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-09 | |||
| 17:27:48 | mgoddard | but if it fails because I'm running it at the wrong point then that's bad | |
| 17:28:20 | dansmith | you know that it will *usually* not return zero right? | |
| 17:28:26 | dansmith | so I mean, again, define "fail" | |
| 17:28:37 | mgoddard | yeah, we allow 0 or 1 | |
| 17:29:05 | dansmith | IIRC, the point of it not using the db models is so it can run entirely against the old schema and do its checks, | |
| 17:29:10 | mgoddard | let me try to rephrase | |
| 17:29:17 | dansmith | so barring bugs, I think you will get constrained results even before db sync | |
| 17:29:44 | mgoddard | in order to get the 'right' answer out of it, should I run upgrade checks only after doing the DB sync and migrations? | |
| 17:30:02 | dansmith | I don't think it's that simple, | |
| 17:30:16 | dansmith | because I think that we've had some checks which were "hey before you do anything you need to manually change X" | |
| 17:31:00 | dansmith | mriedem has done 95% of the work on this and definitely has the current scope/intent in his head more than me, so I guess I'll just shut up, | |
| 17:31:01 | mgoddard | so it could be useful to run both before DB sync, and after? | |
| 17:31:09 | dansmith | but I think you're kinda assuming more about it than you should | |
| 17:31:27 | mgoddard | perhaps | |
| 17:32:32 | openstackgerrit | Merged openstack/nova stable/queens: Add functional recreate test for bug 1764556 https://review.opendev.org/673824 | |
| 17:32:32 | openstack | bug 1764556 in OpenStack Compute (nova) queens ""nova list" fails with exception.ServiceNotFound if service is deleted and has no UUID" [Medium,In progress] https://launchpad.net/bugs/1764556 - Assigned to Matt Riedemann (mriedem) | |
| 17:32:38 | openstackgerrit | Merged openstack/nova stable/queens: Add functional regression test for bug 1778305 https://review.opendev.org/673825 | |
| 17:32:39 | openstack | bug 1778305 in OpenStack Compute (nova) queens "Nova may erronously look up service version of a deleted service, when hostname have been reused" [Medium,In progress] https://launchpad.net/bugs/1778305 - Assigned to Matt Riedemann (mriedem) | |
| 17:34:45 | mgoddard | thanks for the input dansmith, I'll let mriedem weigh in if he disagrees otherwise I'll go with an early check | |
| 17:35:11 | mgoddard | and possibly a second after the sync/migration | |
| 17:35:28 | dansmith | just to be clear, | |
| 17:36:01 | dansmith | well, nevermind | |
| 17:39:16 | melwitt | mriedem: question in https://review.opendev.org/674908 | |
| 17:49:04 | openstackgerrit | Merged openstack/nova stable/queens: Don't generate service UUID for deleted services https://review.opendev.org/673827 | |
| 17:49:37 | mriedem | mgoddard: grenade runs the db syncs before the upgrade status check and fails if the return code is >= 2: https://github.com/openstack/grenade/blob/00ad9a89684360ca53d7cdda62763de37ba98510/projects/60_nova/upgrade.sh#L94 - which aligns with the doc (that's probably part of where i got that for writing the doc), | |
| 17:49:48 | mriedem | running nova-status upgrade check *before* db sync is _undefined_ | |
| 17:49:50 | mriedem | we don't test it that way | |
| 17:50:13 | mriedem | we don't use the versioned object model code, we do straight sqlalchemy db execution stuff, | |
| 17:50:33 | mriedem | but i wouldn't be surprised if at some point someone writes an upgrade check that is looking at maybe a column that doesn't exist until you've sync'ed, idk | |
| 17:50:53 | mriedem | but like dansmith said, i think we try to avoid that and could justify saying something like that is a bug | |
| 17:52:13 | dansmith | mriedem: for things where we need to warn the person that a blocker migration is going to hork on their db state, they'd need to run it ahead of time right? | |
| 17:52:16 | mriedem | i see takashin has a patch up for your bug https://bugs.launchpad.net/nova/+bug/1833278 | |
| 17:52:16 | openstack | Launchpad bug 1833278 in OpenStack Compute (nova) "nova-status upgrade check should fail if db sync has not been performed" [Medium,In progress] - Assigned to Takashi NATSUME (natsume-takashi) | |
| 17:52:23 | dansmith | meaning, if we're about to drop a column or add a constraint | |
| 17:52:27 | dansmith | I thought we had one of those early on | |
| 17:52:29 | mriedem | dansmith: takashi's patch would break that if we did | |
| 17:53:03 | dansmith | I really thought one of the first uses for nova-status was to warn people before we added a constraint, but I don't remember the details | |
| 17:53:18 | mriedem | earliest checks were for cells v2 and placement | |
| 17:53:19 | mriedem | in ocata | |
| 17:53:49 | mriedem | https://github.com/openstack/nova/blob/stable/ocata/nova/cmd/status.py#L358 | |
| 17:53:53 | melwitt | dansmith: I thought the same thing. like, "you have homework to do before running db migrations" | |
| 17:54:33 | melwitt | or maybe it's before online data migrations? | |
| 17:54:53 | dansmith | it tells you if you *haven't* run online migrations, | |
| 17:55:18 | mriedem | right for some definitely - there was a check for missing request specs which was solved with an online data migration, | |
| 17:55:22 | dansmith | but I really thought we were prescribing running it before to handle things like column drops, constraints, etc | |
| 17:55:26 | mriedem | and couldn't be done with a blocker migratoin b/c the records are in different dbs | |
| 17:55:45 | melwitt | yeah ok | |
| 17:57:02 | mriedem | as far as i know we haven't dropped a column or added a new constraint in a loong time | |
| 17:57:25 | mriedem | so haven't really had to think about the ordering of that plus the upgrade check | |
| 17:58:28 | dansmith | mriedem: but, if people are only ever running this after db sync, we can't rely on this to help us in that regard | |
| 17:58:49 | dansmith | and basically have to stick to blocker migrations | |
| 17:58:58 | mriedem | or you add the check in the n-1 release | |
| 17:59:18 | mriedem | "i see you've got stuff in column x which is going to be dropped in the next release, you should do y to drain it" | |
| 17:59:21 | mriedem | or whatever | |
| 17:59:33 | mriedem | we've added checks that get backported, like for the ironic flavor migratoins in pike | |
| 18:00:03 | dansmith | if we're going to require db sync first, why did we go with raw sqla access then? | |
| 18:00:18 | mriedem | but i don't think we've ever said this is an end-all perfect tool, it's part of the whole box - blocker migratoins, online data migratoins, runtime data migrations, and the checks to automate release notes | |
| 18:00:21 | dansmith | I mean, that was the whole point of doing that, AFAIK, so we could talk to the schema independent of the models | |
| 18:00:31 | dansmith | sure | |
| 18:00:36 | mriedem | i'm not saying db sync is required, | |
| 18:00:51 | mriedem | i'm saying that's how we test it, and how it behaves before db sync is undefined | |
| 18:01:02 | dansmith | ack, | |
| 18:01:05 | mriedem | if we want to support both ways, we need to test it both ways | |
| 18:01:12 | dansmith | but I think the question is about intent and assumptions and what-not, | |
| 18:01:19 | dansmith | especially in the face of that patch to require it | |
| 18:01:38 | mriedem | yup, which is a result of a bug that mgoddard opened | |
| 18:01:48 | mriedem | to i guess make us shit or get off the pot | |
| 18:02:01 | mriedem | i prefer to wallow thank you very much | |
| 18:02:30 | mriedem | and i think he did that b/c he's getting pushback in kolla about adding the checks in | |
| 18:02:41 | mriedem | https://review.opendev.org/#/c/665643/ | |
| 18:03:50 | dansmith | his patch actually doesn't make it required, so much as it just checks that you've done it I guess | |
| 18:03:51 | dansmith | which is less concerning | |
| 18:05:27 | dansmith | if the expectation of the tool is to tell you go/no-go for deploying new code, then it's probably fine to have that be an ERROR and not a WARN | |
| 18:05:37 | mriedem | https://review.opendev.org/#/c/667047/10/nova/cmd/status.py@395 is a fail | |
| 18:05:44 | dansmith | right, but ^ | |
| 18:05:54 | mriedem | and https://review.opendev.org/#/c/667047/10/nova/cmd/status.py@370 | |
| 18:06:13 | dansmith | it's still legit to run that before you do db sync to catch other soft changes knowing that the error is expected before you've done anything | |
| 18:07:00 | mriedem | maybe, but then you have to build some logic into your ansible that detects which check failed and ignore this one if you know you're doing before-sync checks | |
| 18:07:04 | dansmith | we could have a pre-upgrade command in that tool which doesn't run stuff like that, but does do things like sanity checks, | |
| 18:07:16 | mriedem | which gets back to something mnaser originally asked for which was more machine-readable output | |
| 18:07:41 | dansmith | we could also have the pre-upgrade run any of the checks from the previous release as error->fail, and any of the current release checks as error->warn or something | |
| 18:07:44 | mriedem | separating the commands would make more sense imo | |
| 18:07:47 | dansmith | and just move them each release | |
| 18:08:03 | mriedem | then you have a defined behavior and we'd test that in grenade | |
| 18:08:13 | dansmith | yeah | |
| 18:08:19 | mnaser | i did this a really long time ago but yeah -- https://review.opendev.org/#/c/576944/ | |
| 18:12:04 | mnaser | also just a friendly bringing up of this fun one https://bugs.launchpad.net/nova/+bug/1839560 | |
| 18:12:04 | openstack | Launchpad bug 1839560 in OpenStack Compute (nova) "ironic: moving node to maintenance makes it unusable afterwards" [High,In progress] - Assigned to Matt Riedemann (mriedem) | |
| 18:12:41 | mriedem | i've had it with friendship | |
| 18:13:18 | mriedem | mnaser: i said we should ping dansmith about that in the morning, and now i've already exceeded my sweet dan quota for the day on upgrade checkers and queens backport reviews | |
| 18:13:38 | dansmith | yeah you were done by 8am | |
| 18:13:41 | mriedem | ha | |
| 18:13:44 | dansmith | please try again on monday | |
| 18:13:55 | mriedem | dan is out of the office and out of clothes | |
| 18:15:24 | mriedem | mnaser: i also feel like i woke up at 2am thinking about a way to avoid the revert and still fix the unique constraint thing in a backportable way, but i think it eludes me. something something catch DuplicateEntry on ComputeNode.create(), look for existing deleted node with the same uuid, and just update it and set deleted=0. | |
| 18:15:28 | mriedem | but that's pretty hacky | |
| 18:16:12 | mnaser | that does feel pretty hacky | |
| 18:16:24 | mnaser | does the compute_node entry get created after or before the 'hypervisor' entry | |
| 18:16:43 | mnaser | aka maybe that hack can happen *only if* its using ironic but i dunno, seems like we're digging ourself a bigger hole | |
| 18:17:05 | mriedem | hypervisor entry? | |