| opendevreview | melanie witt proposed openstack/nova-specs master: Add reminder about Tempest API response schemas https://review.opendev.org/c/openstack/nova-specs/+/949113 | 02:28 |
|---|---|---|
| melwitt | Uggla, gmaan: this is a spec template update to add a note about API response schemas in Tempest ^ | 02:30 |
| melwitt | (fyi) | 02:30 |
| *** ralonsoh_out is now known as ralonsoh | 05:52 | |
| *** benj_6 is now known as benj_ | 06:35 | |
| 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 https://review.opendev.org/c/openstack/nova/+/945204 | 10:02 |
| ishanwar[m] | opendevreview: sean-k-mooney: made a few changes as your requests UTs seems to be working fine now kindly have a look | 10:04 |
| sean-k-mooney | gmaan: question for you https://docs.openstack.org/nova/latest/user/flavors.html | 11:20 |
| sean-k-mooney | we currently default group_policy to non if request groups come form neutron ports | 11:20 |
| sean-k-mooney | but we require group_policy to be passed if you pass multiple groups in the flavor | 11:21 |
| sean-k-mooney | it almost never makes sesne to use isolate because it will break port based request if you ever want more then one port form the same nic | 11:21 |
| sean-k-mooney | i.e. 2 ovs prot or 2 sriov ports | 11:21 |
| sean-k-mooney | so we never recommend using that in general | 11:21 |
| sean-k-mooney | so my question is if we jsut want to extend the defaulting to None to the falvor | 11:22 |
| sean-k-mooney | can tha tbe done without a microverison. i think the answer is know althoguh im hopeing i can argure that we coudl because we are relaxing the constraint | 11:22 |
| sean-k-mooney | its a trivial spec to write if we decied we need a microversion for tha that but honestly i thinik havign server create fail if a user uses the old microvtion with a flavor and pass with the new versiuon and exactly the same flavor is terrible ux | 11:29 |
| sean-k-mooney | we have previouysly said that new extra specs dont need a microvetion and that changing the default shoudl not either | 11:29 |
| sean-k-mooney | so i think we can do this withou one simiarl to the fix for neutron prots https://github.com/openstack/nova/commit/ad4f79836264ec79068f087008367e2d2ae8cbea | 11:30 |
| sean-k-mooney | gibi: ^ you wrote that for the neutron port qos policy i belive do you see any reaons not to extend the defauling to none ot flavor based groups too. | 11:31 |
| ishanwar[m] | <ishanwar[m]> "sean-k-mooney: made a few..." <- Build has passed as well. I think it looks good now | 11:34 |
| 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 |
| 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 |
| ishanwar[m] | sean-k-mooney: pep checks seem to have passed though | 11:37 |
| ishanwar[m] | sorry new to this | 11:37 |
| sean-k-mooney | i think i agree that this is good to go | 11:38 |
| 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 |
| sean-k-mooney | but i think the logic is correct | 11:38 |
| ishanwar[m] | sean-k-mooney: yeah I have double checked | 11:39 |
| ishanwar[m] | let me fix the style | 11:39 |
| ishanwar[m] | i'll ping back | 11:39 |
| 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:40 |
| 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 |
| sean-k-mooney | ishanwar[m]: but it also fixes if for you | 11:45 |
| sean-k-mooney | well it fixes the white space you will need to manually fix the linelenght issue | 11:46 |
| 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 |
| gibi | flavor and we dont support adding policy to the port | 11:53 |
| sean-k-mooney | gibi: yes im aware | 11:57 |
| sean-k-mooney | we currently require you to set it | 11:57 |
| sean-k-mooney | but i think it woudl be a better ux if we jsut defautl to none | 11:57 |
| 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 |
| sean-k-mooney | i know its possibel to use isloate i fyou know exactly how the flavor will be used | 11:58 |
| 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 |
| 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 |
| sean-k-mooney | it hink that will server better longterm | 11:59 |
| 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 |
| sean-k-mooney | stephen and i fixed that in train for the only other extra spec that did not have a namespace | 12:00 |
| sean-k-mooney | we created a namespaced version as an alias and maded the non name "deprecated" | 12:01 |
| sean-k-mooney | i think we shoudl do the same here and add group:policy=isolate|none | 12:01 |
| sean-k-mooney | but again that for a slightly diffent reasons | 12:02 |
| 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:03 |
| sean-k-mooney | so im wondering if we can take some fo the sharp edges off in a backwards compatible way | 12:04 |
| 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:04 |
| gibi | I won't block the defaulting patch. | 12:53 |
| gibi | I'm OK to fix the namespaceing | 12:53 |
| gibi | I have no capacity to dig deeper in the discussion, but I'm happy to review the namespacing fix | 12:54 |
| *** ykarel__ is now known as ykarel | 12:56 | |
| 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 | 13:17 |
| gibi | thanks | 14:27 |
| 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:50 |
| dansmith | nice | 14:51 |
| 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. | 14:52 |
| gmaan | melwitt: thanks. ++ | 15:00 |
| opendevreview | Merged openstack/nova-specs master: Add reminder about Tempest API response schemas https://review.opendev.org/c/openstack/nova-specs/+/949113 | 15:09 |
| 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 |
| sean-k-mooney | gibi: and yes teh reset looks cleaner then i was expecting too so +1 on that | 15:20 |
| 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:23 |
| sean-k-mooney | i belive i had that in my verison ot prevent leakign thread betwen tests | 15:24 |
| sean-k-mooney | if we ever do leak a thread in the pool it should fail the test | 15:24 |
| sean-k-mooney | the same way our greenthread leak proctection works | 15:24 |
| sean-k-mooney | in the test we can kind of get awar with it by using the synconousthreadpool via a fixture too | 15:26 |
| 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 |
| sean-k-mooney | so we likely need to extend this to the new scather gather pool | 15:27 |
| 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:29 |
| 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 | 15:33 |
| gmaan | sean-k-mooney: RE on group_policy, I think namespace is good idea, not sure why it was left in initial effort. | 16:59 |
| 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 |
| gmaan | I mean just checking what is value if it is not passed and groups from cmg from flavor only | 17:01 |
| sean-k-mooney | gmaan: right i know we set it if we get groups form 2 souces | 17:02 |
| sean-k-mooney | im jsut suggesting updating that logic ot set it if we ever have 2 groups and its not set | 17:03 |
| sean-k-mooney | so if you set it in the flavor that woudl alays overwrite it | 17:03 |
| 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 |
| sean-k-mooney | gmaan: not based on gorkas testing of tryign to use 2 groups to ask for two vgpus | 17:04 |
| sean-k-mooney | but maybe there are some code paths where we will | 17:05 |
| gmaan | humm, and is it set 'isolated' in that case? | 17:05 |
| sean-k-mooney | no its not pass to placement at all | 17:05 |
| sean-k-mooney | and then placement raises an error i belive | 17:06 |
| gmaan | ohk | 17:06 |
| sean-k-mooney | i can ask for some logs or access to there enve where they hit this | 17:06 |
| sean-k-mooney | we likely can create a repoducer | 17:06 |
| 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:07 |
| gmaan | we are just adding default here and not actually *changing* the default | 17:08 |
| sean-k-mooney | correct that is what it means | 17:08 |
| sean-k-mooney | gmaan: at least based on what we have documetned here https://docs.openstack.org/nova/latest/user/flavors.html | 17:09 |
| gmaan | yeah, I do not think we need microversion in that case, we can fix it as bug fix | 17:09 |
| 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 |
| 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 |
| sean-k-mooney | or resize | 17:10 |
| sean-k-mooney | that just seam very confussing for the end user to reason about | 17:11 |
| gmaan | yeah | 17:12 |
| 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:13 |
| 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 |
| sean-k-mooney | they were orginall just oging to skip that key which may have worked but its nto really fixign the problem | 17:14 |
| 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:15 |
| 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:17 |
| melwitt | sean-k-mooney: thanks for reviewing that :) I am happy to respin with code comment + release note btw | 17:19 |
| 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 |
| 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 |
| 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 |
| melwitt | sean-k-mooney: haha fun | 17:20 |
| sean-k-mooney | melwitt: i have been thating to the reporter a bit | 17:21 |
| sean-k-mooney | not the reportor | 17:21 |
| sean-k-mooney | the sumbitter of https://review.opendev.org/c/openstack/nova/+/949235 | 17:21 |
| sean-k-mooney | so they are not really familary with workign in openstack and i think git | 17:21 |
| sean-k-mooney | but ya the commit message is not forllwoing or normal convetion | 17:22 |
| 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 |
| 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 |
| melwitt | sean-k-mooney: the commit message claims stuff that isn't in the patch though, like what is that | 17:22 |
| melwitt | huh ok | 17:22 |
| sean-k-mooney | melwitt: so it explainging what has change over time | 17:22 |
| sean-k-mooney | whic is fine in a mailing list btu weired in gerrit | 17:23 |
| sean-k-mooney | also the lost the change id so https://review.opendev.org/c/openstack/nova/+/945204/6 | 17:23 |
| sean-k-mooney | is actully the review they shoudl of updated | 17:23 |
| 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/+/945204 | 17:23 |
| melwitt | I see | 17:23 |
| sean-k-mooney | ishanwar[m]: so you now have two reviews for the same thing because you lost the change id at one point | 17:24 |
| sean-k-mooney | ishanwar[m]: i think you ment to locally squash the two pathces before doing git review | 17:25 |
| 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/+/945204 | 17:25 |
| sean-k-mooney | ishanwar[m]: ok ^ is better but as i was dissusing with melwitt above you shoudl update the commit message to expalin what the current revion of the patch is changing not how the patch changed over time | 17:27 |
| 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/+/945204 | 17:27 |
| sean-k-mooney | ishanwar[m]: https://wiki.openstack.org/wiki/GitCommitMessages document the convetions we try to follow | 17:27 |
| fungi | sean-k-mooney: i can update it, though separately i recommend the nova team consider adding some channel operators at https://opendev.org/openstack/project-config/src/branch/master/accessbot/channels.yaml#L239 | 17:27 |
| 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/+/945204 | 17:28 |
| sean-k-mooney | fungi: oh i didnt know we could do that. i assuem we cant just update the topic via a git reivew? | 17:28 |
| sean-k-mooney | fungi: i htink we used to be able to do this in the chanel before we moved to oftc | 17:29 |
| 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/+/945204 | 17:29 |
| sean-k-mooney | but its been so long i kind of forget the detials | 17:29 |
| ishanwar[m] | sean-k-mooney: i have fixes the pep8 issue kindly have a look whenver you can | 17:30 |
| fungi | sean-k-mooney: you'd add people to an ops list for the channel in that file, then any of them can ask chanserv to set the channel topic like the example in https://docs.opendev.org/opendev/system-config/latest/irc.html#basic-channel-operator-commands | 17:30 |
| sean-k-mooney | fungi: ah cool ill add that to the irc metting adgenda for next week so | 17:31 |
| sean-k-mooney | and we can see what folks think | 17:31 |
| ishanwar[m] | sean-k-mooney: yep changing that | 17:31 |
| fungi | but also if Uggla wants the topic updated in the meantime, i'm happy to do that myself too | 17:31 |
| opendevreview | Ishan Shanware proposed openstack/nova master: [FEAT]: This feat optimises the validate_networks method by reducing networks calls to the database. Here we utilise the quota_details extension to gather the list of uses ports using the neutron extension, this is a smarter query as we don't have to list down all the ports which are in use. Instead we now utilise the quota details extension to get the count of used ports directly. ht | 17:34 |
| opendevreview | Ishan Shanware proposed openstack/nova master: [FEAT]: using quota_details to optimize validate networks https://review.opendev.org/c/openstack/nova/+/945204 | 17:39 |
| opendevreview | Ishan Shanware proposed openstack/nova master: [FEAT]: This feat optimises the validate_networks method by reducing networks calls to the database. Here we utilise the quota_details extension to gather the list of uses ports using the neutron extension, this is a smarter query as we don't have to list down all the ports which are in use. Instead we now utilise the quota details extension to get the count of used ports directly. ht | 17:40 |
| opendevreview | Ishan Shanware proposed openstack/nova master: [FEAT]: using quota_details to optimize validate networks https://review.opendev.org/c/openstack/nova/+/945204 | 17:41 |
| ishanwar[m] | Sorry for the spam everyone | 17:41 |
| ishanwar[m] | i didn't realize opendev has been sending a notification here all this time | 17:41 |
| sean-k-mooney | ya it does it also send an email to those on the review | 17:42 |
| sean-k-mooney | its not that distruptive but its goit to do those iteration locally and then push a reivew | 17:43 |
| opendevreview | Ishan Shanware proposed openstack/nova master: [FEAT]: This feat optimises the validate_networks method by reducing networks calls to the database. Here we utilise the quota_details extension to gather the list of uses ports using the neutron extension, this is a smarter query as we don't have to list down all the ports which are in use. Instead we now utilise the quota details extension to get the count of used ports directly. ht | 17:43 |
| sean-k-mooney | it avoids the spam and the ci churn but that comes with practice | 17:43 |
| ishanwar[m] | yeah sorry about that again, will be more careful from now on. | 17:44 |
| ishanwar[m] | I think i have made all the changes i had to right now | 17:44 |
| ishanwar[m] | you can have a look, whenever you can | 17:44 |
| opendevreview | melanie witt proposed openstack/nova master: libvirt: Use common naming convention for ephemeral disk labels https://review.opendev.org/c/openstack/nova/+/947541 | 17:57 |
| melwitt | sean-k-mooney: added code comment to the test and a release note ^ | 17:58 |
| opendevreview | Merged openstack/nova master: wsgi: Don't create, use lock in same line https://review.opendev.org/c/openstack/nova/+/949161 | 18:00 |
| *** Callum0278 is now known as Callum027 | 19:01 | |
| *** benj_0 is now known as benj_ | 21:55 | |
| opendevreview | sean mooney proposed openstack/nova master: update pre-commit version pins https://review.opendev.org/c/openstack/nova/+/949291 | 22:52 |
| opendevreview | sean mooney proposed openstack/nova master: Allow autopep8 to fix more things https://review.opendev.org/c/openstack/nova/+/949292 | 22:52 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!