| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-21 | |||
| 09:09:18 | kashyap | lyarwood: If you don't mind holding your nose tightly ... here's the repro: https://kashyapc.fedorapeople.org/device-attach-and-detach.bash | |
| 09:10:24 | kashyap | (Fun aside: changing the target dev from sdX to vdX increases the execution of the above script by at least 20 seconds. So attaching 'virtio' disks seems to "slow"? Can't be so) | |
| 09:20:37 | kashyap | That said, I should rewrite it in Python and make it as close to what Nova is doing as possible | |
| 09:23:13 | kashyap | Because, I learnt that if we suspsect a timing issue, 'virsh' has a lot more overhead. A naive connection-based approach is better | |
| 09:50:06 | hemanth_n | sean-k-mooney: Appreciate if you can have a final look on this patch https://review.opendev.org/#/c/749175/ .. thank you | |
| 09:55:26 | lyarwood | kashyap:k~. | |
| 09:55:37 | lyarwood | ops weird connection issue | |
| 09:56:05 | lyarwood | kashyap: ack yeah that sounds like a sane approach, I don't think it's the rapid attach/detach that's the issue btw, just the overall load on the underlying host and libvirtd | |
| 09:58:42 | kashyap | lyarwood: Oh, right; perhaps I should increase the load to 80% and do a detach? | |
| 09:58:55 | kashyap | I'll vary the load (by measuring it in parallel with `sar -u 5`) and see | |
| 10:00:52 | lyarwood | yup sounds good | |
| 10:01:57 | tosky | lyarwood: nice work on the live migration/evacuate stuff, it seems that part is fine | |
| 10:02:05 | tosky | now only the grenade one is missing | |
| 10:10:26 | lyarwood | tosky: np thanks for leading the effort :) | |
| 10:10:53 | lyarwood | gibi / stephenfin ; https://review.opendev.org/#/q/owner:self+topic:native-zuulv3-migration+status:open+project:openstack/nova ^ re the above, the live-migration and new evacuate job should be ready for review now. | |
| 10:14:25 | lyarwood | actually let me update the TODO for the ceph jobs | |
| 10:14:46 | openstackgerrit | Lee Yarwood proposed openstack/nova master: zuul: Introduce nova-evacuate https://review.opendev.org/744883 | |
| 10:14:47 | openstackgerrit | Lee Yarwood proposed openstack/nova master: zuul: Replace nova-live-migration with zuulv3 jobs https://review.opendev.org/752557 | |
| 10:15:19 | lyarwood | there we go, should be good now if the gate behaves | |
| 10:16:16 | tosky | gates and Monday, uhm | |
| 10:16:40 | tosky | there are only 90 (check) + 16 (gate) jobs, so we may be lucky | |
| 10:19:50 | lyarwood | hmmm I wonder if it's actually worth merging the nova-live-migration and nova-evacuate job now that the playbook is written | |
| 10:32:24 | tosky | up to you and the time needed to run the tests vs the time needed to deploy | |
| 10:41:07 | gibi | lyarwood: I have some nits in https://review.opendev.org/#/c/744883 | |
| 10:41:18 | gibi | lyarwood: will you do a follow up or respin it quickly? | |
| 10:42:43 | gibi | brinzhang: sorry I think that refactor needs to wait for W | |
| 10:44:01 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Enable 'vmcoreinfo' feature by default https://review.opendev.org/752912 | |
| 11:01:28 | sean-k-mooney | hemanth_n: sure ill go over it later today | |
| 11:01:41 | hemanth_n | sean-k-mooney: thanks | |
| 11:01:55 | lyarwood | gibi: ack can do | |
| 11:09:57 | openstackgerrit | Lee Yarwood proposed openstack/nova master: zuul: Introduce nova-evacuate https://review.opendev.org/744883 | |
| 11:09:58 | openstackgerrit | Lee Yarwood proposed openstack/nova master: zuul: Replace nova-live-migration with zuulv3 jobs https://review.opendev.org/752557 | |
| 11:22:21 | gibi | lyarwood: thanks | |
| 11:38:29 | openstackgerrit | Yao wang proposed openstack/nova master: Use VIR_DOMAIN_XML_INACTIVE for detach volume https://review.opendev.org/749489 | |
| 11:43:11 | sean-k-mooney | gibi: stephenfin i have figured out why the func test are failing. one issue is i was not adding the 0x prefix if you use "%(func)o" or "%(func)x" it does not genreate the prefix, the main issue however is device detach was only implemented for disk in the fake libvirt driver | |
| 11:44:21 | sean-k-mooney | so im really not sure how the sriov detach func test actully are passing on master but once i fixed the 0x prefix and added a valid impleamtion of detach the tests seam to be passing. now i just need to run all the other func and unit tests before pushing | |
| 11:48:29 | gibi | I guess it only verifies the compute.manager and resource_tracker part of the functionality | |
| 11:49:04 | sean-k-mooney | ya it must never check the doamin xml. anyway that should now be correct too. | |
| 11:49:59 | sean-k-mooney | i dont really know why my minimal chages to the test code caused it to now fail but im going to fix this in a seperate patch below my actul change | |
| 11:50:35 | sean-k-mooney | although maybe i cant actully | |
| 11:50:50 | sean-k-mooney | since im changin some code im adding in my patch | |
| 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 | artom | How would we test that thing specifically? | |
| 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: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 ^ | |