| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 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 | kashyap | stephenfin: Ah, I didn't get tot he _check-secure_boot_support() method yet; looks good | |
| 14:09:40 | stephenfin | if we didn't have that, we'd attempt to enable it and fail | |
| 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? | |
| 14:23:08 | bauzas | sean-k-mooney: vDPA series or the policy one ? | |
| 14:23:28 | bauzas | gibi: nothing crucial for your eyes ? | |
| 14:23:35 | stephenfin | gibi: plus a week for them to actually get through the gate /o\ | |
| 14:23:40 | stephenfin | sigh | |
| 14:23:49 | stephenfin | it really hates my db migration series | |
| 14:23:57 | bauzas | stephenfin: well, no worries if they are accepted before FF | |
| 14:24:07 | bauzas | if they are in the gate, we can recheck if needed | |
| 14:24:18 | artom | stephenfin, sean-k-mooney, so one thing that hadn't occurred to me what we could do is still store something in self (let's say self.numa_topology for now), but access it from consume_requests() and supports_requests(), which are already at the object level, and pass it to the _filter() methods, and we can keep them classmethods in that way | |
| 14:24:19 | bauzas | even if we're past the feature freeze | |
| 14:25:15 | artom | But to be honest, I'm burnt out on the thing. It's good enough as is, I'll remove my -1, merge, and that way folks are free to concentrate on other stuff before FF | |
| 14:25:24 | artom | Instead of endlessly revisiting this | |
| 14:27:31 | stephenfin | ack | |
| 14:27:59 | sean-k-mooney | artom: ya that would have worked. | |
| 14:27:59 | gibi | stephenfin: I will recheck the db series into W or at least "die" trying :) | |
| 14:28:01 | openstackgerrit | Stephen Finucane proposed openstack/nova master: WIP: Change default policy for GET '/os-hypervisors' API https://review.opendev.org/c/openstack/nova/+/765798 | |
| 14:28:17 | sean-k-mooney | i have a ptg topic on reworking the pci handeling and fixing bugs | |
| 14:28:28 | artom | sean-k-mooney, you have *all* the PTG topics ;) | |
| 14:28:30 | sean-k-mooney | artom: lets think about it as part of that next cycle | |
| 14:28:34 | stephenfin | gibi: gmann: That's my approach to the 'os-hypervisors as project admin' change ^ | |
| 14:28:49 | stephenfin | gibi: gmann: tbh, for what it gives us I'm thinking it's less and less a good idea | |
| 14:28:49 | sean-k-mooney | artom: ya i know i feel bad for adding them but we might just skip some | |
| 14:29:12 | sean-k-mooney | artom: more a list of what i knwo we shoudl addrss right now then what we plann on adressing next cycle | |
| 14:29:38 | stephenfin | bauzas: ta for the reviews, btw :) | |
| 14:29:45 | gibi | artom: as I said follow up work for me for the refactor. Do we know that socket - numa cardinality issue on the gate is just gate test node specific and fixebale later? | |
| 14:29:56 | bauzas | stephenfin: sorry for stopping but I want to zap here | |
| 14:30:01 | sean-k-mooney | artom: i plan to stick through or otherwise note which one wont be worked on in xena after we discuss downstream what we have capsity to work on | |
| 14:30:13 | bauzas | so, which one to pick ? | |
| 14:30:14 | artom | gibi, the thing stephenfin pointed out? It's normal, our nodepool VMs are set up that way | |
| 14:30:16 | stephenfin | bauzas: all good. chances are something will fail in the gate so no point bulk approving them | |
| 14:30:29 | artom | gibi, the new code correctly noped out when it saw there were multiple sockets per NUMA node | |
| 14:30:45 | bauzas | stephenfin: that's the usual game | |
| 14:31:14 | gibi | artom: does it also mean that we cannot test this now on the gate in tempest? | |
| 14:31:17 | bauzas | gibi: again, nothing to point out specifically ? | |
| 14:31:17 | sean-k-mooney | gibi: it can happen on really old hardware with a frontside bus. its unlkely to happen on any moderne (made in the last 12 years) hardware | |
| 14:31:32 | artom | gibi, I mean we never could we don't have PCI devices :) | |
| 14:31:32 | bauzas | if not, I'll choose sean-k-mooney's vDPA stuff as it's HPC | |
| 14:31:33 | gibi | sean-k-mooney: thanks, so it is just gate env issue, cool | |
| 14:31:38 | sean-k-mooney | gibi: multipl socket per numa ndoe imples the memory contoler was on the motherboard not the cpu | |
| 14:31:41 | gibi | artom: correct, sorry :) | |