| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-11 | |||
| 13:33:17 | bauzas | mdbooth: https://twitter.com/sylvainbauza/status/951446715470577664 | |
| 13:33:44 | gibi | melwitt: hi!, https://bugs.launchpad.net/nova/+bug/1742688 and https://review.openstack.org/532816 It seems we are the victim of the gate slowness | |
| 13:33:45 | openstack | Launchpad bug 1742688 in OpenStack Compute (nova) "test_live_migration_actions notification sample test fails intermittently with 'notification instance.live_migration_rollback.start hasn't been received'" [Undecided,In progress] - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 13:36:56 | bauzas | gibi: yup, the gate is like super-slow | |
| 13:39:23 | bauzas | mdbooth: so, tell me more about what you want | |
| 13:40:01 | bauzas | mdbooth: you said to have a magical _grab_mdevs(allocations) that would return you a list of mdev UUIDs, right? | |
| 13:40:26 | bauzas | mdbooth: so basically identical to _guest_add_vgpus() except the very last line ? | |
| 13:40:59 | mdbooth | bauzas: Sec, lemme look | |
| 13:42:38 | bauzas | mdbooth: tl;dr: I only care about the guest parameter because I eventually do a guest.add_device() | |
| 13:42:50 | bauzas | but at the very very end of that method | |
| 13:42:54 | mdbooth | bauzas: Yes. There's only like 3 lines of that function which relate to xml generation :) | |
| 13:43:24 | bauzas | and 2 of them are unrelated to the guest itself | |
| 13:43:39 | bauzas | so, then, what would be the interest of splitting that ? | |
| 13:43:49 | mdbooth | bauzas: A few | |
| 13:44:03 | mdbooth | The first as I mentioned is that it's surprising | |
| 13:44:30 | mdbooth | Somebody coming along looking for where this stuff is configured in the workflow isn't going to see _get_guest_xml() and guess that it's there. | |
| 13:44:59 | bauzas | mdbooth: that said, we do that *a lot*, right? :p | |
| 13:45:13 | mdbooth | We do a whole lot of surprising stuff, sure :) | |
| 13:45:21 | bauzas | mdbooth: I mean, that's precisely there where we attach what we want, right ? | |
| 13:45:39 | mdbooth | Second is I suspect we're going to want to have different ways to create this list. | |
| 13:45:41 | bauzas | it's not only the GPU thing, it's all the devices we create | |
| 13:45:45 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Resolve 'oslo.context' deprecation warnings https://review.openstack.org/532822 | |
| 13:45:52 | mdbooth | e.g. creating new mdevs vs re-using existing mdevs | |
| 13:46:16 | mdbooth | s/existing/already allocated to this instance/ | |
| 13:46:25 | bauzas | mdbooth: that is precisely something that should kept hidden from the other libvirt places | |
| 13:46:33 | bauzas | for tech debt reasons | |
| 13:46:34 | mdbooth | Where creating == create or reallocate | |
| 13:46:54 | bauzas | how the vGPUs are implemented in libvirt should be kept very isolated from the rest | |
| 13:47:16 | mdbooth | I'm talking about the code in libvirt/driver.py here | |
| 13:47:20 | bauzas | sure | |
| 13:47:30 | mdbooth | So it shouldn't affect the decisions of other drivers. | |
| 13:47:37 | bauzas | I still think we need to limit the driver knowledge about what is a vGPU | |
| 13:48:16 | mdbooth | I think the guest needs to end up with a list of mdevs it should add | |
| 13:48:37 | bauzas | mdbooth: that's just basically splitting the method in twice | |
| 13:48:53 | bauzas | and introducing a new level of indirection, but okay | |
| 13:49:52 | bauzas | mdbooth: I still don't see why we really need to split in twice but okay | |
| 13:50:03 | mdbooth | Well I think the split is helpful to the reader, because it calls out a different type of operation in a different place. It also allows us to generate the list in different ways based on caller context without having to pass the context all the way down into _get_guest_xml | |
| 13:50:05 | bauzas | it just makes the interfaces a bit cleaner, but that's it | |
| 13:50:38 | mdbooth | e.g. If we're creating a new guest, allocating new instances, vs re-generating our existing xml with our current mdevs. | |
| 13:50:52 | bauzas | mdbooth: a-ha, that's different | |
| 13:51:01 | bauzas | mdbooth: I see your thoughts | |
| 13:51:36 | bauzas | mdbooth: ie. not passing allocated all the way down to _get_guest_xml() but rather just call _give_me_vgpus() in the caller itself ? | |
| 13:51:57 | bauzas | and then the caller attaching those vgpus *after* it got the XML ? | |
| 13:51:57 | mdbooth | bauzas: Something like that would be my preference. | |
| 13:52:08 | mdbooth | No, you have to do it before | |
| 13:52:39 | bauzas | mdbooth: you understand that it's mostly a convenience problem ? | |
| 13:52:41 | mdbooth | So you have to do the host config first, then you can generate the guest xml based on how you did it | |
| 13:53:04 | mdbooth | It's the same as the volume attachment problem. You can't generate the guest xml until you can tell libvirt where you mounted the volume. | |
| 13:53:09 | bauzas | mdbooth: so you would pass the extra bits down _get_guest_xml, right? | |
| 13:53:22 | mdbooth | Yes. | |
| 13:54:52 | bauzas | mdbooth: and then the whole _give_me_mdevs() would be a synchronized section? | |
| 13:55:15 | bauzas | gibi: chairing the nova meeting, right? | |
| 13:55:33 | mdbooth | bauzas: Yes, it would have to be. | |
| 13:55:55 | bauzas | mdbooth: I still consider the synchronized section to be in a follow-up | |
| 13:56:02 | bauzas | mdbooth: timeline wise | |
| 13:56:25 | bauzas | because we're 2 weeks away from the FF, and I don't want that feature to be punted | |
| 13:56:58 | mdbooth | bauzas: Adding that would be way simpler than moving code around out of _get_guest_xml | |
| 13:57:18 | mdbooth | Just a simple host-wide local lock | |
| 13:57:26 | bauzas | I agree | |
| 13:57:49 | bauzas | here are my thoughts | |
| 13:58:07 | bauzas | based on what I see important to be merged before end of Queens | |
| 13:58:16 | bauzas | #1 the patch itself | |
| 13:58:32 | bauzas | (with the synchronized lock) | |
| 13:58:50 | bauzas | #2 the possibility to disallow nova to create mdevs | |
| 13:59:10 | bauzas | #3 the fix for power-on and others | |
| 13:59:28 | bauzas | #4 the possibility to recreate mdevs after a reboot | |
| 13:59:33 | bauzas | and the rest looks optional to me | |
| 13:59:44 | bauzas | we can refine the interfaces in Rocky | |
| 14:00:23 | bauzas | besides the fact that the problem you mention isn't particular to the VGPU feature, but rather for a shit ton of extra devices and features we add to the guest | |
| 14:00:36 | bauzas | mdbooth: agree ? | |
| 14:00:38 | mdbooth | Only volumes, I think. | |
| 14:00:45 | mdbooth | I agree with your prioritisation. | |
| 14:00:55 | gibi | bauzas: I did not get the memo that I'm the chair | |
| 14:00:56 | bauzas | mdbooth: no, we litterally generate all the devices at the same place | |
| 14:01:10 | gibi | bauzas: but I can jump in | |
| 14:01:13 | efried | gibi bauzas Somebody oughtta do it. The time is now :) | |
| 14:03:01 | mdbooth | bauzas: I really think it's only volumes. | |
| 14:03:30 | mdbooth | The others we're just adding devices to the guest. The different with volumes and now vgpus is that we're reconfiguring the host. | |
| 14:03:46 | mdbooth | So we're doing _get_guest_xml() and it has surprising side-effects | |
| 14:03:48 | bauzas | mdbooth: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4882 ? | |
| 14:04:09 | mdbooth | bauzas: Yeah, that's just pulling them out of the instance. | |
| 14:04:16 | mdbooth | IIUC they've already been allocated elsewhere. | |
| 14:04:17 | bauzas | mdbooth: what's the difference between attaching a PCI device or a mediated device ? | |
| 14:04:40 | mdbooth | Because you're not creating the pci device, or doing any kind of 'scheduling' | |
| 14:05:13 | mdbooth | You're not calling _get_guest_xml() and suddenly you've created a new pci device | |
| 14:05:59 | mdbooth | Anyway, I agree with your prioritisation. That said, I really don't like it about volumes, and I'm uncomfortable adding more of it. | |
| 14:06:03 | bauzas | mdbooth: but the allocation was made way earlier :) | |
| 14:06:12 | bauzas | for the VGPU | |
| 14:06:35 | bauzas | if you go to that compute, that's because it has free space for a vGPU | |
| 14:06:45 | bauzas | because placement told you | |
| 14:06:58 | bauzas | exactly like NUMA things | |
| 14:07:12 | bauzas | the scheduler or placement won't know which specific device you're assigned | |
| 14:07:19 | bauzas | but it will know there is room for you | |
| 14:07:39 | bauzas | in theory, you should never raise the exception I added | |
| 14:08:47 | mdbooth | bauzas: There's also the case of recreating the domain xml with existing mdevs. | |
| 14:09:35 | mdbooth | Although perhaps that doesn't matter as long as we're absolutely certain the domain isn't running. | |
| 14:09:58 | mdbooth | But again that's an embedded assumption which makes everything harder to understand. | |
| 14:10:32 | bauzas | mdbooth: the problem is that the XML isn't persisted when you stop the instance | |
| 14:10:38 | bauzas | I mean in Nova | |
| 14:10:46 | bauzas | that's surprising to me | |