Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-09-21
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
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

Earlier   Later