Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-26
16:32:35 mriedem sean-k-mooney: i've been busy with some stuff most of the morning and i'm going to lunch soon but i have that in a tab for later
16:33:04 mriedem efried: same for you
16:34:15 mriedem melwitt: i would have expected https://review.opendev.org/#/c/507486/65 to be smaller after you rebased it away from https://review.opendev.org/#/c/672604/1 ?
16:35:45 melwitt mriedem: oh, so I wasn't sure whether I should toss out the func test or if it still has any value
16:36:10 mriedem_lunch well, i thought the func test didn't work w/o the other change so that's why i was confused
16:36:36 melwitt it does a false positive for _archive_rows_for_table (whatever it's named) is not targeted. but it might have other value so I wasn't sure whether to kill it
16:37:37 melwitt mriedem_lunch: it works, it just returned a false positive when there was a bug in targeting for one of the archive db api methods
16:38:26 dansmith a proper test should have multiple cell dbs,
16:38:32 dansmith and archive things out of all of them, right?
16:38:39 melwitt if you think it's not useful given the nova-next testing, I'm fine with throwing it away. just wasn't sure what you'd want
16:38:46 dansmith so I would expect if it's pedantic enough it would catch the non-targeting behavior
16:39:00 melwitt dansmith: right, that's what the func test does
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 and to config.py https://review.opendev.org/636318
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 :)

Earlier   Later