Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-26
14:48:58 slaweq aspiers: yw :)
14:56:40 sean-k-mooney aspiers: you can use cidr notation with egreess rules
14:56:55 sean-k-mooney you do not need to use remote sec groups at all
15:09:43 melwitt dansmith: hey, appreciate the review on the last context manager patch. I'm investigating into the our test setup code to be able to reply about some things. thanks again for looking
15:10:16 dansmith melwitt: okay, you haven't rebased the archive patch yet right?
15:10:45 melwitt dansmith: not yet no, wanted to get a run with the post test hook change before shuffling things around
15:11:11 melwitt which I got last evening, just need to rebase from the last context manager patch now
15:11:23 melwitt *away from
15:12:07 melwitt should do that right now actually, I got distracted
15:13:17 openstackgerrit melanie witt proposed openstack/nova master: nova-manage db archive_deleted_rows is not multi-cell aware https://review.opendev.org/507486
15:13:18 openstackgerrit melanie witt proposed openstack/nova master: Verify archive_deleted_rows --all-cells in post test hook https://review.opendev.org/672840
15:14:16 melwitt still have mriedem's +2 on the top patch, yusss
15:14:42 sean-k-mooney mriedem: could you take a second look at https://review.opendev.org/#/c/669738/2 i added the functional tests and its pretttry trivial otherwise so it would be nice to land that
15:27:42 efried https://review.opendev.org/#/c/672800/
15:27:42 efried https://review.opendev.org/#/c/672805/
15:27:42 efried mriedem: Got $NOVA_CPU_CONF working
15:30:24 sean-k-mooney efried: are you finally fixing the fact that we override the /etc/nova/nova-cpu.conf
15:30:34 efried yes, see above.
15:31:25 sean-k-mooney cool i have been setting things for both nova.conf and nova-cpu.conf in my local.conf for years at this point to work around it
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

Earlier   Later