| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-22 | |||
| 17:02:28 | stephenfin | mriedem: The zuul comments, yes. Third party CI are hidden | |
| 17:02:47 | stephenfin | I suspect that was intentional, since the button now reads "Toggle Extra CI" | |
| 17:07:28 | stephenfin | dansmith: Think you might be right, in which case this is much ado about nothing. I'll respond once I've made sure | |
| 17:07:36 | efried | artom: looks like mriedem is engaged with https://review.opendev.org/#/c/670593/ ? | |
| 17:07:53 | artom | efried, yep, thanks for looking | |
| 17:08:04 | dansmith | stephenfin: I didn't realize you were worried about something acutely happening now when we discussed earlier, I thought you were just generally wondering about the impact of moving away from those filters | |
| 17:08:09 | artom | and I think we're going back to drawing board for that one anyways | |
| 17:08:31 | efried | cdent: re shared disk spec, if it doesn't get some comeback from the author (or get picked up by someone else) then I guess it's dead, yeah. | |
| 17:08:36 | artom | The review process (sean-k-mooney mostly) unearthed other problems in that code | |
| 17:08:42 | stephenfin | No, it was pretty much the 'hw:emulator_threads_policy=isolate' case that I was worried about breaking | |
| 17:15:34 | sean-k-mooney | artom: my mother just arrived home but yes im still around | |
| 17:16:09 | openstackgerrit | Merged openstack/nova master: libvirt: move checking CONF.my_ip to init_host() https://review.opendev.org/671471 | |
| 17:16:28 | artom | sean-k-mooney, I thought I had a way of solving the tag <-> XML device problem for VFIO-PCI, but when writing it out in the review, turns out I didn't | |
| 17:16:45 | artom | I was thinking of using instance.pci_devices, but they don't have any MAC either | |
| 17:17:07 | artom | Unless we do a DB migration and start shoving them in there | |
| 17:17:35 | sean-k-mooney | you can use the vif['binding_profile']['pci_slot'] | |
| 17:17:48 | openstackgerrit | Andreas Jaeger proposed openstack/nova master: Update api-ref location https://review.opendev.org/672077 | |
| 17:18:11 | artom | sean-k-mooney, that won't solve the problem | |
| 17:18:12 | sean-k-mooney | that will be the host pci address for that port | |
| 17:21:47 | sean-k-mooney | artom: we can construct the NetworkInterfaceMetadata without setting the mac filed | |
| 17:21:55 | sean-k-mooney | or rather setting it condionally | |
| 17:22:19 | artom | sean-k-mooney, I know, but we need to correlate the tag in nova/objects/network_request.py to the correct device in the instance XML | |
| 17:22:36 | artom | Currently that's done by MAC address, because it's the one piece of info that's common (sometimes) to both | |
| 17:23:01 | sean-k-mooney | artom: but its not the "one" pices of info that is common | |
| 17:23:18 | sean-k-mooney | the pci address should be common too i belive | |
| 17:23:40 | artom | sean-k-mooney, right, but all of the existing code assumes the MAC will be used | |
| 17:24:11 | artom | So we write the tag in a VirtualInterface object, which has the MAC and nothing else. | |
| 17:24:46 | sean-k-mooney | well that is entirly broken | |
| 17:25:05 | artom | Now you tell me :P | |
| 17:25:06 | sean-k-mooney | you can have two ports with the same mac on different networks and attach them both to the same instance | |
| 17:25:36 | artom | Where were you 3 years ago or whenever Newton happened | |
| 17:25:38 | artom | ;) | |
| 17:26:14 | artom | See, I though Neutron placed a MAC uniqueness constraint per instance as well, not just per-Network | |
| 17:26:32 | sean-k-mooney | i proably pointed that out 3 years ago if i reviewd it :) | |
| 17:26:33 | sean-k-mooney | no | |
| 17:26:36 | sean-k-mooney | it does not | |
| 17:27:03 | sean-k-mooney | but nova proably does | |
| 17:27:15 | artom | I recall *something* of that sort | |
| 17:27:25 | sean-k-mooney | in anycase vifs_to_expose is a lis tof nova.model.Vif objects? | |
| 17:27:34 | artom | No, VirtualInterface | |
| 17:27:45 | sean-k-mooney | ah that was what i was about to ask | |
| 17:28:59 | sean-k-mooney | looking at the object https://github.com/openstack/nova/blob/master/nova/objects/virtual_interface.py#L34-L49 | |
| 17:29:04 | sean-k-mooney | you have the neutorn pot uuid | |
| 17:29:21 | sean-k-mooney | so you can get the full vifs form the network info cache | |
| 17:29:29 | sean-k-mooney | and then get the mac for that | |
| 17:30:49 | artom | We have the network ID, not the port | |
| 17:32:13 | sean-k-mooney | is this not the port id https://github.com/openstack/nova/blob/master/nova/objects/virtual_interface.py#L47 | |
| 17:32:29 | artom | NO that | |
| 17:32:36 | artom | That's the VIF uuid itself | |
| 17:33:25 | sean-k-mooney | with is the VIF uuid? | |
| 17:33:35 | sean-k-mooney | why would it need one? | |
| 17:33:52 | sean-k-mooney | also is this really the unique constarit https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L860-L861 | |
| 17:35:00 | artom | sean-k-mooney, IIRC there was something else higher up | |
| 17:35:05 | artom | (About the constraint) | |
| 17:35:15 | sean-k-mooney | well that constiarint is wrong | |
| 17:35:30 | sean-k-mooney | it would only allow 1 instnace of the mac in a cell | |
| 17:35:45 | sean-k-mooney | which is not required by neutorn | |
| 17:36:34 | artom | Makes device tagging mostly work though ;) | |
| 17:37:31 | sean-k-mooney | im not sure we want to fix all the issue here in one patch | |
| 17:37:50 | sean-k-mooney | we could but im not sure you want to have to write db migrations to fix this | |
| 17:39:34 | sean-k-mooney | the uniqui constratint should be (address,deleted,uuid) if uuid is the network uuid | |
| 17:40:02 | artom | sean-k-mooney, btw, just double checked, and the virtual_interface uuid does appear to be the interface UUID, not the port | |
| 17:40:38 | sean-k-mooney | the instance uuid should be in the instance_uuid field | |
| 17:40:48 | sean-k-mooney | not the uuid field | |
| 17:41:37 | artom | I mean the virtual_interface has its own UUID, much like an instance | |
| 17:41:45 | artom | Dunno why, but that's what it is | |
| 17:41:54 | sean-k-mooney | a ok | |
| 17:42:07 | artom | I checked on my devstack, the UUIDs don't match between ports and virtal_interfaces | |
| 17:42:21 | sean-k-mooney | in that case the constratin would be (address,deleted,network_id) | |
| 17:42:29 | sean-k-mooney | ok | |
| 17:42:53 | artom | I don't disagree, but it has the side effect of allowing device tagging to work, and it's kinda too late to impose it now | |
| 17:43:25 | sean-k-mooney | it a fairly sever regression | |
| 17:43:34 | sean-k-mooney | that was intoduec a long time agp | |
| 17:44:03 | sean-k-mooney | can you try and boot two vms with ports that have the same mac on different network and see what happens? | |
| 17:44:46 | sean-k-mooney | i need to run to the shop to grab food for dinner | |
| 17:45:14 | artom | Sure, thanks for the help so far | |
| 17:45:37 | sean-k-mooney | ill be back in an hour or so. for now i would suggest lets just fix the case we know is broken but should work | |
| 17:45:51 | artom | I suppose | |
| 17:45:54 | sean-k-mooney | we can try and figureout how to make vfio-pci work as a sperate patch | |
| 17:46:09 | artom | Yeah, incremental improvement and all that | |
| 17:46:45 | sean-k-mooney | yep. i also agree with mriedem it would be nice to add a functional test for this if we can. | |
| 17:47:17 | sean-k-mooney | anyway ill be back in about an hour and a half after dinner | |
| 17:47:19 | artom | That's always the problem with functional tests - unless you *know* all the possible things the hardware can do, they're mostly useless | |
| 17:47:32 | mriedem | i didn't necessary ask for a functional test, | |
| 17:47:41 | mriedem | anything in functional testing for that would have to be stubbed with fakelibvirt anyway | |
| 17:47:53 | artom | Otherwise you're essentially re-writing the same code, but shaped like a test | |
| 17:48:15 | mriedem | bbiab but i'll review https://review.opendev.org/#/c/667177/ when i get back | |
| 17:48:20 | artom | And clearly I *don't* know all the possible things the hardware can do, that's why we're here in the first place | |
| 17:55:09 | sean-k-mooney | ya i guess that is fair. i wont be back at my place in shannon until wednesday but i can look at recreating the vfio-pci case later this week or early next week. | |
| 17:56:16 | sean-k-mooney | i was assuming we would just extend https://github.com/openstack/nova/blob/master/nova/tests/functional/libvirt/test_pci_sriov_servers.py to check metadtaa was generated. | |
| 17:57:19 | artom | sean-k-mooney, for the record, 2 VMs with identical MACs on different networks (1 VM per network) work fine | |
| 17:59:10 | sean-k-mooney | cool the unique constraint musts be changed somewhere | |
| 17:59:24 | artom | ... and actually, 1 VM with 2 ports, same MAC on both but different networks, also works fine | |
| 17:59:37 | artom | Which is... worrying | |
| 18:01:50 | artom | Oh, the address is now being stored as MAC/port_uuid | |
| 18:01:54 | artom | This seems new... | |
| 18:07:44 | sean-k-mooney | oh right i rememebr that hack | |
| 18:07:48 | sean-k-mooney | ok that makes sense | |
| 18:08:43 | sean-k-mooney | we did it that way because nova networks required a unique mac | |
| 18:08:47 | sean-k-mooney | but neutron did not | |