| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-09 | |||
| 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 | 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:04 | mnaser | also just a friendly bringing up of this fun one https://bugs.launchpad.net/nova/+bug/1839560 | |
| 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? | |
| 18:17:21 | mriedem | tbachman: he was gone after the last summit | |
| 18:17:23 | mnaser | sorry its been a long day | |
| 18:17:31 | tbachman | I’m so out of touch | |
| 18:17:39 | mriedem | gone but not forgotten | |
| 18:17:41 | tbachman | heh | |
| 18:17:42 | mnaser | i think he's at aws | |
| 18:17:45 | mriedem | i just chatted him up this morning | |
| 18:17:46 | mriedem | he is | |
| 18:17:47 | mnaser | i still see him respond to placement threads | |
| 18:17:53 | mnaser | on the ML | |
| 18:17:55 | mnaser | so not too far away :P | |
| 18:18:31 | tbachman | Just when I thought I was out, they pull me right back in | |
| 18:19:03 | mriedem | those are more like self-inflicted wounds | |
| 18:19:57 | tbachman | LI profile says Principal Open Source Engineer at AWS | |
| 18:20:24 | tbachman | ah, but I guess he’s a kubernetes guy now | |
| 18:20:25 | mriedem | yes he's working on EKS | |
| 18:20:47 | tbachman | thx for bringing me up to date | |
| 18:21:09 | tbachman | checks yahoo email | |
| 18:21:17 | tbachman | (that last one is actally true) | |
| 18:26:12 | mriedem | melwitt: replied, sort of | |
| 18:27:25 | melwitt | mriedem: thanks. yeah, just thought it was weird if you happened to know what/why that is. I hadn't seen it before | |
| 18:38:24 | mriedem | you must be on cve backport duty with lee | |
| 18:42:33 | artom | mriedem, just come work for us, at this point. You know our internal better than we do ;) | |
| 18:42:37 | artom | *internals | |
| 18:43:07 | artom | Actually don't - we already have too many cores on staff. | |
| 18:45:13 | melwitt | heh, well, it's a cve so important to get the fixes merged and out there. and I had the context on the bug and fix for reviewing | |
| 18:45:30 | mriedem | artom: but then i could support the same-company approvals policy! | |
| 18:45:51 | artom | mriedem, sellout | |
| 18:46:04 | mriedem | melwitt: yeah i know, i appreciate the quick reviews | |
| 18:53:53 | mriedem | dansmith: if you prefer handling the DuplicateEntry-on-create-then-update-and-undelete thing here https://review.opendev.org/#/c/675496/ i can go that directoin | |
| 18:54:13 | mriedem | personally i'd like to keep the ironic node id, compute node uuid and resource provider id all aligned for debuggability | |
| 18:54:19 | mriedem | but it's low priority | |
| 18:55:47 | mriedem | the alignment i mean, not the fix that mnaser is waiting on | |
| 19:00:49 | dansmith | mriedem: un-deleting it lets you do that, no? | |
| 19:00:54 | dansmith | but yeah, definitely my preference | |
| 19:05:16 | mriedem | dansmith: right | |