| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-01-18 | |||
| 11:53:19 | lyarwood | so you can call the second with persistent=False | |
| 11:53:23 | lyarwood | as we did previously | |
| 11:54:42 | kashyap | gibi: FWIW, I summarized the IRC chat here: https://review.opendev.org/c/openstack/nova/+/770246/7/nova/virt/libvirt/driver.py#2330 | |
| 11:55:02 | gibi | lyarwood: still that first timeout seems wrong, why not we get an event | |
| 11:55:34 | lyarwood | gibi: I think it's a single event once detached from both the persistent and live config | |
| 11:55:53 | gibi | lyarwood: and we need to send two detach to have detach it from both config? | |
| 11:56:06 | gibi | if yes then it make sense | |
| 12:08:56 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove 'hypervisor_version' from 'libvirt_info' https://review.opendev.org/c/openstack/nova/+/744199 | |
| 12:13:19 | lyarwood | gibi: one request can detach the device from both | |
| 12:13:40 | lyarwood | gibi: it just depends on the guestOS for the device to be detached from the live config | |
| 12:15:20 | gibi | lyarwood: so the first detach attempt triggers the detach from both the persistent and the live config. The detach call returns, but we timeout waiting for the event. So I have to assume that either a) we didn't wait long enough to get the event b) we _have to_ send two detach to get the device detached from the live config. | |
| 12:16:09 | gibi | or c) libvirt does not send the event in some case | |
| 12:16:22 | gibi | but that seems wrong | |
| 12:17:33 | lyarwood | right so for a and b we retry the detach | |
| 12:17:48 | lyarwood | we can even check if the device is in either config before doing that | |
| 12:18:02 | lyarwood | but it should always be gone from the persistent config at that point | |
| 12:18:21 | lyarwood | just to go back to the original blind retry issue | |
| 12:19:05 | lyarwood | the issue with that approach in CI was the time between actual detach calls to libvirt and then down into QEMU was tiny | |
| 12:19:35 | lyarwood | so much so that they would overlap and cause the undocumented QEMU behaviour | |
| 12:20:15 | lyarwood | with your approach I really think it's safe to wait for longer between attempts to detach now, so something like 20 to 30 seconds | |
| 12:20:41 | lyarwood | I know you're waiting for 60 at the moment but as I said before the previous behaviour was to start at 5 seconds and increase that with each iteration | |
| 12:22:21 | sean-k-mooney | Yumeng: +1 on the vgpu spec. https://review.opendev.org/c/openstack/nova-specs/+/750116 | |
| 12:22:44 | sean-k-mooney | gibi: bauzas ^ if you have time to re review i think that is ready | |
| 12:22:48 | gibi | I still don't see the difference between the old and the new approach. In both code wait a predefined time before retry, and in both code this waiting is only happen if the first try fails, if the first try succeeds then no waiting happens. So I don't see why we are in the better situation with the new code | |
| 12:23:12 | gibi | sean-k-mooney: ack | |
| 12:23:30 | sean-k-mooney | gibi: didnt the old code retyr regardless of failure or success | |
| 12:23:45 | sean-k-mooney | i tought it just started teh time out and retired without waiting | |
| 12:23:49 | gibi | sean-k-mooney: the old case also checked the existence of the device in the live config before retry | |
| 12:23:50 | sean-k-mooney | for an event | |
| 12:24:13 | sean-k-mooney | gibi: right but that is not a vaild check | |
| 12:24:28 | gibi | sean-k-mooney: that is what we do both in the old and the new code | |
| 12:24:35 | sean-k-mooney | the issue is that with new qemu issuing a second detach cancles the operation | |
| 12:24:56 | gibi | sean-k-mooney: but it seems waiting for the event times out | |
| 12:24:57 | sean-k-mooney | so it may still be in the live domain but that does not mean you can send a second detach | |
| 12:25:09 | lyarwood | right, the main thing I wanted here was to allow n-cpu to wait longer between requests to libvirt to detach | |
| 12:25:45 | gibi | sean-k-mooney: it seems that the libvirt detach call returns at the same time when the detach event is sent, so the whole thing seems synchronous | |
| 12:25:50 | lyarwood | thus making it harder if not impossible for us to hit the weird undocumented QEMU behaviour | |
| 12:26:07 | gibi | sean-k-mooney: but then I don't know how we was able to step on the first detach handling with the retry | |
| 12:26:52 | sean-k-mooney | gibi: i think that depends on the load | |
| 12:26:53 | gibi | lyarwood: but then finding the right timeout is the goal both in the new and the old code. but for that we don't have to blow up the old code | |
| 12:27:06 | sean-k-mooney | i dont think it is synconos jsut look that way with light load | |
| 12:27:19 | sean-k-mooney | gibi: you could simulate that with cgroups/taskset | |
| 12:27:33 | gibi | sean-k-mooney: above kashyap and lyarwood said it looks synchronous | |
| 12:27:33 | lyarwood | gibi: so the old code is also a mess | |
| 12:27:36 | sean-k-mooney | you could limit the execution time of libvirtd | |
| 12:27:51 | gibi | lyarwood: fair the old code is a mess and can be factored to be cleaner | |
| 12:28:01 | sean-k-mooney | gibi: i see havent looked at ti too closely | |
| 12:28:12 | lyarwood | gibi: and about things being sync or async, I was talking about the failure event | |
| 12:28:28 | gibi | lyarwood: ohh, I missed that then | |
| 12:28:37 | sean-k-mooney | so the even is asyc right | |
| 12:28:38 | lyarwood | gibi: I think the success event can be async | |
| 12:28:39 | gibi | lyarwood: so the happy case still can be async? | |
| 12:28:45 | sean-k-mooney | but on a fast host it will be almost instant | |
| 12:28:49 | lyarwood | right | |
| 12:28:50 | gibi | cool, then the event waiting code is better | |
| 12:29:14 | lyarwood | right that allows us to wait longer | |
| 12:29:31 | lyarwood | as we should get an event back when the device does eventually detach | |
| 12:30:40 | gibi | I will do more test to see why we don't get any event back from libvirt at the first detach attempt | |
| 12:30:45 | gibi | in the above scenario | |
| 12:30:56 | gibi | as that missing event is bothering me | |
| 12:31:00 | sean-k-mooney | gibi: you could try doing "sudo cpulimit -l 1 -e libvirtd" or "sudo cpulimit -l 1 -p <libvirtd pid>" | |
| 12:31:09 | gibi | sean-k-mooney: thanks I will try | |
| 12:31:26 | sean-k-mooney | that should limit libvirt to 1% of 1 core | |
| 12:33:43 | lyarwood | gibi: we don't get the event as the device is still attached to the live config | |
| 12:34:06 | lyarwood | gibi: and failure is for sync failures where QEMU rejects the request to detach | |
| 12:34:07 | gibi | lyarwood: but we expect that libvirt will detach from the live config as we asked for it | |
| 12:34:17 | gibi | in the first request | |
| 12:34:19 | lyarwood | gibi: we expect that it should try | |
| 12:34:32 | gibi | and if fails then it should tell us it failed, isn't it? | |
| 12:35:04 | kashyap | gibi: lyarwood: So, Peter Krempa came back; let me post the chat summary snippet here, from #virt, OFTC: | |
| 12:35:05 | lyarwood | gibi: no, again the failure event is for when QEMU tells libvirt that it can't detach the device when the request is initially made | |
| 12:35:36 | lyarwood | gibi: if that request is accepted but the guestos doesn't allow the device to detach then we don't get anything back | |
| 12:35:49 | lyarwood | that's my understanding at least, lets see what kashyap has | |
| 12:36:50 | kashyap | lyarwood: Yeah; now I hear some additional subtleties; I'm going to post the full summary on the change; and we all can read and discuss here | |
| 12:48:20 | kashyap | gibi: lyarwood: Done; see here (I also like DanPB's advice on dealin w/ active vs. inactive config separately): https://review.opendev.org/c/openstack/nova/+/770246/7/nova/virt/libvirt/driver.py#2330 | |
| 12:51:29 | kashyap | Let me know if that answers your question; if not, please note there, if anything doesn't add up :-) | |
| 12:51:34 | gibi | sorry, I'm on a call | |
| 12:51:53 | kashyap | No rush at all | |
| 13:00:34 | gibi | kashyap: thanks for the summary, I will work DanPB's suggestion into the patch as handling the live an the persistent config separately feels cleaner to me too | |
| 13:03:01 | kashyap | Np | |
| 13:22:21 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Bump MIN_{LIBVIRT,QEMU}_VERSION and NEXT_MIN_{LIBVIRT,QEMU}_VERSION https://review.opendev.org/c/openstack/nova/+/754700 | |
| 13:22:21 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Drop support for Xen https://review.opendev.org/c/openstack/nova/+/743231 | |
| 13:43:36 | openstackgerrit | Brin Zhang proposed openstack/nova master: Nova supports password encrypted VNC https://review.opendev.org/c/openstack/nova/+/622336 | |
| 14:24:40 | stephenfin | sean-k-mooney: Can you fix the failures on https://review.opendev.org/c/openstack/nova-specs/+/765901 real quick? | |
| 14:25:42 | sean-k-mooney | oh it failed yes ill do it now | |
| 14:26:04 | sean-k-mooney | sorry should have ran pep8 | |
| 14:30:27 | sean-k-mooney | stephenfin: for the leading underscore | |
| 14:30:30 | sean-k-mooney | for [2] | |
| 14:30:37 | sean-k-mooney | shoudl it be [_2] | |
| 14:30:38 | stephenfin | *trailing | |
| 14:30:42 | stephenfin | [2]_ | |
| 14:30:51 | sean-k-mooney | oh | |
| 14:31:07 | sean-k-mooney | and the first one does not need that? | |
| 14:31:20 | stephenfin | you already have it on the first one :) | |
| 14:31:41 | sean-k-mooney | oh for [1]_ | |
| 14:31:49 | sean-k-mooney | i have another instance of [2] before that | |
| 14:31:56 | sean-k-mooney | cool ill fix them all | |
| 14:35:36 | openstackgerrit | sean mooney proposed openstack/nova-specs master: add port scoped numa affinity spec https://review.opendev.org/c/openstack/nova-specs/+/765901 | |
| 14:36:19 | sean-k-mooney | stephenfin: i think ^ fixes everything | |
| 14:36:30 | sean-k-mooney | let me know if there is anything else and ill respin | |