| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 13:24:21 | sean-k-mooney | sicne one of the two will merge first | |
| 13:24:30 | sean-k-mooney | and the other need to be respun | |
| 13:24:46 | artom | sean-k-mooney, lemme drop the kids off | |
| 13:24:51 | artom | Back in ~1 hour | |
| 13:26:09 | sean-k-mooney | sure ill wait for stephenfin or gibi to comment and ill do whatever they say | |
| 13:26:36 | sean-k-mooney | if they say put user first ill rebase on top of https://review.opendev.org/c/openstack/nova/+/772779 | |
| 13:43:03 | stephenfin | sean-k-mooney: what's the question? | |
| 13:44:47 | sean-k-mooney | stephenfin: artoms socket patch conflict with both my port numa and vdpa patches so i thin we need to stack them | |
| 13:45:05 | sean-k-mooney | stephenfin: question is port numa first or socket | |
| 13:45:11 | sean-k-mooney | with vpda last | |
| 13:45:21 | stephenfin | socket | |
| 13:45:23 | stephenfin | it's closest to merge | |
| 13:45:27 | stephenfin | in fact it's approved | |
| 13:45:40 | sean-k-mooney | right but ig cant merge since it broken | |
| 13:46:03 | stephenfin | huh? | |
| 13:46:04 | sean-k-mooney | artom need to fix suport form multipel socekt on one numa node | |
| 13:46:17 | sean-k-mooney | by making it a list and taking the first socket | |
| 13:46:26 | sean-k-mooney | to make it work in the gate vms | |
| 13:46:31 | stephenfin | can't that be a follow-up? | |
| 13:46:48 | stephenfin | as it's an edge condition | |
| 13:46:54 | sean-k-mooney | i gues but i also am not happy with the numa topology object being passed as a copy to the pci tracker | |
| 13:46:59 | stephenfin | is there real hardware with that design, out of curiosity? | |
| 13:47:11 | sean-k-mooney | we should have passed a dict of numa:socket | |
| 13:47:16 | stephenfin | yeah, neither was I but I couldn't find a better way | |
| 13:47:21 | stephenfin | that not much better | |
| 13:47:30 | sean-k-mooney | well we should have passed it to the function | |
| 13:47:31 | stephenfin | it's still information that can get out of sync | |
| 13:47:39 | sean-k-mooney | that cant | |
| 13:47:57 | sean-k-mooney | we dont supprot memory hotplug on the compute host | |
| 13:48:00 | stephenfin | and the NUMATopology can, for what we need? | |
| 13:48:09 | sean-k-mooney | so the numa node to sockt mapping wont get out of daty | |
| 13:48:13 | stephenfin | *what we need it for there? | |
| 13:48:24 | sean-k-mooney | stephenfin: its a copy so the pinned cpus and mempages wil be out of date | |
| 13:48:34 | stephenfin | but we don't use that, right? | |
| 13:48:56 | stephenfin | so we're simply passing too much information, some of which will get out-of-date | |
| 13:48:59 | sean-k-mooney | right but we dont have any comment stating that it will be incorrect and if we started trying to use it it could break | |
| 13:49:07 | sean-k-mooney | stephenfin: yep | |
| 13:49:14 | stephenfin | okay, fair | |
| 13:49:33 | stephenfin | I'm happy to look at alternatives so | |
| 13:49:47 | sean-k-mooney | i would have prefered not to store it in the class too and pass it to the filter evne if that involed passing it donw the layres but the too mugh datat it my main concnern | |
| 13:49:48 | stephenfin | but I wouldn't block on it, because of how close feature freeze is | |
| 13:50:07 | stephenfin | yeah, artom and I went over that in the review | |
| 13:50:12 | sean-k-mooney | ya so we coudl reduce the amount of data we pass in a folowup | |
| 13:50:20 | stephenfin | I believe him when he said it was too ugly | |
| 13:50:46 | sean-k-mooney | i just dislike the fact i need to change my mental model | |
| 13:51:03 | sean-k-mooney | the filter were not ment to use any data not passed into them | |
| 13:51:14 | sean-k-mooney | which is why they were class metods | |
| 13:51:17 | sean-k-mooney | to signal that | |
| 13:51:23 | stephenfin | did you see my other idea? | |
| 13:51:28 | stephenfin | from https://review.opendev.org/c/openstack/nova/+/774149/12 | |
| 13:51:33 | stephenfin | "Spitballing. How about adding 'host_socket' and maybe 'host_cell' attributes to 'InstanceNUMACell'? The latter isn't really necessary, since we use 'id' for that right now, but it would let us decouple this in the future (it's a confusing design, IMO). That former would let us avoid passing host NUMA information through to the PCI manager." | |
| 13:51:42 | sean-k-mooney | nope i only saw this after the patches were merged | |
| 13:52:04 | stephenfin | Well then, thoughts on that? Unnecessary duplication or potentially useful? | |
| 13:52:35 | stephenfin | host_cell should probably read host_node too | |
| 13:52:37 | sean-k-mooney | well i orginally wanted to put the socket in the extra_info dict in the pcidevice | |
| 13:52:54 | sean-k-mooney | so we did not need the lookup | |
| 13:53:02 | sean-k-mooney | but let me read that a few times | |
| 13:53:57 | sean-k-mooney | stephenfin: am adding host_socket to instanceNumaCell wont help unfortunetly | |
| 13:54:09 | sean-k-mooney | stephenfin: we dont have the socket of the pci device | |
| 13:54:25 | sean-k-mooney | if we did that and we added host_socket to the pci extra info then yes | |
| 13:54:39 | yonglihe | sean-k-mooney, alex_xu gibi: thanks, guys. | |
| 13:54:45 | stephenfin | oh, right | |
| 13:54:50 | stephenfin | darn | |
| 13:55:16 | stephenfin | then nvm | |
| 13:55:37 | sean-k-mooney | so what artom has merged in the first to pathces works its just got sharp edges | |
| 13:56:02 | sean-k-mooney | and if we reduced it to a dict of numa node to list of sockets | |
| 13:56:19 | sean-k-mooney | and then jsut did mapping[cell.id][0] | |
| 13:56:28 | sean-k-mooney | i think that would be enough | |
| 13:56:42 | sean-k-mooney | and we coudl do that in a follow up | |
| 13:57:24 | sean-k-mooney | so add a properyt or function to the host numa object to return that mappign dict and pass that in to the pci_tracker when we contuct it | |
| 13:57:48 | sean-k-mooney | that also get rid of some of the nested loops | |
| 13:58:24 | sean-k-mooney | stephenfin: ill rebase my port patch on his then but what do you think of ^ | |
| 13:58:42 | stephenfin | wfm | |
| 13:58:51 | stephenfin | but as a follow-up, IMO | |
| 13:58:58 | stephenfin | just to de-risk the whole enterprise | |
| 13:59:11 | sean-k-mooney | sure | |
| 14:03:36 | kashyap | stephenfin: I'm looking at the UEFI code; I'm commenting there as I go along (to facilitate parallel processing). I see that you're already neck-deep in another discussion here; no rush | |
| 14:04:51 | kashyap | stephenfin: Another thing on my mind (maybe you've already done it somwhere, and I need to catch up): a "guard" / check that denies secure boot for non-q35 machine types -- it's a q35-only feature | |
| 14:06:00 | stephenfin | kashyap: I don't need to do that explicitly | |
| 14:06:10 | stephenfin | but I do, to be safe | |
| 14:06:42 | stephenfin | the firmware metadata files won't report support for pc-i440fx-* machine types with the secure-boot feature | |
| 14:06:54 | stephenfin | so we'll hit https://review.opendev.org/c/openstack/nova/+/779302/2/nova/virt/libvirt/host.py#1506 | |
| 14:07:07 | stephenfin | see also line 1499 in that | |
| 14:07:34 | stephenfin | one of the reasons I invested so much time in beefing up the FakeLibvirtFixture was to "automate" as much of this as possible | |
| 14:07:35 | kashyap | stephenfin: Indeed, the JSON files report, correctly so, only for pc-q35-* (ugly name; but it's QEMU's "historical decision) | |
| 14:07:45 | stephenfin | correct | |
| 14:08:07 | kashyap | stephenfin: I see. Yeah, I've seen your work fly-by on the test / fixtures stuff. Fine work | |
| 14:08:22 | stephenfin | with that being said https://review.opendev.org/c/openstack/nova/+/776681/7/nova/virt/libvirt/driver.py#5837 | |
| 14:08:37 | stephenfin | That's mostly for test purposes | |
| 14:09:09 | stephenfin | actually, no, I remember - it's so we can decided whether to enable secure boot or not in the 'optional' case | |
| 14:09:40 | stephenfin | if we didn't have that, we'd attempt to enable it and fail | |
| 14:09:40 | kashyap | stephenfin: Ah, I didn't get tot he _check-secure_boot_support() method yet; looks good | |
| 14:12:18 | kashyap | Yeah, we don't want to enable secure Boot by "default" (may people tend to opt out of the UEFI complexity, for good reasons) | |
| 14:14:14 | sean-k-mooney | stephenfin: artom summerised that here https://review.opendev.org/c/openstack/nova/+/772779/17#message-744974dfe27df3782bd2e6c066aa1b241fc7136a | |
| 14:14:28 | sean-k-mooney | ill be afk untill the top of the hour and ill rebase my patches then | |
| 14:14:56 | gibi | stephenfin, sean-k-mooney, artom: fixing the sharp edges in artoms socket policy series as a followup works for me. I can review both that and the port numa policy when they are put in order. The order does not matter to me I have the intention to push both through :) | |
| 14:16:28 | sean-k-mooney | :) | |
| 14:16:48 | gibi | and we have more than a day! :D | |
| 14:22:50 | bauzas | I'm switching away a bit from stephenfin's uefi secure boot series while 50% of the changes are now approved | |
| 14:22:57 | bauzas | who wants reviews here? | |