| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-10-06 | |||
| 14:07:15 | bauzas | maybe this is just a codium issue | |
| 14:07:24 | sean-k-mooney | ya maybe | |
| 14:07:40 | bauzas | but everytime I need to doublecheck what I commit, that's super cumbersome | |
| 14:07:45 | sean-k-mooney | you did have an issue with codium adding a nobreaking space at one point | |
| 14:08:31 | bauzas | honestly, I need time to investigate what's wrong but I'm rushing into delivering a fix for https://bugs.launchpad.net/nova/+bug/2125935 | |
| 14:09:15 | bauzas | so I'll check that later | |
| 14:09:26 | sean-k-mooney | ack | |
| 14:41:07 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| 14:41:07 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 14:41:08 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 14:42:02 | gibi | dansmith: this is how unit test for the new product algo will look like https://review.opendev.org/c/openstack/placement/+/962776/5 | |
| 14:48:25 | gibi | https://review.opendev.org/c/openstack/placement/+/962776/6/placement/tests/unit/objects/test_allocation_candidate.py | |
| 14:48:27 | dansmith | gibi: okay.. that will require some study as well | |
| 14:49:34 | gibi | no worries. I have plenty of quirks to debug now that the unit test show some strange steps the algo makes | |
| 14:56:08 | dansmith | gibi: so all those "bug duplicate" ones are things it should have filtered? | |
| 15:03:47 | gibi | those are steps in the algo that should not be made. (I guess we have a off by one index error somewhere) | |
| 15:04:16 | gibi | somehow we check the same product 3 times instead of one | |
| 15:04:54 | gibi | I will figure it out why... | |
| 15:05:07 | gibi | don't hang up on that now. | |
| 15:05:28 | gibi | but the test already shows where we skip products so that is what is interesting | |
| 15:05:31 | gibi | for review | |
| 15:05:41 | opendevreview | Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156 | |
| 15:08:04 | dansmith | gibi: yeah, when I first skimmed it I thought you were identifying the things that would be skipped by using yours over the standard product, so just wanted to confirm that it was actually further skipping that needed to happen | |
| 15:08:10 | dansmith | I'm deeper into it now so it's obvious | |
| 15:20:25 | dansmith | gibi: can you respond here: https://review.opendev.org/c/openstack/placement/+/962776/comment/05b0ce6d_48710288/ ? | |
| 15:20:48 | dansmith | I'm trying to grok the test first, so maybe I'm being stupid | |
| 15:26:42 | bauzas | dansmith: fwiw, fill_metadata() seems non deterministic when you use a wrong context | |
| 15:27:29 | bauzas | ie. in my reproducer, some host with two instances has one with metadata set while the other not | |
| 15:28:05 | dansmith | bauzas: I dunno why you think that's possible :D | |
| 15:28:14 | bauzas | I know my patch fixes the whole problem but I'm puzzled why I'm still getting metadata only for one (and that could explain why your UT didn't catch the failure) | |
| 15:28:22 | dansmith | bauzas: I think it's more likely that you're using a shared context and it's being mutated in a way that seems like it's changing | |
| 15:28:33 | bauzas | yeah, that's what I thought | |
| 15:29:01 | bauzas | I'm passing the same context object for both instances so this probably was mutate | |
| 15:29:04 | bauzas | mutated | |
| 15:29:45 | dansmith | the instances are from the same cell right? | |
| 15:30:13 | opendevreview | Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156 | |
| 15:31:29 | opendevreview | Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156 | |
| 15:33:03 | gibi | dansmith: responded | |
| 15:33:35 | gibi | the mock is on the consolidate call not on the exceeds_capacity call | |
| 15:34:07 | gibi | so we assert whant product or partial products are generated and tested by the algo, not what is eventually returned by the product algo. I can add the later to the test as well | |
| 15:34:31 | gibi | s/later/latter/ | |
| 15:35:33 | dansmith | oh, right, so for that 1,2,1 case we would have seen a result of "nope not valid" from _consolidate and thus we would not have yielded it, right? | |
| 15:37:18 | gibi | yepp | |
| 15:37:57 | gibi | and we would also notice that 1,2,1 is an invalid prefix, so if there would be more groups, we would not test any of 1,2,1,x partial products | |
| 15:38:18 | gibi | as x can be any value the product would be invalid | |
| 15:38:20 | dansmith | yeah, sorry seems obvious now :P | |
| 15:39:24 | dansmith | okay so the solution is starting to come into view now.. the odometer is supposed to roll past a bunch of product "solutions" at the first index that isn't viable | |
| 15:39:33 | gibi | yepp | |
| 15:39:55 | gibi | (odometer or I learned during the weekend that it is called a mixed radix number in english that the odometer represents) | |
| 15:40:06 | dansmith | this is why I wanted to see a unit test to understand what the desired behavior was instead of trying to synthesize it in my head :) | |
| 15:41:02 | gibi | totally valid request to have unit tests | |
| 15:41:55 | dansmith | oh I know, I just mean.. I thought it would be easier to review the code with unit tests to show the behavior | |
| 15:42:18 | dansmith | that's not always the case, but here.. this is some complex stuff to reason about (until you get the idea in your head) | |
| 16:11:14 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| #openstack-nova - 2025-10-07 | |||
| 06:59:15 | jlejeune1 | gibi: hello, what do you think about: https://review.opendev.org/c/openstack/nova/+/963156 ? first requirement to fix bug 2085135 | |
| 08:19:16 | sthoffmann[m] | Hi, I wrote a RFE for how to wait for ovn-controller at live-migration https://bugs.launchpad.net/neutron/+bug/2069718. I included some feedback to the POC changes I created for that. Can you approve from nova side, that the RFE/blueprint is ok? | |
| 08:50:38 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| 08:50:38 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 08:50:39 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 09:58:03 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| 09:58:04 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 09:58:04 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 10:08:33 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| 10:08:33 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 10:08:34 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 10:23:14 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 10:23:14 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 11:51:07 | priteau | Hi Nova team! I have a trivial doc fix that has been open for a while: https://review.opendev.org/c/openstack/nova/+/955566 | |
| 12:00:27 | gibi | priteau: thanks. fast approved it | |
| 12:03:58 | priteau | thanks! | |
| 12:11:00 | opendevreview | Merged openstack/nova master: doc: Fix typo in nova-manage command https://review.opendev.org/c/openstack/nova/+/955566 | |
| 12:14:00 | opendevreview | Balazs Gibizer proposed openstack/placement master: Prune a_c search space by invalid prefixes https://review.opendev.org/c/openstack/placement/+/962776 | |
| 12:14:00 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 12:14:01 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline exceeds_capacity https://review.opendev.org/c/openstack/placement/+/963053 | |
| 12:34:04 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 12:34:57 | opendevreview | Balazs Gibizer proposed openstack/placement master: Inline _consolidate_allocation_requests https://review.opendev.org/c/openstack/placement/+/963052 | |
| 12:36:38 | gibi | dansmith: sean-k-mooney: ^^ the prune https://review.opendev.org/c/openstack/placement/+/962776 and the inlining of consolidation https://review.opendev.org/c/openstack/placement/+/963052 patches are ready for review from my perspective. I'm abandoning the 3rd patch https://review.opendev.org/c/openstack/placement/+/963053 as it does not significantly help | |
| 12:39:38 | gibi | I will add a 3rd patch on top with a reno | |
| 12:58:40 | opendevreview | Kamil Sambor proposed openstack/nova master: [hacking] Improve N373 to catch also other primitives https://review.opendev.org/c/openstack/nova/+/958408 | |
| 13:01:01 | opendevreview | Balazs Gibizer proposed openstack/placement master: Release notes for bug/2126751 https://review.opendev.org/c/openstack/placement/+/963275 | |
| 13:02:01 | sean-k-mooney | gibi: ack i see you chosse not to factor out the logic into a helper. that is fine we can mark that restolved. i think the fact that you are not just purly inline and are integrating it into the algorthim while graduclaly buildign the candiates is called out enough in the commit | |
| 13:02:23 | sean-k-mooney | ill try and make some time later to treview them | |
| 13:02:55 | sean-k-mooney | gibi: nit: wyou have whitespace issues in teh relese note patch | |
| 13:03:54 | opendevreview | Balazs Gibizer proposed openstack/nova master: DNM: test with placement fix https://review.opendev.org/c/openstack/nova/+/963035 | |
| 13:03:55 | opendevreview | Balazs Gibizer proposed openstack/nova master: DNM:Test with optimize_for_wide_provider_trees https://review.opendev.org/c/openstack/nova/+/963036 | |
| 13:11:36 | gibi | sean-k-mooney: ack I will fix them | |
| 13:16:14 | opendevreview | Balazs Gibizer proposed openstack/nova master: [doc]PCI in Placement tuning https://review.opendev.org/c/openstack/nova/+/963281 | |
| 13:52:34 | dansmith | gibi: what was the fix for not generating the extra duplicate matches? | |
| 13:54:15 | opendevreview | Balazs Gibizer proposed openstack/nova-specs master: Placement child resource provider usage in allocation-candidates https://review.opendev.org/c/openstack/nova-specs/+/938070 | |
| 13:55:27 | dansmith | oh was it the deepcopy in the test itself maybe? | |
| 13:55:31 | opendevreview | Balazs Gibizer proposed openstack/nova-specs master: Placement child resource provider usage in allocation-candidates https://review.opendev.org/c/openstack/nova-specs/+/938070 | |
| 13:56:08 | gibi | dansmith: I needed to copy the call args in the test as they are lists that are mutated between calls and the mock only stores the reference to the list | |
| 13:56:45 | dansmith | ack, so the code was doing the right thing, the test was just wrong.. I was hoping there was more optimization coming from the fix for that :) | |
| 13:56:53 | gibi | me too | |
| 13:56:53 | gibi | :) | |
| 13:58:01 | gibi | sean-k-mooney: dansmith: as you see the links above I pushed a nova doc change to link to the placement optimizations in the PCI in Placement doc. And I updated our backlog spec about the general allocation candidated explosioin problem with the new learning about the invalid candidates | |
| 13:58:22 | dansmith | ++ | |
| 14:01:36 | dansmith | gibi: some comments and a couple questions on the latest rev | |
| 14:01:57 | gibi | OK. I will check | |
| 14:01:59 | gibi | thanks | |