Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-09-21
13:58:36 sean-k-mooney :)
14:14:13 openstackgerrit Merged openstack/nova stable/train: Correctly disable greendns https://review.opendev.org/751622
14:31:07 dansmith artom: I didn't see any discussion of a test for this: https://review.opendev.org/#/c/751302/
14:31:26 artom dansmith, there wasn't
14:31:37 dansmith artom: since we've had a few ordering things changed in there in recent memory, can't we have a test that pokes at eventlet internals or something to check that things happen like we expect?
14:31:37 artom How would we test that thing specifically?
14:31:51 artom dansmith, ah I see what you mean
14:31:59 artom dansmith, yeah, could be done
14:32:01 artom I guess?
14:32:39 dansmith well, it would make it easier to validate that you've really changed behavior.. clearly the original code didn't think the ordering mattered, so if it does, it'd be nice if we could, you know, validate that
14:33:28 artom dansmith, I agree with your point. Originally I 1. couldn't think of a way to test specifically this and 2. figured the NOTE I left was the best we could do to prevent future reorderings
14:33:56 dansmith but I'm guessing the reviewers just assumed that it does matter and that your change fixed it sufficiently,
14:34:31 dansmith and it'd just be nice to have a test where you can revert the code change and make sure the test validates some wrong behavior, which is (or used to be) common double checking for obscure fixes like this
14:36:23 artom dansmith, I think it still is - we try to do the "regression test that assers the wrong thing -> fix + test fix" thing when we can
14:36:38 artom I suppose in this case we all assumed it wasn't realistic
14:37:50 dansmith assumed but didn't even discuss or question, it seems... just sayin', this is pretty fundamental code, merged pretty quick and backported quick as well.. would be nice to have something to validate it
14:38:09 dansmith like, maybe even the version of eventlet on stable/train behaves a little different or something
14:39:55 artom dansmith, yep, I'mma try to hax something
14:41:11 dansmith I think maybe one tricky thing is going to be that a test will run after eventlet has already been imported and this monkey patching has been done, which means you just need to have the test check that greendns is disabled in the internals or whatever,
14:41:27 dansmith and then testing that the patch fixed it will just be running the same test but without the code change
14:41:59 dansmith meaning, the test won't actually call monkey_patch() to do the test, but just examine the result of it and make sure it's good after the test harness did
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

Earlier   Later