Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-09-21
11:50:59 sean-k-mooney and some code that was already there.
11:51:23 gibi I did see that nova and libvirt generates the func number differnetly I think libvirt adds the prefix while nova doesn't
11:51:38 gibi and libvirt accepts both
11:53:17 sean-k-mooney we strip the prfix when generating the adress doing [2:]
11:53:41 sean-k-mooney i was basically passing an unprefixed string and that endup passing the empty stiring to oslo
11:54:15 sean-k-mooney oslo pciadress field then rejected it since the func part of the pci adress was empty
11:57:37 openstackgerrit Claudiu Belu proposed openstack/nova master: hyperv: Configures chassis asset tags for VMs. https://review.opendev.org/752723
12:53:09 openstackgerrit sean mooney proposed openstack/nova master: libvirt: delegate ovs plug to os-vif https://review.opendev.org/602432
13:37:19 gibi sean-k-mooney: please remind me which patch is the one where you faced this pci function issue?
13:40:11 sean-k-mooney https://review.opendev.org/#/c/602432/18
13:40:22 sean-k-mooney the followup to the one you jsut reviewd
13:40:47 sean-k-mooney this is the detach impleamtion https://review.opendev.org/#/c/602432/18/nova/tests/unit/virt/libvirt/fakelibvirt.py@1084
13:40:51 gibi sean-k-mooney: thanks. then I stoppped to early in my review on that series
13:40:54 gibi continuing now
13:41:36 sean-k-mooney the second hunk in that file is where i had dropped the 0x prefix
13:41:49 sean-k-mooney that was the second issue
13:44:53 sean-k-mooney gibi: the first patch really is a fix for a different bug but i just happen to need that to maintain the same behavior for my own patch since libvirt would nolonger be unplugging the vifs in this case now that os-vif handels the pluging.
13:48:05 sean-k-mooney gibi: by the way i have already accpeted that these wont qualify for rc1 and they will be backported after wards so while i am glad for the review if something else needs review for rc1 then that obviously is higher priority. thanks in anycase.
13:50:52 gibi sean-k-mooney: ack
13:51:08 gibi at the moment I'm not tracking anything for RC1 that needs review
13:52:22 sean-k-mooney ya same. i think lees ubuntu cloud archive patches have also merged in devstack at least the ussuri one i think so the libvirt bumps are really the only think im mindful off that we should still try to land
13:52:36 sean-k-mooney i think we are in a pretty good state overall
13:53:19 gibi yepp
13:55:05 sean-k-mooney gmann: care to send https://review.opendev.org/#/c/747123/ through the gate now that the ussuri version has merged
13:58:06 gmann sean-k-mooney: done
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

Earlier   Later