| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-21 | |||
| 16:48:02 | sean-k-mooney | there are 7 direct imports and 5 from eventlet import... | |
| 16:48:18 | dansmith | artom: it's not tox, it's stestr | |
| 16:48:25 | artom | dansmith, right, sorry | |
| 16:48:35 | dansmith | and it does it in parallel and often in different orders based on the bucketing | |
| 16:48:54 | artom | In that case we can't really have a unit test for this | |
| 16:49:13 | artom | We would need a way to guarantee "first run" | |
| 16:49:23 | artom | In order to be the first to import eventlet | |
| 16:49:30 | artom | `del` and importlib.reload() don't work, I tried | |
| 16:50:05 | sean-k-mooney | you could remove it form the module path but honestly im not sure this is worth it | |
| 16:50:22 | dansmith | artom: this is what I was saying earlier, which might require some poking into eventlet | |
| 16:50:44 | artom | dansmith, ok, it only me like 2 hours to catch up with you | |
| 16:50:48 | sean-k-mooney | artom: do you jsut want to call the un patched socket lib | |
| 16:50:52 | sean-k-mooney | *module | |
| 16:50:53 | dansmith | sean-k-mooney: I do think *some* sort of validation is worth it, because artom didn't confirm by hand, says it's hard to do so, and five minutes ago, wasn't confident that the fix was actually a fix :) | |
| 16:51:02 | artom | And you've been breathing smoke for 2 weeks, so your brain is at like 20% capacity | |
| 16:51:16 | sean-k-mooney | dansmith: im more wondering what is the intent of the test | |
| 16:51:27 | artom | sean-k-mooney, to make sure we've properly disbled greendns | |
| 16:51:43 | sean-k-mooney | when called via a monkeypatched module | |
| 16:51:59 | dansmith | sean-k-mooney: well, for one thing, a unit test of that monkeypatch code to make sure it's doing the things we expect seems like a good plan to me | |
| 16:52:05 | artom | Yeah, so something like socket.gethostaddr() | |
| 16:52:10 | artom | Like I did in my demonstrator code | |
| 16:52:15 | dansmith | I think we started that around uwsgi time and have been lumping stuff in there from the start without really much validation | |
| 16:52:25 | sean-k-mooney | artom: do you have a link to the orginial patch | |
| 16:52:37 | sean-k-mooney | i just see the unit test but its hard to follow without context | |
| 16:52:46 | artom | sean-k-mooney, https://review.opendev.org/#/c/751302/ | |
| 16:53:17 | dansmith | artom: maybe we go with the approach that the profiler thing uses, which is another unittest run after a successful regular one so you can constrain the ordering? | |
| 16:54:20 | artom | dansmith, that's just an extra line in tox.ini... | |
| 16:54:28 | dansmith | right | |
| 16:54:45 | dansmith | we've had multiple bugs related to the ordering of this monkeypatch thing, so .. maybe it's time :) | |
| 16:55:11 | artom | So, an entirely separate file, outside of the unit tests, that just import nova.monkey_patch and tests stuff? | |
| 16:55:13 | dansmith | like every line in that function is "be suuper careful to do this before that | |
| 16:55:25 | sean-k-mooney | are we sure that the coment is correct | |
| 16:55:36 | artom | sean-k-mooney, which comment | |
| 16:55:46 | sean-k-mooney | that eventlet processes theis at import time and not when we call eventlet.monkeypatch | |
| 16:55:48 | dansmith | sean-k-mooney: I'm not sure any of this is correct because apparently nobody has tested it :P | |
| 16:56:29 | artom | sean-k-mooney, fairly certain, by looking at the eventlet code: https://github.com/eventlet/eventlet/blob/v0.26.0/eventlet/green/socket.py#L20 | |
| 16:56:30 | sean-k-mooney | this one https://review.opendev.org/#/c/751302/2/nova/monkey_patch.py@33 | |
| 16:56:54 | sean-k-mooney | ok so that is module scope | |
| 16:57:08 | sean-k-mooney | so it will only be processed once the first tim ethe socket module is loaded | |
| 16:57:27 | artom | 'zactly | |
| 16:57:40 | sean-k-mooney | which wont be when we do import eventlet | |
| 17:00:16 | sean-k-mooney | its import here https://github.com/eventlet/eventlet/blob/fc8cc0f6a77896234284ef40d7226d62345922c0/eventlet/patcher.py#L424 | |
| 17:00:34 | artom | Ah, true | |
| 17:00:42 | artom | But... when is that called? | |
| 17:00:56 | artom | Because it definitely *behaves* like it's processing EVENTLET_NO_GREENDNS at import time | |
| 17:01:05 | artom | IOW, it has to be set before importing eventlet | |
| 17:01:09 | sean-k-mooney | form eventlet.monkey_patch | |
| 17:01:10 | artom | Otherwise it has no effect | |
| 17:01:46 | openstack | Launchpad bug 1895322 in OpenStack Compute (nova) ussuri "Nova is not actually disabling greendns" [Undecided,Fix committed] | |
| 17:01:46 | artom | Look at my Demonstration in https://bugs.launchpad.net/nova/+bug/1895322 | |
| 17:04:45 | sean-k-mooney | this is where its being patched as far as i can tell https://github.com/eventlet/eventlet/blob/fc8cc0f6a77896234284ef40d7226d62345922c0/eventlet/patcher.py#L274-L314 | |
| 17:10:01 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: [WIP] Test for 7c1d964faa and monkey patching in general https://review.opendev.org/753072 | |
| 17:10:10 | artom | dansmith, sean-k-mooney ^^ so that actually works | |
| 17:10:56 | dansmith | artom: sure, but I think you can maybe also get away with it being a proper unittest and run it with stestr in the normal way | |
| 17:11:32 | dansmith | if you make your module do stuff before it imports nova.test or whatever causes the import chain that leads to eventlet, I think stestr won't tickle anything else until you do | |
| 17:11:34 | artom | dansmith, how though? I thought we established we can't have other tests importing eventlet before us | |
| 17:11:41 | dansmith | if you don't discovery and don't run anything else | |
| 17:11:48 | sean-k-mooney | artom: i think this is where we currently monkeyp[atch for test by the way https://github.com/openstack/nova/blob/master/nova/test.py#L24 | |
| 17:12:08 | dansmith | artom: I still mean a special invocation like you have, but a unittest instead of a bare python file | |
| 17:12:29 | sean-k-mooney | artom: so when youd do "from nova import test" | |
| 17:12:37 | sean-k-mooney | that shoudl monkey patch | |
| 17:12:48 | artom | Right, but unittest means stestr will read the whole tree... and so will cause eventlet to be imported, even if we don't execute the tests themselves | |
| 17:13:02 | dansmith | no, | |
| 17:13:05 | dansmith | not without discover I think | |
| 17:13:50 | dansmith | for other unittest runners, if you provide an actual class module without discovery, it will try to import that path direct and then execute | |
| 17:13:51 | artom | Oh, you literally mean `--no-discover` | |
| 17:13:52 | sean-k-mooney | dansmith: we would have to put it in a seperate folder | |
| 17:13:58 | dansmith | artom: right, like the profile test does | |
| 17:14:13 | artom | *facepalm* OK yeah | |
| 17:14:15 | sean-k-mooney | we could skip using a regex if we did that and use the --isolate parmater i think | |
| 17:14:24 | dansmith | sean-k-mooney: no, you can just make it skip for the regular tests, if necessary, or let it run in the regular ones if it will falsely claim success anyway :) | |
| 17:14:25 | artom | Worth a try, anyways | |
| 17:14:40 | dansmith | sean-k-mooney: you could just make it skip if sys.modules already has eventlet | |
| 17:14:54 | sean-k-mooney | dansmith: ah | |
| 17:14:55 | artom | This bare Python file is definitely teh uglies | |
| 17:15:06 | sean-k-mooney | i tought you were going to sugges adding it to the test blacklist | |
| 17:15:13 | sean-k-mooney | but having the test auto skip i think is fine | |
| 17:15:31 | sean-k-mooney | that way we dont need to list a load of special tests if we add more like htis | |
| 17:15:40 | dansmith | right | |
| 17:15:41 | artom | Auto-skip is dangerous, no? Means we'd be silently missing potential errors | |
| 17:15:52 | sean-k-mooney | no in the ci | |
| 17:15:55 | sean-k-mooney | locallly sure | |
| 17:16:03 | dansmith | artom: well, you can also do what the profile test does and set an envar to enable :) | |
| 17:16:05 | sean-k-mooney | but if its run by tox | |
| 17:16:11 | sean-k-mooney | it should be fine | |
| 17:16:39 | artom | dansmith, ah, so folks can disable it if needed | |
| 17:16:50 | artom | Whodda thunk precedent was so useful ;) | |
| 17:16:51 | sean-k-mooney | artom: proably enable if needed | |
| 17:16:52 | dansmith | artom: no, | |
| 17:16:59 | sean-k-mooney | given its racy by defualt | |
| 17:17:12 | dansmith | artom: it's disabled by default, enabled explicitly in that second stestr run command in tox | |
| 17:17:14 | sean-k-mooney | you would set the env var wehn you run it explcitly | |
| 17:17:25 | sean-k-mooney | yep what dansmith said | |
| 17:17:28 | artom | dansmith, sorry, right | |
| 17:17:49 | artom | I meant in the sense of... tox will run with it enabled | |
| 17:18:03 | artom | Everything else won't | |
| 17:18:06 | sean-k-mooney | tox -e py<whatever> sure | |
| 17:18:19 | dansmith | seriously, there are a bunch of other things in that file with comments like "this is fragile if order isn't respected" comments... it'd be nice to get tests for those things too since we're clearly just flying by the seat of our pants on these things | |
| 17:18:26 | sean-k-mooney | but in two calls to stester | |
| 17:18:47 | artom | Wait, no, that second tox command that I'll add will enable and run it | |