Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-10-06
12:15:52 sean-k-mooney that explains why it was not in github https://review.opendev.org/c/openstack/placement/+/962776/5/placement/objects/allocation_candidate.py#850
12:16:34 sean-k-mooney so really your folding the exceeds_capacity logic inter the generation
12:16:45 gibi yes
12:17:09 sean-k-mooney ok ya that make more sense as to why it would have a benifit
12:17:14 gibi because of the result of that check informs what part of the product space can be ignored
12:18:02 gibi i.e. if G1+G2 fails the check then G1+G2+Gx+...Gy will awas fail the check so they can be ignored
12:18:29 sean-k-mooney right so we might be able to not inline the fucntion if we return more info beyond True or False like the atribute that exceed capsity
12:19:51 sean-k-mooney i dont really object to the inlineing DRY is nice but im ok wiht specific code where we have a perfroamce need for it
12:20:24 sean-k-mooney im just wonderign if we can get the benift of a testibale function while also achiving the speed up if we adjust the return value to provdie more context
12:21:52 sean-k-mooney looking at https://github.com/openstack/placement/blob/master/placement/objects/research_context.py#L345
12:22:30 sean-k-mooney the behvior is differnt enough that i dont think it makes sense to reuse exceeds_capacity and modify it as i was suggesting
12:24:41 gibi I will think about making the code cleaner once I have pinned down the behavior with some unit test
12:25:47 sean-k-mooney ack i think we coudl factor https://review.opendev.org/c/openstack/placement/+/963052/1/placement/objects/allocation_candidate.py#746 out into its own helper if we wanted but having its own impleaiton i belive is resonable
13:26:13 opendevreview Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156
13:34:17 bauzas dansmith: remember when you asked me why my comments were usually having two spaces between the hashtag and the first word ? looks to me this is due to autopep8 which automatically 'fixes' my comments 🤦
13:34:43 dansmith bauzas: I don't, but.. good to know
13:38:37 bauzas still have to confirm, but unless I'm fool, I don't understand why those extra spaces suddently appear on my code while I doublechecked before committing I wasn't having them
13:39:02 bauzas so this is either autopep8 or vscode (was having the same problem before I started using Cursor)
13:39:23 bauzas I was becoming crazy tbh
13:40:12 bauzas I'll do further tests with --no-verify
13:40:21 bauzas so it would skip autopep8
14:03:15 sean-k-mooney bauzas: its from pep8 https://peps.python.org/pep-0008/#inline-comments
14:03:38 sean-k-mooney bauzas: "An inline comment is a comment on the same line as a statement. Inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space."
14:03:55 bauzas I'm not talking of inline statements sorry
14:04:19 sean-k-mooney oh ok. never mind then
14:04:26 bauzas just a whole comment line like # TODO(x) : This is a comment
14:04:57 sean-k-mooney ah well ther eshoudl be 1 space between the # and TODO
14:04:59 bauzas I'm writing "# TODO" and eventually it commits "#  TODO" with two spaces
14:05:23 sean-k-mooney hum that is odd indeed
14:05:44 sean-k-mooney i have not seen that but that could be the agressive option. although im not sure which guidelien would be influcancing that
14:06:16 sean-k-mooney the change to drop agressive=3 is still open and mergable so if ye want to proceed with that do
14:06:39 sean-k-mooney bauzas: do you happen to have a linke to the code that was formated?
14:07:04 bauzas that's always when I write a new comment line
14:07:11 sean-k-mooney i woudl be interested in seign the example. given we did not have to reformat any code when i enabeld that i was really not expectign it to have such an effect
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

Earlier   Later