| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-10-18 | |||
| 13:50:09 | bauzas | wfm so | |
| 13:50:16 | sean-k-mooney | ya its listraly just a file exist check | |
| 13:50:21 | dansmith | bauzas: but is that really ideal? I mean, it's a bit of a reach, but there are other busses on other systems, and I can totally imagine this sort of virt-friendly hardware someday exposing something totally different there | |
| 13:50:44 | dansmith | (that == being so tied to pci addresses specifically) | |
| 13:50:45 | sean-k-mooney | dansmith: you are correct that mdev does not require pci device parents | |
| 13:51:00 | bauzas | dansmith: I don't disagree with you long-term, but fwiw, the only vfio-mdev devices I've seen rely on being PCI addresses | |
| 13:51:02 | sean-k-mooney | that is an atritifal limiation in novas code base today to simplfy the code | |
| 13:51:12 | dansmith | yeah | |
| 13:51:32 | bauzas | so I'd please defer that PCI untanglement until some real hardware comes up which isn't PCI based | |
| 13:51:39 | bauzas | serial or whatever it would be | |
| 13:51:44 | sean-k-mooney | bauzas: tehre are once that use cpx? compute express link what ever the acfronum is | |
| 13:51:56 | sean-k-mooney | ah CXL | |
| 13:51:58 | dansmith | so the plan is to stick to pci addresses for now, but validate them by checking in sysfs and tolerate non-pci things if they're in /virtual/<addr> ? | |
| 13:52:35 | bauzas | sean-k-mooney: I haven't yet asked to add CXL into nova and I'll stick with playing the ostrich for now :) | |
| 13:52:44 | bauzas | dansmith: yeah | |
| 13:52:46 | sean-k-mooney | actully my orgianll proposal a few days ago was for device_adress to be set to the subpath under /sys/devices | |
| 13:52:58 | sean-k-mooney | so that it woudl work in any buss | |
| 13:53:29 | sean-k-mooney | bauzas: i have talked about CXL for nova several times and we have also said not yet | |
| 13:53:30 | bauzas | only for mediated devices | |
| 13:53:38 | dansmith | sean-k-mooney: ah yeah that's interesting | |
| 13:53:46 | bauzas | mdev capable devices won't show up in /sys/devices | |
| 13:54:05 | sean-k-mooney | bauzas: they will under /sys/devices/pci/ | |
| 13:54:17 | bauzas | ah yeah you're right | |
| 13:54:37 | bauzas | yay, another good idea worth implementing it someday | |
| 13:55:07 | sean-k-mooney | so bacially for now i think the minimal change would be "if not a pci adress assume its a virual device" | |
| 13:55:41 | sean-k-mooney | in the futre we can add a sysfs check if we want, but if we plan do do that in the future we shoudl consider upgraades now | |
| 13:55:43 | bauzas | I can try to write it the way it prepares ourselves to admit that pci devices are just a subset of all the devices that can create mdevds | |
| 13:56:17 | bauzas | sean-k-mooney: I think reading sys https://review.opendev.org/c/openstack/nova/+/898100/2/nova/virt/libvirt/driver.py#7975 is cheap and worthwhile | |
| 13:56:59 | sean-k-mooney | bauzas: it is an you can resue the funciton for the cpu ofline fucntions | |
| 13:57:12 | sean-k-mooney | bauzas: i also suggested this a few days ago | |
| 13:57:51 | bauzas | yup, and there are multiple things we can do this cycle about how we do the liaison | |
| 13:58:10 | bauzas | I'm still having this enhancement on me about using libvirt for creating the mdev | |
| 13:58:43 | sean-k-mooney | speaking of which i think its fair to say htat after the ptg wheree we should discuss some of this we should likely capture the plan in a spec | |
| 13:59:54 | sean-k-mooney | but for sysfs reading in genal plwad use https://github.com/openstack/nova/blob/master/nova/filesystem.py#L28 if you do implement this | |
| 14:00:05 | bauzas | sean-k-mooney: yup, I'll | |
| 14:01:02 | sean-k-mooney | besided the fact this should have a spec, the main reason i want one is to defien the scoep and what will be doen in which order and wthat will eb defered to a future release | |
| 14:01:14 | sean-k-mooney | partly so we dont forget soemthing | |
| 14:02:36 | bauzas | don't disagree | |
| 14:11:06 | dansmith | sean-k-mooney: bauzas I didn't save a link to the bug filed yesterday about the upgrade check, but was there any discussion there about revert vs. change to warning? | |
| 14:11:39 | bauzas | dansmith: I very briefly looked at your convo this morning, can you explain it again please ? | |
| 14:11:52 | bauzas | iirc, it was because of non-slurp | |
| 14:11:56 | dansmith | no | |
| 14:12:23 | dansmith | bauzas: basically sean-k-mooney strong-armed me into adding a status check for comptue-object-ids in this last cycle, which raised an error if you hadn't completed the upgrade (which you wouldn't have yet anyway) | |
| 14:12:59 | dansmith | I begged for mercy but there was no mercy given so I added the check and then an op showed up yesterday wondering why I had ruined his day | |
| 14:13:06 | dansmith | (I may be exaggerating here) | |
| 14:13:24 | dansmith | anyway, the status check really should be *next* cycle saying "you haven't finished your homework from last time" | |
| 14:13:52 | dansmith | so we have two options: 1. just revert out the check entirely from 2023.2 or 2. convert it to a warning result | |
| 14:14:20 | sean-k-mooney | fyi https://paste.opendev.org/show/bGuYFgfWt70oGOHcAuH9/ | |
| 14:14:28 | sean-k-mooney | it looks like its working for me too | |
| 14:14:37 | sean-k-mooney | but i have not tried to boot any vms with those mdevs yet | |
| 14:15:37 | sean-k-mooney | i have allcoated 4 mdevs in this case | |
| 14:15:38 | bauzas | sean-k-mooney: I went down to create and even sshing to the cirros guest, the serial was present | |
| 14:15:46 | bauzas | dansmith: recollecting | |
| 14:16:21 | sean-k-mooney | bauzas: ack ill do that quickly too just to confirm but i htink this is usabel for startting to do ci | |
| 14:16:36 | dansmith | \o/ | |
| 14:16:50 | sean-k-mooney | we have some basic mdev/gpu testsin whitebox | |
| 14:17:04 | sean-k-mooney | so we coudl try extending the job to depend on your patch and enable those | |
| 14:17:20 | dansmith | sean-k-mooney: what happens if you try to migrate an instance with a non-migratable mdev? Maybe that's something we can test in normal multinode tempest in the short term.. that we don't break | |
| 14:17:36 | sean-k-mooney | unfortunetlly the tests as written wont work | |
| 14:17:44 | sean-k-mooney | since they depend on pci | |
| 14:18:17 | dansmith | oh we have to do this in whitebox because we need to know mdev details on the host I guess? | |
| 14:18:25 | sean-k-mooney | dansmith: good question i currently only have singel node devstack deployed so cant check but sylvain should be able to just try that | |
| 14:18:38 | sean-k-mooney | dansmith: no we can do this is tempest too | |
| 14:18:44 | dansmith | okay | |
| 14:18:49 | dansmith | seems like a good negative test to add | |
| 14:18:57 | sean-k-mooney | we just would not eb able ot look at the xml but we can just check for the device in the guest via ssh | |
| 14:19:00 | dansmith | to make sure we don't sh*t the bed if the migration fail for that reason | |
| 14:19:29 | sean-k-mooney | i would expect libvirt to fail the migration wehn we call migrate_to_uri3 or whatever the libvirt function is | |
| 14:19:43 | sean-k-mooney | same as if the cpu compatiable check in libvirt fails | |
| 14:19:49 | dansmith | yep, just seems like a good check if it's easy | |
| 14:19:50 | sean-k-mooney | or if you have hostdev devices | |
| 14:19:55 | bauzas | dansmith: good question, I know that we test some move ops | |
| 14:20:20 | sean-k-mooney | so you think the best approch woudl be to start creating new geneic mdev tests in tempest right | |
| 14:20:32 | bauzas | and yeah I have a 2-node env with one mtty, can try to migrate my test instance | |
| 14:20:42 | sean-k-mooney | cool | |
| 14:20:56 | dansmith | sean-k-mooney: depends on what they look like really, but.. yeah why not? | |
| 14:20:59 | sean-k-mooney | cold migrate should work live migrate will likely fail or we/libvirt have a bug | |
| 14:21:01 | bauzas | back to dansmith's point about status checks | |
| 14:21:06 | dansmith | this is clearly a gap | |
| 14:21:37 | sean-k-mooney | dansmith: no reason not to just wanted to confirm that is your prefernce | |
| 14:22:04 | bauzas | dansmith: so, about status checks, I'd say revert it | |
| 14:22:05 | sean-k-mooney | so we will need a new compute feature config option in tempest for it and perhasp a cofnig option to identify how to detect the device in the vm | |
| 14:22:16 | dansmith | sean-k-mooney: yeah | |
| 14:22:23 | bauzas | no real gain in having a warning imho | |
| 14:22:31 | dansmith | bauzas: okay that was sean-k-mooney's opinion too | |
| 14:22:46 | dansmith | bauzas: so revert in master, backport that, and then maybe add it back later when we start depending on it? | |
| 14:22:54 | bauzas | wfm | |
| 14:23:06 | dansmith | aight | |
| 14:23:23 | sean-k-mooney | ya so i think that would make sense | |
| 14:23:32 | sean-k-mooney | fell like pushing the revert patch | |
| 14:23:50 | sean-k-mooney | and we can review it quickly since we are talking about it | |
| 14:24:42 | dansmith | I'm doing it | |
| 14:24:45 | dansmith | I need to find that bug tho | |
| 14:25:50 | opendevreview | Dan Smith proposed openstack/nova master: Revert "Add upgrade check for compute-object-ids linkage" https://review.opendev.org/c/openstack/nova/+/898741 | |
| 14:28:57 | bauzas | hmmm, looking more at the mdev parent being 'computer' in the device XML, there may be some confusions when creating inventories if we were about asking two different virtual devices, like say mtty and mdpy | |
| 14:29:14 | bauzas | that's a potential bug, so I'm pdb'ing it on my devstack | |
| 14:29:49 | bauzas | another slight difference from libvirt that may need to change on their side | |
| 14:51:54 | sean-k-mooney | well we can have mulitpel inventories of mdevs for diffent types | |
| 14:52:31 | sean-k-mooney | so it will be a slightly didfdfent toplogy i.e. 1 rp for all virtual devs rahter then one per pareent devce | |
| 14:52:34 | sean-k-mooney | but i think that is fine | |