| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-21 | |||
| 16:41:22 | artom | You need to hit a specific situation, which is hard to reproduce on the kind of deployment I have access to | |
| 16:41:32 | artom | So I went on the demonstrator code snippet | |
| 16:41:43 | dansmith | okay | |
| 16:42:02 | artom | ... which doesn't work if something else has imported evenlet before you even do anything | |
| 16:42:45 | sean-k-mooney | artom: nothing else should be importing evenetlet | |
| 16:43:42 | artom | sean-k-mooney, dunno what to tell you, but if I `if 'eventlet' in sys.modules` *before* we import it, it's there | |
| 16:43:43 | sean-k-mooney | at least nothing else should be monkeypatching | |
| 16:44:05 | artom | sean-k-mooney, monkeypatching doesn't matter here, the env variable we care about is processed by eventlet at import-time | |
| 16:44:10 | artom | https://github.com/eventlet/eventlet/blob/af407c77f208ceefe5a35e39aed0cf3fdfc07cb9/eventlet/green/socket.py | |
| 16:45:51 | sean-k-mooney | well i guess teh issue you will have is that you cant contol the order in which the tests run | |
| 16:46:18 | sean-k-mooney | we import eventlets in a number of different tests | |
| 16:46:49 | artom | As in directly `import eventlet`? | |
| 16:46:55 | sean-k-mooney | https://github.com/openstack/nova/search?q=%22import+eventlet%22&unscoped_q=%22import+eventlet%22 | |
| 16:46:58 | sean-k-mooney | yep | |
| 16:47:21 | artom | Ah, so maybe that's it | |
| 16:47:25 | sean-k-mooney | actully better https://github.com/openstack/nova/search?q=%22import+eventlet%22+path%3A%2Fnova%2Ftests&unscoped_q=%22import+eventlet%22+path%3A%2Fnova%2Ftests | |
| 16:47:51 | dansmith | artom: that's what I was saying.. an artifact of the test infrastructure that doesn't really happen in prod | |
| 16:47:53 | artom | I suppose passing a test name regex to tox doesn't prevent it from reading the entire tests tree and thus importing eventlet | |
| 16:48:01 | dansmith | in prod we control the entry ordering exactly, but not so much in the test stuff | |
| 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 | 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 | |