Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-09-21
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 artom Look at my Demonstration in https://bugs.launchpad.net/nova/+bug/1895322
17:01:46 openstack Launchpad bug 1895322 in OpenStack Compute (nova) ussuri "Nova is not actually disabling greendns" [Undecided,Fix committed]
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
17:18:53 artom Nothing else will because it's racy
17:19:01 dansmith urllib3, oslo_context, threading things,
17:19:10 dansmith artom: right, exactly.. you know, like the profiling test :)
17:19:21 sean-k-mooney dansmith: ya we have a cople of example of this
17:19:39 artom dansmith, you keep saying that like I'm some sort of genius that understands it the first time around
17:19:41 artom ;)
17:19:53 sean-k-mooney not sure why oslo_context would be on that list but why not
17:19:57 dansmith artom: it just makes me feel better to highlight the first-mention-to-grok time gap :)
17:20:09 dansmith sean-k-mooney: maybe if we had a test with comments in it, we'd know :/
17:20:33 artom dansmith, I'm pretty sure it's exponential with age (and inverse with sleep )
17:20:40 sean-k-mooney :) well it was more oslo is an openstack thing so hopefully we could fix the oslo issue there instead of nova
17:20:47 dansmith artom: we should plot it on a graph
17:21:14 dansmith sean-k-mooney: not likely if it's something oslo can't do if imported post-monkey-patch
17:21:16 artom ∞ there's your graph
17:21:19 dansmith heh
17:22:41 sean-k-mooney https://bugs.launchpad.net/nova/+bug/1773102
17:22:41 openstack Launchpad bug 1773102 in OpenStack Compute (nova) queens "Abnormal request id in logs" [Medium,Fix committed] - Assigned to Radoslav Gerganov (rgerganov)

Earlier   Later