| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-21 | |||
| 14:42:59 | artom | dansmith, I dunno - does the test framework actually call monkey_patch()? | |
| 14:43:28 | artom | Lol yes it does: `import nova.monkey_patch # noqa` | |
| 14:43:33 | artom | First line in nova/test.py | |
| 14:43:43 | dansmith | well, I guess I'm not sure.. we have to in functional to make some of it work I thought | |
| 14:43:56 | dansmith | heh, yeah okay | |
| 14:44:32 | dansmith | in some cases you could get away with it, but in others we'd never be able to do certain things | |
| 14:45:14 | artom | So it could be as stupid as the demonstrator I had in my bug | |
| 14:45:33 | artom | Try to resolve 'lulz.fake', examine the stack... | |
| 14:45:42 | dansmith | yeah | |
| 14:45:43 | artom | Pass if it does NOT contain greendns | |
| 14:45:49 | dansmith | ....yeah | |
| 15:18:37 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: 'video.vram' property must be an integer https://review.opendev.org/753011 | |
| 15:23:29 | stephenfin | gibi, lyarwood: Real simple bugfix there ^ | |
| 15:33:32 | kashyap | stephenfin: Nice sleuthing ... TIL: "This appears to be a Python 3 thing, introduced by division of ints now returning a float." | |
| 15:35:57 | gibi | stephenfin: should https://opendev.org/openstack/nova/src/branch/master/nova/tests/unit/virt/libvirt/test_driver.py#L6738 | |
| 15:36:01 | gibi | stephenfin: fail now? | |
| 15:37:26 | stephenfin | gibi: um, yes, it will | |
| 15:40:07 | bauzas | sorry folks, was borked by internal stuff and the fact i have a presentation to make for a deadline in 10 days | |
| 15:40:15 | bauzas | any reviews you want me to do ? | |
| 15:40:26 | bauzas | stephenfin: remember the presentation deadline, btw. /o\ | |
| 15:40:39 | bauzas | why on Earth did I wanted to provide a proposal ? | |
| 15:40:52 | bauzas | what a curse | |
| 15:41:12 | gibi | bauzas: at the moment I'm not tracking anything for RC1 that needs review | |
| 15:41:18 | bauzas | cool | |
| 15:41:33 | bauzas | gibi: sorry, I hate myself being dragged from RC1 as it looks to me more priority | |
| 15:42:06 | bauzas | and I promised to see a vGPU usage for both V100 and GVT-g, oh man | |
| 15:42:13 | bauzas | I'm doomed | |
| 15:43:40 | gibi | bauzas: no worries | |
| 15:43:49 | gibi | I think we are pretty well set for RC1 at the moment | |
| 15:55:41 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: 'video.vram' property must be an integer https://review.opendev.org/753011 | |
| 16:00:57 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Use cell targeted context to query BDMs for metadata https://review.opendev.org/752459 | |
| 16:00:58 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Clean up the DynamicVendorData constructor https://review.opendev.org/752718 | |
| 16:09:26 | artom | dansmith, looks like you get to dance around me yelling "I told you so" | |
| 16:09:45 | artom | My unit test appears to show we're *still* not disabling greendns correctly | |
| 16:10:20 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Unit test for 7c1d964faa https://review.opendev.org/753072 | |
| 16:10:44 | artom | dansmith ^^ | |
| 16:11:06 | artom | I suppose I should have WIP'ed it, but it's not going to pass Zuul regardless | |
| 16:25:38 | artom | Huh, by the time nova/monkey_patch.py runs, we've *already* imported evenlet | |
| 16:38:19 | dansmith | artom: meaning the tests defeat the fix because they import eventlet before they've run the thing that sets-before-import like cmd/* should? | |
| 16:38:50 | artom | dansmith, no, I think the test shows that the fix doesn't work, because something else imports eventlet before Nova does | |
| 16:40:22 | dansmith | but is that an artifact of our unit tests vs. real production stuff, or is there another place? | |
| 16:40:35 | dansmith | I thought you validated the fix locally on a real deployment and it had the desired effect? | |
| 16:40:52 | artom | dansmith, I did not. | |
| 16:41:05 | dansmith | ah, okay, I guess I thought from the bug report you had | |
| 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 | |