| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-26 | |||
| 14:34:52 | aspiers | sean-k-mooney: yeah I remember the history from 2012 :) | |
| 14:35:41 | sean-k-mooney | aspiers: well stpehn is activly deleteing the remainder of the code this cycle | |
| 14:35:47 | aspiers | finally \o/ | |
| 14:35:54 | sean-k-mooney | i think he has done cellsv1 | |
| 14:36:07 | sean-k-mooney | and some of the xen only console stuff | |
| 14:36:16 | sean-k-mooney | nova networks is next on his list | |
| 14:36:46 | sean-k-mooney | after he finishes the cpu pinning via placement stuff | |
| 14:39:39 | slaweq | aspiers: from what I'm now looking into Neutron documentation, I think this remote_group_id isn't really explained there | |
| 14:39:42 | slaweq | :( | |
| 14:39:55 | aspiers | slaweq: thanks for checking! | |
| 14:40:00 | aspiers | that sounds like a doc bug | |
| 14:40:09 | slaweq | aspiers: yes, indeed | |
| 14:40:16 | slaweq | aspiers: but I hope it's clear for You now :) | |
| 14:40:21 | aspiers | I think so | |
| 14:40:45 | aspiers | is it also possible to have an egress rule with remote secgroup set? | |
| 14:41:07 | aspiers | so that the rule only matches outgoing traffic to another (or maybe the same) secgroup? | |
| 14:41:18 | slaweq | aspiers: yes | |
| 14:41:26 | aspiers | cool | |
| 14:41:41 | slaweq | technically it creates ipset with IP addresses which belongs to other ports with same SG | |
| 14:41:54 | slaweq | and use this ipset in match conditions for rule | |
| 14:42:10 | aspiers | makes sense | |
| 14:42:13 | slaweq | but if You want to do so, please remember to remove default rule which allows all egress traffic | |
| 14:42:29 | slaweq | otherwise it will match first and still all will work fine | |
| 14:42:51 | slaweq | s/work fine/be allowed | |
| 14:42:52 | slaweq | :) | |
| 14:42:59 | aspiers | got it | |
| 14:43:12 | aspiers | OK think I understand much better now, thanks a lot! | |
| 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 | mriedem: Got $NOVA_CPU_CONF working | |
| 15:27:42 | efried | https://review.opendev.org/#/c/672805/ | |
| 15:27:42 | efried | https://review.opendev.org/#/c/672800/ | |
| 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 | |