| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-22 | |||
| 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 | |
| 18:08:49 | artom | So https://review.opendev.org/#/c/304511/, which depends on MAC address global uniqueness (or at least per-instance uniqueness), merged the same say as https://review.opendev.org/#/c/336069/, which removed said uniquenss | |
| 18:12:37 | mriedem | that's from newton | |
| 18:12:59 | artom | I know, I'm just going back in time and realizing that device tagging never fully worked | |
| 18:13:15 | dansmith | WAT | |
| 18:13:18 | artom | Because if you had a VM with 2 identially-MAC'ed NICs, device tags could get switched around | |
| 18:15:08 | artom | Most people probably have autogenerated MACs, so the likelihood of collisions is pretty low (made apparent by this not having been reported in 3 years), but still technically broken | |
| 18:16:49 | dansmith | ah well, that's less concerning to me I think | |
| 18:17:02 | dansmith | a single machine with two identical macs on different networks would be confusing to the guest OS as well | |
| 18:27:17 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Device tags: don't pass pf_interface=True to get_mac_by_pci_address https://review.opendev.org/670593 | |
| 18:27:17 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: WIP: Device tagging: expose target PCI address, not source https://review.opendev.org/672127 | |
| 18:27:28 | artom | mriedem, sean-k-mooney ^^ | |
| 18:28:55 | artom | (Thanks for hitting those backports!) | |
| 18:29:18 | mriedem | you didn't really need to rebase this... https://review.opendev.org/#/c/670593/1..2 | |
| 18:34:07 | sean-k-mooney | dansmith: i think the usecase for identical mac on different networks had something to do with bonds/loadbalancing but its limited scope for when it actully makes sense | |
| 18:34:50 | dansmith | sean-k-mooney: that only makes sense if you've got upstream filtering by the network or something.. normally boding two nics in linux uses two macs and the bonding agent chooses one | |