| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-26 | |||
| 16:39:11 | melwitt | archives things out of cell0, cell1, and cell2 | |
| 16:39:44 | melwitt | but since the fixture auto-targets the last targeted context, it gives a false positive when one of the db api archive methods was *not* properly targeting | |
| 16:40:04 | dansmith | yeah that's what I don't get | |
| 16:40:20 | melwitt | so I went in and removed what I thought was just cruft from the fixture to make it able to catch that, not realizing that would be problematic in any way | |
| 16:43:53 | dansmith | won't this catch if we fail to target? https://review.opendev.org/#/c/507486/65/nova/tests/functional/test_nova_manage.py@1541 | |
| 16:43:57 | dansmith | because we wouldn't have archived in each cell? | |
| 16:44:31 | melwitt | well, last context manager is extremely confusing so I don't fully understand it either. but I tested it and sure enough, removing it makes the test properly fail when the bug is in the patchset | |
| 16:45:13 | melwitt | dansmith: it will. and it passes with last context manager + buggy patchset and fails without last context manager + buggy patchset | |
| 16:45:50 | melwitt | I have to dig in more to figure out how/when last context manager manages to target each of cell0, cell1, and cell2 databases in a way that makes it archive properly when it shouldn't | |
| 16:46:02 | melwitt | when the bug is present | |
| 16:46:07 | dansmith | hmm, sounds fishy to me.. would definitely like to fully understand it either way, because I'd worry that means there's maybe still some behavior we don't expect | |
| 16:46:10 | dansmith | er, understand | |
| 16:47:51 | melwitt | ok. I shall try to figure out exactly what happens re: last context manager. my brain might not be good enough though | |
| 16:57:30 | melwitt | dansmith: I'm in luck, looks like it's a simple reason. the nova-manage command itself targets each cell as it archives and the bug in a previous patchset was that a helper method in db api was not properly targeting any database via a RequestContext https://review.opendev.org/#/c/507486/65/nova/cmd/manage.py@573 | |
| 16:58:20 | melwitt | so each target_cell in manage.py will save last context manager, and then the db api method that is buggily untargeted will pick up that last context manager | |
| 16:59:17 | melwitt | and then archive the records properly in func test land when it does not archive records properly in real life | |
| 17:00:22 | dansmith | nothing in db_api should be doing targeting | |
| 17:00:35 | dansmith | so I'm not sure I understand | |
| 17:00:53 | dansmith | manage iterates the cells, targets, and calls db_api.archiveblah() right? | |
| 17:00:55 | melwitt | right, lemme show you the bug | |
| 17:01:23 | melwitt | that will make it make sense | |
| 17:02:59 | melwitt | dansmith: here, the bug was that this get_engine() call was not being passed context=context in an earlier patchset https://review.opendev.org/#/c/507486/65/nova/db/sqlalchemy/api.py@5442 | |
| 17:03:51 | melwitt | so it had no access to any db connection info that came from cell targeting | |
| 17:03:58 | dansmith | is this because this method isn't using the context manager like the rest of them? | |
| 17:04:26 | dansmith | i.e. @pick_context_manager and friends | |
| 17:05:37 | melwitt | looking, refreshing my memory | |
| 17:06:05 | dansmith | like i'm guessing maybe this method is super old and just didn't get context managerified or something | |
| 17:06:19 | dansmith | because it's only part of manage and not runtime code | |
| 17:06:24 | dansmith | which is a problem anyway | |
| 17:06:58 | dansmith | so if the assertion is that we couldn't catch this with the fixture because the underlying db code was totally not adhering to our db api conventions, then that makes sense | |
| 17:07:17 | dansmith | but it's not a super strong argument to me.. valid for sure, just not big concern I think | |
| 17:08:05 | melwitt | yeah, I'm not sure. the pick_context_manager looks to just pick up the db_connection attr from the RequestContext and does ctxt_mgr.reader.using() whatever that does. it's not directly calling get_engine() | |
| 17:08:30 | dansmith | but those are the places that honor the targeting in the context, | |
| 17:08:46 | dansmith | which is what this helper is missing, and why this helper needed special plumbing into the engine to keep it targeted | |
| 17:08:49 | melwitt | I assume maybe the archive stuff needs to go more manual because it's moving things to shadow tables. but I don't really know, I'm not that familiar with it | |
| 17:09:54 | dansmith | it just needs a pointer to the engine is all | |
| 17:11:05 | dansmith | anyway, I get it.. still meh, but I understand | |
| 17:11:13 | melwitt | you are right that this is a pretty special case | |
| 17:11:34 | melwitt | (special to have a bug like that) | |
| 17:12:26 | dansmith | I need to go through the other uses of get_engine in more detail and figure out if we maybe need some other trap to make sure that we don't get an engine with no context, even in other situations where cell targeting isn't a thing | |
| 17:12:33 | dansmith | but I don't have time for that today | |
| 17:12:42 | melwitt | ok. it caused me a lot of confusion and time spent so I thought there might be value in removing the last context manager weirdness, but if you think it's better to have than not, I can accept that | |
| 17:26:18 | aspiers | efried: found some more issues with SEV series, please bear with me. /cc bbobrov | |
| 17:26:36 | efried | ack | |
| 17:48:17 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 17:48:17 | openstackgerrit | Adam Spiers proposed openstack/nova master: Provide HW_CPU_X86_AMD_SEV trait when SEV is supported https://review.opendev.org/638680 | |
| 17:48:18 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 17:48:18 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract SEV-specific bits on host detection https://review.opendev.org/636334 | |
| 17:48:19 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable booting of libvirt guests with AMD SEV memory encryption https://review.opendev.org/666616 | |
| 17:48:19 | openstackgerrit | Adam Spiers proposed openstack/nova master: Apply SEV-specific guest config when SEV is required https://review.opendev.org/644565 | |
| 18:02:42 | openstackgerrit | Merged openstack/nova master: Convert nova-next to a zuul v3 job https://review.opendev.org/670196 | |
| 18:16:27 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable booting of libvirt guests with AMD SEV memory encryption https://review.opendev.org/666616 | |
| 18:22:40 | artom | This NUMA LM func test is actually some of the most fun code I've written in a while | |
| 18:22:50 | artom | ... not sure that's a good thing :P | |
| 18:25:08 | mriedem | efried: should https://review.opendev.org/#/c/631242/ be rebased on your SDK series now? | |
| 18:25:18 | mriedem | ...or is that a bad idea b/c cyborg isn't in the sdk yet? | |
| 18:26:15 | efried | mriedem: I would like it if cyborg used sdk, but the cyborg blueprint shouldn't have to be dependent on the sdk one. | |
| 18:26:36 | efried | mriedem: If we could merge https://review.opendev.org/#/c/643664/ then yeah, I would say we should do that. | |
| 18:26:50 | efried | any cores around? dansmith? | |
| 18:27:02 | mriedem | he's out, or on the way | |
| 18:27:12 | efried | melwitt? | |
| 18:27:35 | mriedem | efried: in general I'm just wondering about what looks like stalled progress on https://review.opendev.org/#/q/topic:bp/nova-cyborg-interaction+(status:open+OR+status:merged) | |
| 18:28:15 | efried | mriedem: I think the progress is on the cyborg side for the mo https://review.opendev.org/#/c/659233/ | |
| 18:28:15 | mriedem | we're <2 months from FF | |
| 18:28:29 | mriedem | oh ok | |
| 18:29:15 | efried | mriedem: recall at ptg demo time, they were working against a PoC feature branch off of cyborg, not code merged (or even proposed) in master. So they're catching up on that now. | |
| 18:29:40 | efried | though I'll admit progress could be faster. | |
| 18:30:19 | mriedem | yar | |
| 18:30:55 | mriedem | now that we're past spec freeze for train, i'm sure you don't feel like it, but what are your thoughts on a state-of-the-state email about the status of our cycle themes? https://specs.openstack.org/openstack/nova-specs/priorities/train-priorities.html | |
| 18:31:18 | mriedem | i'm writing my monthly report which is why i'm in reporting mode :) | |
| 18:31:49 | efried | mriedem: that sounds like a friggin hoot | |
| 18:32:07 | efried | I should delegate that | |
| 18:32:12 | efried | to someone in reporting mode | |
| 18:32:14 | mriedem | man it looks like https://review.opendev.org/#/c/659233/8 should be split up | |
| 18:32:46 | efried | I agree, but it's such a garbled mess right now it would be tough to tell how. | |
| 18:34:35 | mriedem | right again | |
| 18:35:34 | dansmith | doesn't it have a recipe in the commit message? | |
| 18:35:53 | dansmith | I mean "dos2unix of a file" does not belong in a patch with anything else | |
| 18:35:56 | dansmith | I mean good lord | |
| 18:36:25 | mriedem | yeah i saw that too | |
| 18:36:57 | dansmith | file renames should be their own deal | |
| 18:36:57 | efried | I don't know what kind of recipes you use, that have two 4s. | |
| 18:37:02 | efried | must be why you're so skinny. | |
| 18:37:40 | dansmith | heh | |
| 18:38:12 | efried | Y'all should feel free to add your -1 for those reasons as well. I've done enough ranting on that patch for one day. | |
| 18:38:18 | mriedem | i just did | |
| 18:38:27 | dansmith | can I just -1 with "smh" ? | |
| 18:39:01 | mriedem | given i had to google that, i'm guessing someone from china won't get it either :) | |
| 18:39:23 | dansmith | heh, I feel like I'm 20 years older than my self, but you're like.. eighty | |
| 18:39:25 | efried | I'd rather keep the comments on the constructive side, even if brutal. | |
| 18:39:38 | efried | not much they can do with "smh" | |
| 18:39:41 | dansmith | efried: I wasn't serious of course | |
| 18:39:52 | efried | yeah | |
| 18:39:56 | efried | of course | |
| 18:39:58 | dansmith | but I did see you were like "woah, nuke this whole sumbitch" | |
| 18:40:49 | efried | well, the *probably* can't justify holding a copy of provider_tree, but they *definitely* can't justify holding a copy of os-resource-classes | |
| 18:40:58 | efried | that's just... no. | |
| 18:41:04 | dansmith | heh | |
| 18:41:16 | dansmith | yeah, that's like... well, ... smh | |
| 18:41:17 | efried | but also, yeah, get rid of 90% of the ReportClient | |