Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-05-08
11:34:13 ishanwar[m] <ishanwar[m]> "sean-k-mooney: made a few..." <- Build has passed as well. I think it looks good now
11:37:08 sean-k-mooney ishanwar[m]: i ment to push my comment a while ago but its going to fail our code style checks
11:37:53 sean-k-mooney ishanwar[m]: i got pinged for soemthign else so i didnt finish the review but i pushed my comment about the whitespace sissue
11:37:58 ishanwar[m] sorry new to this
11:37:58 ishanwar[m] sean-k-mooney: pep checks seem to have passed though
11:38:01 sean-k-mooney i think i agree that this is good to go
11:38:31 sean-k-mooney you shoudl stil fix https://review.opendev.org/c/openstack/nova/+/945204/6/nova/tests/unit/network/test_neutron.py#4101
11:38:49 sean-k-mooney but i think the logic is correct
11:39:31 ishanwar[m] i'll ping back
11:39:31 ishanwar[m] let me fix the style
11:39:31 ishanwar[m] sean-k-mooney: yeah I have double checked
11:40:53 opendevreview Stephen Finucane proposed openstack/nova master: wsgi: Don't create, use lock in same line https://review.opendev.org/c/openstack/nova/+/949161
11:45:00 sean-k-mooney ishanwar[m]: so running ti locally it fails both the trim trailign spaces check adn the lighleght check when i run tox -e pep8
11:45:33 sean-k-mooney ishanwar[m]: but it also fixes if for you
11:46:14 sean-k-mooney well it fixes the white space you will need to manually fix the linelenght issue
11:53:29 gibi sean-k-mooney: I think if the groups are coming from the flavor then we can simply ask the flavor creator to add policy according to the needs. I.e. if they want two vgpus from two different pgpus and they won't use qos ports then it is a case when isolate policy is needed. For the neutorn port case we need a defaulting as the person creating the port might not be the same as the one creating the
11:53:35 gibi flavor and we dont support adding policy to the port
11:57:23 sean-k-mooney gibi: yes im aware
11:57:30 sean-k-mooney we currently require you to set it
11:57:43 sean-k-mooney but i think it woudl be a better ux if we jsut defautl to none
11:58:01 sean-k-mooney partly because i honestly think we shoudl delete the suprpot for group_polcy in the flavor in its currnt form
11:58:23 sean-k-mooney i know its possibel to use isloate i fyou know exactly how the flavor will be used
11:59:07 sean-k-mooney i.e. if you have a flavor for a specific aplication that that will only be used with prots that either dont hav eresouce request or wont be fullfield form the same rp
11:59:45 sean-k-mooney but i think adding it to the flavor in its current form was a mistake so if we can avoid admin ever settign it by defaulting to None
11:59:53 sean-k-mooney it hink that will server better longterm
12:00:31 sean-k-mooney gibi: there is a similar but diffent issue with it too in that it breakes our filter because it not namespaced
12:00:49 sean-k-mooney stephen and i fixed that in train for the only other extra spec that did not have a namespace
12:01:23 sean-k-mooney we created a namespaced version as an alias and maded the non name "deprecated"
12:01:58 sean-k-mooney i think we shoudl do the same here and add group:policy=isolate|none
12:02:32 sean-k-mooney but again that for a slightly diffent reasons
12:03:23 sean-k-mooney until we can supprot expressign the policy betwen sepcific groups to me isolate is not practiclly usable unless you a nova/placment core or have spent a very very long time lookign at all the details
12:04:00 sean-k-mooney so im wondering if we can take some fo the sharp edges off in a backwards compatible way
12:04:34 sean-k-mooney by 1 defaulting to none in all cases if not set, and 2 providing the namespaced version to fix the filters
12:53:26 gibi I won't block the defaulting patch.
12:53:39 gibi I'm OK to fix the namespaceing
12:54:27 gibi I have no capacity to dig deeper in the discussion, but I'm happy to review the namespacing fix
13:17:54 sean-k-mooney gibi: thanks. i just want to get some early feedback on direction. i will try and find time to file the relevent bug tracker and work on some kind of poc of this for wider dicussion
14:27:38 gibi thanks
14:50:38 gibi dansmith: sean-k-mooney: FYI: here are the pre-fork executor cleanup change https://review.opendev.org/c/openstack/nova/+/947966/9/nova/cmd/scheduler.py it is cleaner in my eyes. It works well in my local non testing and on the gate (CI result is in the nova-next job of the last patch in the series)
14:51:10 dansmith nice
14:52:15 gibi oslo also did some changes based on our feedback about the fork issue but I haven't been able to check it out yet.
15:00:57 gmaan melwitt: thanks. ++
15:09:13 opendevreview Merged openstack/nova-specs master: Add reminder about Tempest API response schemas https://review.opendev.org/c/openstack/nova-specs/+/949113
15:20:19 sean-k-mooney gibi: how woudl you feall about getign rid of the glbal and doign this instead https://review.opendev.org/c/openstack/nova/+/947966/comment/dd7fc7e3_f4e926fb/
15:20:49 sean-k-mooney gibi: and yes teh reset looks cleaner then i was expecting too so +1 on that
15:23:46 sean-k-mooney gibi: by the way we likely should modify our base testcase to ensure we shutdown teh threadpool and reset it between test runs
15:24:28 sean-k-mooney i belive i had that in my verison ot prevent leakign thread betwen tests
15:24:39 sean-k-mooney if we ever do leak a thread in the pool it should fail the test
15:24:55 sean-k-mooney the same way our greenthread leak proctection works
15:26:14 sean-k-mooney in the test we can kind of get awar with it by using the synconousthreadpool via a fixture too
15:27:34 sean-k-mooney but we do have fixture to mange this currently https://github.com/openstack/nova/blob/master/nova/test.py#L197 https://github.com/openstack/nova/blob/master/nova/test.py#L324-L329
15:27:55 sean-k-mooney so we likely need to extend this to the new scather gather pool
15:29:19 sean-k-mooney i made the SynchronousThreadPoolExecutorFixture also apply to the futurist.ThreadPoolExecutor https://review.opendev.org/c/openstack/nova/+/922497/5/nova/tests/fixtures/nova.py#1254
15:33:10 sean-k-mooney gibi: bug i think you shoudl also update https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L1191-L1247 and https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L2056 or add new ones for this thread pool
16:59:55 gmaan sean-k-mooney: RE on group_policy, I think namespace is good idea, not sure why it was left in initial effort.
17:01:05 gmaan sean-k-mooney: about changing default to None. is not it None as default in ResourceRequest object and as you mentioned it is set 'None' explicitly if two group from different than flavor source
17:01:47 gmaan I mean just checking what is value if it is not passed and groups from cmg from flavor only
17:02:50 sean-k-mooney gmaan: right i know we set it if we get groups form 2 souces
17:03:06 sean-k-mooney im jsut suggesting updating that logic ot set it if we ever have 2 groups and its not set
17:03:23 sean-k-mooney so if you set it in the flavor that woudl alays overwrite it
17:04:06 gmaan sean-k-mooney: yeah, I agree on that. I am just saying we might be doing the same currently (not in flavor but somewhere in request spec/ResourceRequest )?
17:04:56 sean-k-mooney gmaan: not based on gorkas testing of tryign to use 2 groups to ask for two vgpus
17:05:23 sean-k-mooney but maybe there are some code paths where we will
17:05:47 gmaan humm, and is it set 'isolated' in that case?
17:05:56 sean-k-mooney no its not pass to placement at all
17:06:03 gmaan ohk
17:06:03 sean-k-mooney and then placement raises an error i belive
17:06:23 sean-k-mooney i can ask for some logs or access to there enve where they hit this
17:06:35 sean-k-mooney we likely can create a repoducer
17:07:46 gmaan I think I agree with your proposal. As 'none' means request can be satisfied by different or same providers. if not passed, setting it default to None in flavor make sense. I do not think it change behavior from user perspective.
17:08:24 gmaan we are just adding default here and not actually *changing* the default
17:08:44 sean-k-mooney correct that is what it means
17:09:16 sean-k-mooney gmaan: at least based on what we have documetned here https://docs.openstack.org/nova/latest/user/flavors.html
17:09:52 gmaan yeah, I do not think we need microversion in that case, we can fix it as bug fix
17:10:47 opendevreview Ishan Shanware proposed openstack/nova master: [FEAT]: using quota_details to optimize validate networks [FIX]: added error handling for .get() statements [FIX]: pep8 formatting fixed for changes [FIX]: added additional unit tests changes to mock validate_networks [FIX]: fixes using autopep8 https://review.opendev.org/c/openstack/nova/+/949235
17:10:57 sean-k-mooney thansk for confirming. as i said above i didnt like the idea of a server create failign with a given flavor based on the microversion used
17:10:59 sean-k-mooney or resize
17:11:15 sean-k-mooney that just seam very confussing for the end user to reason about
17:12:43 gmaan yeah
17:13:20 sean-k-mooney gmaan: there si already a bug for the filter part https://bugs.launchpad.net/nova/+bug/2048874 so ill likely resue that
17:14:27 sean-k-mooney ill proably just take over the orgianl authors patch and try an dalign it to what we have dicussed https://review.opendev.org/c/openstack/nova/+/905210
17:14:57 sean-k-mooney they were orginall just oging to skip that key which may have worked but its nto really fixign the problem
17:15:45 sean-k-mooney although i might just reuse the bug and not the patch since the logic of the fix will end up beign differnt
17:17:25 sean-k-mooney while i have this open https://review.opendev.org/c/openstack/nova/+/947541 is a nice small fix form melwitt that might be nice ot land if folks agree
17:19:26 melwitt sean-k-mooney: thanks for reviewing that :) I am happy to respin with code comment + release note btw
17:20:00 sean-k-mooney frickler: fungi: do either of ye op access on the channel it might be nice to update the "development-planning: https://etherpad.opendev.org/p/nova-2025.1-status" to "development-planning: https://etherpad.opendev.org/p/nova-2025.2-status" in the topic if Uggla agrees. i think the rest of the topic looks correct
17:20:25 melwitt the above uploaded patch in the backscroll (https://review.opendev.org/c/openstack/nova/+/949235) looks like one of the garbage AI ones
17:20:27 sean-k-mooney melwitt: i got asked about that issue downstream, found the bug and was like oh melwitt has a fix up
17:20:50 melwitt sean-k-mooney: haha fun
17:21:01 sean-k-mooney melwitt: i have been thating to the reporter a bit
17:21:17 sean-k-mooney not the reportor
17:21:31 sean-k-mooney the sumbitter of https://review.opendev.org/c/openstack/nova/+/949235
17:21:50 sean-k-mooney so they are not really familary with workign in openstack and i think git
17:22:06 sean-k-mooney but ya the commit message is not forllwoing or normal convetion
17:22:25 sean-k-mooney is more like what you do on mailing list where for each revision to expalin what you changed form the last one
17:22:25 melwitt yeah. it's such a tiny thing to fix I thought might as well + backport it. I agree with you that the long term thing will be to stop formatting eph disks
17:22:34 melwitt sean-k-mooney: the commit message claims stuff that isn't in the patch though, like what is that
17:22:46 melwitt huh ok
17:22:57 sean-k-mooney melwitt: so it explainging what has change over time
17:23:06 sean-k-mooney whic is fine in a mailing list btu weired in gerrit

Earlier   Later