| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 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 | sean-k-mooney | standard is a device that does not supprot sriov | |
| 21:47:03 | artom | But it has no parent, is my point | |
| 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 | artom | Not that it as intentional | |
| 21:49:56 | sean-k-mooney | well you might be but its a noop for you | |
| 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 | Yeah, if I do for pool in pools: | |
| 21:51:56 | artom | if 'socket' in pool: | |
| 21:51:56 | artom | del pool['socket'] | |
| 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 | |
| 23:01:52 | dansmith | m'thanks | |
| 23:38:31 | openstackgerrit | sean mooney proposed openstack/nova master: Support per port numa policies with SR-IOV https://review.opendev.org/c/openstack/nova/+/773792 | |
| #openstack-nova - 2021-03-11 | |||
| 04:31:26 | openstackgerrit | melanie witt proposed openstack/nova master: Dynamically archive FK related records in archive_deleted_rows https://review.opendev.org/c/openstack/nova/+/773834 | |
| 05:37:12 | openstackgerrit | Merged openstack/nova master: libvirt: Deprecate disable_native_luksv1 and rbd_volume_local_attach https://review.opendev.org/c/openstack/nova/+/778004 | |
| 05:54:16 | openstackgerrit | Merged openstack/nova master: Follow up for QoS interface attach https://review.opendev.org/c/openstack/nova/+/777112 | |
| 06:42:58 | openstackgerrit | Merged openstack/nova master: setup.cfg: Resolve warning https://review.opendev.org/c/openstack/nova/+/779449 | |
| 06:57:40 | openstackgerrit | Merged openstack/nova master: Make nova-ceph-multistore use policy.yaml https://review.opendev.org/c/openstack/nova/+/779815 | |
| 07:46:27 | openstackgerrit | Yongli He proposed openstack/nova master: smartnic support - reject server move and suspend https://review.opendev.org/c/openstack/nova/+/779913 | |
| 07:50:03 | openstackgerrit | Yongli He proposed openstack/nova master: smartnic support - reject server move and suspend https://review.opendev.org/c/openstack/nova/+/779913 | |
| 07:52:49 | openstackgerrit | Yongli He proposed openstack/nova master: smartnic support - reject server move and suspend https://review.opendev.org/c/openstack/nova/+/779913 | |
| 08:05:31 | yonglihe | sean-k-mooney: alex_xu: could you please remove that -1 (base on discuss with alex): https://review.opendev.org/c/openstack/nova/+/771363 | |
| 08:20:21 | gibi | yonglihe: hi, sorry I was not able to get back to the smartnic patches yesterday | |
| 08:26:05 | yonglihe | gibi: got, busy day. | |
| 08:26:48 | gibi | yonglihe: I see -1s on https://review.opendev.org/c/openstack/nova/+/771363/12 does the problem resolved? | |
| 08:27:18 | yonglihe | I post another patch to fix alex coments. | |
| 08:27:38 | yonglihe | and there are one unit test case will posted soon. | |