| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-11-02 | |||
| 14:13:39 | sean-k-mooney | dansmith: so would you be ok with adding max_instances to the mdev_type config section | |
| 14:14:01 | sean-k-mooney | or is that still too static | |
| 14:14:02 | bauzas | sean-k-mooney: if we go this way, this wouldn't be backportable, right? | |
| 14:14:10 | dansmith | well, I'm not sure that's a solution entirely, but I think making the operator tell us some of the math so we can do it better on the backend makes sense | |
| 14:14:30 | bauzas | I really tried to propose https://review.opendev.org/c/openstack/nova/+/899625 as a backportable mechanism | |
| 14:14:31 | dansmith | I'd need to hear the whole story before "being okay" with it, but that seems like a good direction to me | |
| 14:15:03 | bauzas | dansmith: finish your call and then we could do a gmeet | |
| 14:15:09 | dansmith | sure | |
| 14:15:11 | sean-k-mooney | bauzas: i dont really like that because it implies your still doing the math incorrect | |
| 14:15:14 | bauzas | I have the PTG summary to write | |
| 14:15:36 | dansmith | I don't love the "quick, delete the provider" approach either, FWIW | |
| 14:15:45 | bauzas | sean-k-mooney: it implies people were lazy enough to not explicitely tell which PCI devices to allocate | |
| 14:16:21 | bauzas | dansmith: that's understandable, but then you're pro-statically defining a list of VFs or an integer counting allocatable VFs | |
| 14:16:26 | sean-k-mooney | bauzas: and we are not internally trackign which device the mdev is allcoated form meaning we are not mapping the allcoations correctly in placment | |
| 14:16:30 | bauzas | (just clarifying the options) | |
| 14:17:17 | sean-k-mooney | removing the rp works if and only if we gurentte that the mdevs are created by packing the RPs | |
| 14:17:20 | bauzas | sean-k-mooney: if you want to have my frank opinion, I'm not a big fan of changing all the allocations we make | |
| 14:17:47 | dansmith | bauzas: I'm for doing better math, and it sounds like some hints for that math (in config) are necessary | |
| 14:17:48 | bauzas | sean-k-mooney: I very humbly think you're wrong on the fact we miscount | |
| 14:18:10 | bauzas | we very correcly count which mdevs against RPs | |
| 14:18:18 | dansmith | still would need to understand the whole picture *and* how we migrate to it, impact to existing people, and whether or not it's worth it if mdevs are going away | |
| 14:18:52 | bauzas | the latter conditional is hard to guess | |
| 14:19:03 | bauzas | but that's fair assumptions | |
| 14:19:37 | bauzas | and I'm not particularly attached to the bugfix itself | |
| 14:20:00 | bauzas | but again, I have my wills : a simple approach not redesigning our placement tracking which would be backportable | |
| 14:20:06 | sean-k-mooney | bauzas: in sriov mode is aviable_instances alway 1 | |
| 14:20:26 | sean-k-mooney | becasue for intels implemation (which they never released) it was not | |
| 14:20:30 | bauzas | see the bug report, yah until all mdevs are created | |
| 14:20:48 | bauzas | I literally wrote that in the bug description | |
| 14:21:10 | sean-k-mooney | what i dont like about that is that is a vendor specific detail | |
| 14:21:32 | bauzas | hell yeah, hence why I wanted to make the fix agnostic | |
| 14:22:09 | bauzas | we're just doing some math in some method that say 'oh look, that inventory is now 0, but meh, I don't care updating it' | |
| 14:22:38 | sean-k-mooney | well your solution does not work if the VF have aviable_instance>1 initally | |
| 14:22:40 | bauzas | this is honestly to me a wrong approach, and at least a LOG warning should tell the operator something weird happened | |
| 14:22:57 | bauzas | beg your pardon ? | |
| 14:23:09 | sean-k-mooney | simple case | |
| 14:23:17 | sean-k-mooney | we have one mdev capable device | |
| 14:23:31 | sean-k-mooney | it has an mdev type that allow 2 allcations total | |
| 14:23:37 | sean-k-mooney | it creats n VF | |
| 14:23:55 | sean-k-mooney | each vf reporst aviabel_invetnory=2 | |
| 14:24:14 | bauzas | (assuming then you're not talking of the nvidia driver then) | |
| 14:24:15 | sean-k-mooney | i allcoate one mdev from two different vfs | |
| 14:24:34 | sean-k-mooney | you can delete the other vf rps | |
| 14:24:56 | sean-k-mooney | but we will would be miss reporting the 2 remain rps | |
| 14:25:09 | bauzas | are you talking of something inexistent now but as a future possibility ? | |
| 14:25:54 | sean-k-mooney | im talkign generically | |
| 14:26:05 | bauzas | again, I don't understand your logic, but let's imagine your scenario for a sec | |
| 14:26:20 | bauzas | say I have 16 VFs, each of them with available_instances=2 | |
| 14:26:42 | bauzas | say now I create one mdev twice, each on a distinct VF | |
| 14:27:00 | bauzas | for 2 VFs, available_instances would be equal to 1 | |
| 14:27:07 | bauzas | two mdevs would exist | |
| 14:27:17 | bauzas | so the inventory wouldn't change for those | |
| 14:27:26 | sean-k-mooney | right but only 2 can be created | |
| 14:27:31 | bauzas | we would still count : total=2 | |
| 14:27:40 | sean-k-mooney | i gues what would happen is that when i allcoate the first mdev | |
| 14:27:49 | sean-k-mooney | the aviable_insstance on all vfs would go to 1 | |
| 14:27:56 | sean-k-mooney | and the second one it would go to 0 | |
| 14:28:05 | bauzas | okay, then it's the driver's responsibility to change available_instances to 0 for all the VFs | |
| 14:28:12 | bauzas | including the two ones we use | |
| 14:28:17 | sean-k-mooney | ya | |
| 14:28:35 | bauzas | total would be one for 2 RP inventories then, right? | |
| 14:28:44 | bauzas | total=1 I mean | |
| 14:28:48 | sean-k-mooney | so the only mdev hardware i have used that supprotes sriov vf did not have that probelm | |
| 14:29:10 | bauzas | anyway, this isn't possible with nvidia SR-IOV, all VFs *have* available_instances=1 | |
| 14:29:42 | bauzas | you lost me, honestly | |
| 14:29:47 | bauzas | if you're talking of T4s | |
| 14:30:01 | bauzas | what we had was a basically non-SRIOV accountancy | |
| 14:30:09 | sean-k-mooney | no im talking about hardware i used at intel and trying to think if it would break | |
| 14:30:23 | sean-k-mooney | i think the delete your are proposing would work | |
| 14:30:49 | bauzas | tbc, I've experienced a race condition | |
| 14:30:49 | sean-k-mooney | but i dont like the idea of us creating and deleting rps in genreal based on workload usage | |
| 14:31:09 | bauzas | since we update resource providers on a periodic | |
| 14:31:29 | bauzas | I was able to create 3 VMs (and allocate three VGPU allocations) | |
| 14:31:43 | bauzas | that's why I added the claim stuff | |
| 14:32:02 | bauzas | so, see, again, I'm not particularly attached to do this | |
| 14:32:19 | bauzas | if we go with statically defining which VFs are going to be used, fair | |
| 14:32:24 | sean-k-mooney | bauzas: we normally run the update avaiabel reosuces call elsewhere | |
| 14:32:32 | bauzas | I have checked | |
| 14:32:40 | sean-k-mooney | bauzas: we expcitly do it form the comptue manager in move claims | |
| 14:32:42 | bauzas | we do run the update *before* the spawn | |
| 14:32:53 | bauzas | not *after* :) | |
| 14:33:05 | dansmith | bauzas: agreed, deleting RPs as a result of allocation feels very wrong | |
| 14:33:26 | sean-k-mooney | dansmith: the only mitigation is we dont delete the mdev after the instance is moved/deleted | |
| 14:33:39 | sean-k-mooney | so provide the compute node is not restated | |
| 14:33:46 | sean-k-mooney | we will do the delete once | |
| 14:33:47 | dansmith | could we set reserved=total instead of deleting maybe? | |
| 14:33:57 | sean-k-mooney | but the extra RPs will get recated after a host reboot | |
| 14:34:15 | bauzas | dansmith: we would need to implement the reverse logic then | |
| 14:34:20 | sean-k-mooney | dansmith: reserved=total is what i was orgianlly thinking but the issue is total is going to 0 | |
| 14:34:26 | sean-k-mooney | and total cant be 0 | |
| 14:34:38 | sean-k-mooney | so we would need to do resrved=total=1 | |
| 14:34:47 | bauzas | and when to unreserve ? | |
| 14:34:57 | bauzas | say I delete my mdev ? | |
| 14:35:11 | bauzas | I'm a stubborn operator that deletes the mdevs, y'know | |
| 14:35:22 | sean-k-mooney | or you do a host reboot | |
| 14:35:39 | bauzas | honestly, I like the config approach | |
| 14:35:43 | sean-k-mooney | so ya the reserved=total approch is more complext then delete | |
| 14:35:56 | dansmith | sean-k-mooney: right I meant reserved=old_total | |
| 14:36:13 | bauzas | if you specific the max VGPUs per type, then we could do htis | |
| 14:36:34 | bauzas | I feel a disturbance in the force when I tell about deleting RPs | |