| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 21:36:34 | sean-k-mooney | then stop nova compute and libvirt fully and start them again | |
| 21:36:39 | artom | I didn't | |
| 21:37:06 | sean-k-mooney | when that was -1 it was failing i hadd to add pool_keys.pop('socket', None) | |
| 21:37:16 | sean-k-mooney | to find_pool | |
| 21:37:27 | sean-k-mooney | ill try removing that again | |
| 21:39:23 | sean-k-mooney | i can test it with pci alis too i guess i just need to spend some time to do it | |
| 21:40:51 | artom | That's the thing, I wasn't touching /sys, or indeed the PCI devices | |
| 21:41:13 | artom | Besides calculating the socket and saving it in pool['socket'] | |
| 21:41:29 | sean-k-mooney | ya well this is what is printign for the final resouce view | |
| 21:41:31 | sean-k-mooney | PciDevicePool(count=1,numa_node=0,product_id='10c9',tags={dev_type='type-PF',physical_network='public'},vendor_id='8086'), PciDevicePool(count=7,numa_node=0,product_id='10ca',tags={dev_type='type-VF',parent_ifname='eth1',physical_network='public'},vendor_id='8086')] | |
| 21:41:50 | sean-k-mooney | so the socket is not being added there incorerctly | |
| 21:43:02 | sean-k-mooney | a pic dev look more or less normal too | |
| 21:43:24 | sean-k-mooney | {"dev_id": "pci_0000_01_11_3", "address": "0000:01:11.3", "product_id": "10ca", "vendor_id": "8086", "numa_node": 0, "label": "label_8086_10ca", "dev_type": "type-VF", "parent_addr": "0000:01:00.1", "parent_ifname": "eth1", "capabilities": {"network": ["rx", "tx", "sg", "tso", "gso", "gro", "rxvlan", "txvlan", "txudptnl"]}} | |
| 21:45:02 | sean-k-mooney | strange it seam to be workign without the pop(socket ,none) | |
| 21:45:04 | artom | sean-k-mooney, hrmm, I really think it's the parent PF issue | |
| 21:45:10 | artom | It doesn't have the socket | |
| 21:45:40 | sean-k-mooney | ya maybe althou it shoudl | |
| 21:45:55 | sean-k-mooney | well in the same way the VF will | |
| 21:45:59 | artom | The logging I added to remove_device() and _find_pool() logs what I expect with your tests | |
| 21:46:05 | artom | But not with my functional tests | |
| 21:46:22 | artom | Meaning - it's never called, because in my func tests it only tests PFs (or "standard" PCI) | |
| 21:46:26 | artom | (Not sure which) | |
| 21:46:38 | artom | By testing with VFs and their parent PFs, you've uncovered an issue | |
| 21:46:52 | sean-k-mooney | standard is not a PF | |
| 21:47:03 | artom | But it has no parent, is my point | |
| 21:47:03 | sean-k-mooney | standard is a device that does not supprot sriov | |
| 21:47:09 | sean-k-mooney | a PF is type-PF | |
| 21:47:16 | sean-k-mooney | standard is type-PCI | |
| 21:47:36 | artom | The point is not the type, it's the parent device (or lack thereof) | |
| 21:48:26 | sean-k-mooney | correct you do not have to mark the parent as unavaible | |
| 21:48:30 | sean-k-mooney | as we do with VFs | |
| 21:48:49 | sean-k-mooney | or the child as unaviable with PFs | |
| 21:49:36 | sean-k-mooney | so you are not calling _handle_device_dependents | |
| 21:49:39 | sean-k-mooney | in your tests | |
| 21:49:48 | artom | Nope :) | |
| 21:49:56 | sean-k-mooney | well you might be but its a noop for you | |
| 21:49:56 | artom | Not that it as intentional | |
| 21:50:00 | artom | Or that I was aware it existed | |
| 21:50:26 | sean-k-mooney | you are calling it | |
| 21:50:51 | sean-k-mooney | but i does nothing for STANDARD/type-PCI | |
| 21:51:56 | artom | del pool['socket'] | |
| 21:51:56 | artom | if 'socket' in pool: | |
| 21:51:56 | artom | Yeah, if I do for pool in pools: | |
| 21:52:01 | artom | At the end of my new filter | |
| 21:52:05 | artom | It fixes your func tests | |
| 21:52:39 | artom | I guess you've made your point about being purely functional and not having side effects ;) | |
| 21:53:49 | sean-k-mooney | making it it a pure fucntion may or may not have caught it depending on if we wrote the test right | |
| 21:54:43 | artom | I think the "best" solution would be to not set pool['socket'] altogether, and come at this from another angle: figure out which NUMA nodes are allowed, and filter on those | |
| 21:54:53 | artom | No side effects | |
| 21:55:09 | sean-k-mooney | well you should not be setting pool sockets | |
| 21:55:25 | sean-k-mooney | and you dont need too if you did what was in my last comment | |
| 21:56:01 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/772779/17#message-744974dfe27df3782bd2e6c066aa1b241fc7136a | |
| 21:57:01 | sean-k-mooney | the body of your filter could be basically this right | |
| 21:57:03 | sean-k-mooney | vm_sockets = [mappings[cell.id] for cell in numa_cells] | |
| 21:57:05 | sean-k-mooney | pools_to_remove = [] | |
| 21:57:07 | sean-k-mooney | for pool in pools: | |
| 21:57:09 | sean-k-mooney | pool_socket = mappings.get(pool['numa_node']) | |
| 21:57:11 | sean-k-mooney | if not (pci_socket and pool_socket in vm_sockets): | |
| 21:57:13 | sean-k-mooney | pools_to_remove += pool | |
| 21:57:15 | sean-k-mooney | return [pool for pool in pools if pool not in pools_to_remove] | |
| 21:57:49 | artom | There are some unbound variables in there, so I can't quite grok it | |
| 21:57:56 | artom | But I can come up with something equivalent | |
| 21:58:16 | sean-k-mooney | where mappings is basically {numa_node:[socket_id,socket_id] | |
| 21:58:32 | sean-k-mooney | artom: mappings is the only one right | |
| 21:58:42 | artom | pci_socket as well | |
| 21:59:03 | sean-k-mooney | oh that is pool_socket | |
| 21:59:15 | sean-k-mooney | i forcot to rename it | |
| 22:00:27 | sean-k-mooney | artom: although maybe that shoudl be | |
| 22:00:57 | sean-k-mooney | if not pool_socket or pool_socket in vm_sockets: | |
| 22:01:13 | sean-k-mooney | for socket policy you dont allow device with no numa node correct | |
| 22:01:34 | sean-k-mooney | so that should be an or not an and | |
| 22:06:28 | artom | sean-k-mooney, well, I did a thing, and it made all tests pass. | |
| 22:06:36 | artom | It's brute force and dumb, but "easy" to understand | |
| 22:06:38 | artom | Shall I push? | |
| 22:06:40 | sean-k-mooney | artom: so i can "fix" this in my patch by adding pool_keys.pop('socket', None) to _find_pool | |
| 22:07:01 | artom | It'll kick my last patch out of the gate, and we'll have to get +2s and +As again tomorrow | |
| 22:07:48 | sean-k-mooney | i dont know its technically broken but you have a followup or i can fix it in my patch | |
| 22:08:00 | sean-k-mooney | we proably shoudl kick it out of the gate as much as i hate that | |
| 22:08:10 | artom | Yeah, it's the correct thing to do | |
| 22:08:21 | artom | As it, it's broken with any parent-having device | |
| 22:08:35 | sean-k-mooney | ya | |
| 22:09:09 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: pci: implement the 'socket' NUMA affinity policy https://review.opendev.org/c/openstack/nova/+/772779 | |
| 22:09:10 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Support per port numa policies with SR-IOV https://review.opendev.org/c/openstack/nova/+/773792 | |
| 22:09:17 | sean-k-mooney | so the minimal fix is just add one line pool_keys.pop('socket', None) to _find_pool | |
| 22:10:32 | sean-k-mooney | ah i see what you did | |
| 22:11:05 | sean-k-mooney | are you still ok with rewriting it again in the followup | |
| 22:11:23 | sean-k-mooney | that should work as is though | |
| 22:11:26 | sean-k-mooney | ill test it now | |
| 22:12:05 | artom | sean-k-mooney, yeah, we can improve in a follow up | |
| 22:12:15 | artom | For now, do the simple stupid thing that's easy to understand and review, and works ;) | |
| 22:15:33 | sean-k-mooney | ok i have the unit and funct test running for those locally now. im going to go get somethign to eat and when i get back ill test boot real vms and ill comment on the review | |
| 22:16:07 | sean-k-mooney | then ill rebase the vdpa change on top of both patches and test those | |
| 22:16:16 | artom | That's... optimistic ;) | |
| 22:16:36 | artom | Though I suppose if that was the only outstanding issue with the port NUMA policies, might as well aim for the moon ;) | |
| 22:17:27 | sean-k-mooney | i have some nits form stephen it looks like i mised but ya i think its the only issue | |
| 22:18:09 | sean-k-mooney | these https://review.opendev.org/c/openstack/nova/+/773792/11#message-8d43910738d79b4f530fd6b4176074ad6f53c543 | |
| 22:20:56 | openstackgerrit | Merged openstack/os-resource-classes master: Fix hacking min version to 3.0.1 https://review.opendev.org/c/openstack/os-resource-classes/+/727557 | |
| 22:58:54 | dansmith | melwitt: easy stats: https://review.opendev.org/c/openstack/nova/+/779815 | |
| 22:59:27 | dansmith | converting the single-line glance policy I write from old json to new yaml, so I can write to it easier from a child job to change other stuff | |
| 23:01:11 | melwitt | +2 | |