Thursday, 2025-03-06

opendevreviewCallum Dickinson proposed openstack/nova master: Add image meta to libvirt XML metadata  https://review.opendev.org/c/openstack/nova/+/94276600:23
*** jayaanand_ is now known as jayaanand01:24
opendevreviewmelanie witt proposed openstack/nova master: Let oslo.limit handle retrieval of the endpoint ID  https://review.opendev.org/c/openstack/nova/+/94350702:54
opendevreviewMichael Still proposed openstack/nova master: Don't calculate the minimum compute version repeatedly.  https://review.opendev.org/c/openstack/nova/+/94084807:33
opendevreviewMichael Still proposed openstack/nova master: libvirt: Add extra spec for sound device.  https://review.opendev.org/c/openstack/nova/+/92612607:33
opendevreviewMichael Still proposed openstack/nova master: Protect older compute managers from sound model requests.  https://review.opendev.org/c/openstack/nova/+/94077007:33
opendevreviewMichael Still proposed openstack/nova master: libvirt: Add extra specs for USB redirection.  https://review.opendev.org/c/openstack/nova/+/92735407:33
opendevreviewXiang Wang proposed openstack/nova master: Improve Error Handling for Placement API Responses in Nova  https://review.opendev.org/c/openstack/nova/+/94353008:42
opendevreviewXiang Wang proposed openstack/nova master: Improve Error Handling for Placement API Responses in Nova  https://review.opendev.org/c/openstack/nova/+/94353008:43
opendevreviewGaudenz Steinlin proposed openstack/nova master: Make SSH compression for remote copy configurable  https://review.opendev.org/c/openstack/nova/+/94337709:57
sean-k-mooneygibi: bauzas  this is a nice performance fix form mikal that came up during the spice series https://review.opendev.org/c/openstack/nova/+/940848 it would be nice to include that in epoxy10:49
bauzasdone10:53
* bauzas doing the rubber stamping paperwork to look at what we merged and what we need before RC110:53
sean-k-mooneycool let me know if we have anything pendign that need another pair of eyes10:54
bauzaswe'll need to merge the usual release patches10:55
bauzasbut I'll ping folks for sure10:55
* bauzas goes cycling, the weather is nice10:55
sean-k-mooneyim starting to prepare the same for wathcer. i diid they cycle highlights yesterday and have a draft of the prelude but i need to update launchpad blueprints and the specs repo ectra10:56
sean-k-mooneyi also need to confirm if watcher needs the same rpc aliases ectra that we need for nova10:56
opendevreviewMerged openstack/nova master: Don't calculate the minimum compute version repeatedly.  https://review.opendev.org/c/openstack/nova/+/94084812:54
opendevreviewGaudenz Steinlin proposed openstack/nova master: Make SSH compression for remote copy configurable  https://review.opendev.org/c/openstack/nova/+/94337713:45
dansmithgibi: this is a little different than I remember it originally: https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L205915:15
dansmiththat seems to do an atomic swap and not an allocate..delete15:15
dansmithI can go dig, but you might know quicker: will that check reserved or let us get away with the swap if we're reserved?15:16
gibidansmith: I have not context but at least that seems to be a singel request from nova to placement. I don't know how placement processes that. I know that when nova tries to move an allocation that is for a reserved resource the placement call will fail.15:24
dansmithokay yeah i think it will15:24
gibibut I don't know the exact behavior of placement here15:24
gibiI mean the internal behavior15:24
dansmithwhat I think placement does is start a transaction, delete the source allocations, create the replacement allocations, then _check_capacity_exceeded() and if that fails, roll back the transaction15:25
sean-k-mooneyhttps://github.com/openstack/placement/blob/master/placement/tests/functional/gabbits/allocations-bug-1779717.yaml15:25
sean-k-mooneywe can update/create mutliple allocation at once15:25
dansmithright, but they will still check the reserved amount15:25
sean-k-mooneypossibly15:25
dansmiththey do15:26
sean-k-mooneyok15:26
dansmithhttps://github.com/openstack/placement/blob/master/placement/objects/allocation.py#L7315:26
dansmithsee my logic above, this ^ will run after the swap and fail because we're over capacity due to the reserved amount15:26
sean-k-mooneyright i think that what happend for evacuate too15:26
dansmithyeah, so I think from the outside, it's reasonable for an api consumer to assume that "allocations.replace_all()" is just a swap but it's not15:27
sean-k-mooneywell maybe15:28
sean-k-mooneyits was not orgianlly for swappign it was for doign muitple updates i think15:28
gibifrom API semantic perspective it is not well defined what it means when such a POST call has multiple consumers15:28
dansmithit's also reasonable to assume that we check the capacity again, I'm just saying I think it's a bit ambiguous15:28
dansmithgibi: exactly15:28
gibiit might be a move it might be two independent allocation in the POST15:29
sean-k-mooneyyep15:29
dansmithso one way to resolve that would be to say "if we were over capacity before, we allow you to be over capacity by the same exact amount after, otherwise fail"15:29
sean-k-mooneythat or a flag to contole the behavior15:29
sean-k-mooneyeither way i think it need a new microversion15:29
dansmithyeah I was also thinking a flag would be a thing15:30
dansmithsean-k-mooney: can you dig up a reference on the evacuate bug?15:30
sean-k-mooneygibi found it not long ago i can check my email/irc/slack logs and see15:31
sean-k-mooneyi know the last time it came up as a downstream it was because the oeprator had adjusted the allcoation ratio15:31
sean-k-mooneyputting the host into an oversubsiped state and then later could not evacuate15:31
sean-k-mooneybut there was a second edge case too15:32
dansmithright, so that's what I was going to say,15:32
dansmithI think that an alloc ratio change or reserved change will make our existing code fail to swap an allocation of equal size, when it really shouldn't15:33
dansmithfor all the move ops15:33
sean-k-mooneyin the evacuate case i think we only have one allcoation however it just happens ot have resouce form 2 hosts or somethign slightly odd15:34
dansmithany feeling on allowing the reserved check to be skipped if the total size of the allocations against a provider are not changing? or do you think we need a flag to ask for that specifically15:35
dansmithI tend to think a new microversion after which we say that's the behavior is okay,15:35
sean-k-mooneyhttps://bugs.launchpad.net/nova/+bug/1943191 https://bugs.launchpad.net/nova/+bug/1924123 and https://bugs.launchpad.net/nova/+bug/194189215:35
dansmithbut if we want to only do that if we specifically ask for it then...15:35
dansmithheh the subject of that first bug is exactly what I'm saying yeah15:36
sean-k-mooneynon of those actully say evacuate bu ti think its a releated issue15:37
dansmithbut blocking cold migrate is worse15:38
dansmithbrb15:38
sean-k-mooneyas is sid i think, i could be wrong about this, that for evacuate we do not have a migration uuid becuase its actully not a "move op" in that sence it just rebuidl to a new host15:38
sean-k-mooneyso for evacuate i think we extend the single allcoaiton with rp for the dest15:38
sean-k-mooneyi could be wrong about that because i know we change that at some point15:39
sean-k-mooneyi would have to look at the detail15:39
dansmithyeah idk, I'm more familiar with the cold migrate case, which is clearly broken already15:40
gibisean-k-mooney: yeah evac is implement the way you describe15:41
sean-k-mooneywe talked about chaging htat at some point but there was a reaons why it was hard15:42
gibiI'm wondering if it is enough to check if the allocated value does not change to use the swap mode instead of using the current independent allocation mode. I mean from placement API perspective there just not enough information about the intention of the user.15:42
gibiwhat if at some point we want a swap behavior that also decreases the size15:42
dansmithgibi: I don't think there are multiple different operations are there?15:43
dansmithwe're using POST /allocations for both15:43
dansmithand placement is using the same logic regardless of if we're deleting and re-allocating the same provider vs. different ones, AFAIK15:43
gibiyeah, my point is that POST call can be understood both ways (swap or create/delete) even if the total allocated resources does not change in the process15:44
dansmithbut yeah that was my point.. if the operation is not further over-subscribing the provider (or maybe not changing the amount at all) it seems reasonable to let it through15:44
gibiso placemetn does not know15:44
dansmithyeah15:44
sean-k-mooneyif the set of RPs in the set of allction dont change and the total resouces consuemd dont hcange then we might be able to skip the capastiy check15:44
gibiif nova tells placement it is a swap, then do a swap regardless of the allocation size. but otherwise keep the current behavior of create/delete15:45
dansmithon the one hand, if you have an allocation for X, right now you can't even *decrease* the allocation size to get closer to happy because "closer to happy is still oversubscribed so reject"15:45
sean-k-mooneyhonestly it would be better to do this in 2 calls15:45
dansmithsean-k-mooney: I don't see how.. two calls seems worse to me15:45
sean-k-mooney1 call to create the dest allocaotn and a dedicated call to swap the 2 allocations15:45
dansmithgibi: a flag to just skip the consistency check also feels wrong to me15:45
dansmithsean-k-mooney: oh that's not the problem, it's the swap15:46
sean-k-mooneywell right now i think we do the swap an creation of the dest allocation in one call15:46
gibiif a flag in the current API is to anti REST then lets have a separate resource explicitly for swapping15:46
dansmitheither way, for the purposes of one-time-use devices, can we say this is already a problem that needs to be solved and it'll block cold migrate for OTU but once we fix it, we're good?15:46
gibiyes that is fair to say15:47
dansmithI will commit to fixing both, FWIW15:47
sean-k-mooneyi.e. dont we update the allcoation with the instance uuid to the test host RPs and move the exiting allcotions to the migraiton uuid in one call that just "modifies multiple allcotions"15:47
gibisean-k-mooney: we remove the allocation from the instance uuid and add it to the migration uuid in a POST call then we do a scheduling15:48
dansmithgibi: I think a flag would be okay if the semantics are "allow equal or less oversubscription to exist after you do this"15:48
dansmithright now it is "oversubscription means fail" even if it's the same amount as before15:48
gibidansmith: if the client explicity call a swap between two consumers placement should just do a swap without extra checks15:48
gibii.e. keep the API generic15:49
sean-k-mooneyif we move the current allction to the migration uuid before we schedule15:49
dansmithgibi: well, that's not very RESTful I think15:49
sean-k-mooneythen im a little confused15:49
dansmithgibi: what about a PATCH to just change the consumer uuid?15:49
gibidansmith: sounds good to me15:49
dansmith(not sure if we can be that fine-grained about it, I'd have to look)15:49
sean-k-mooneybecause at that point we jus tneed to convert the allcoation candiate abck to an allction with the instance uuid15:49
sean-k-mooneyand then elete the miggration uuid15:50
sean-k-mooneyso it does not need ot be in one api post15:50
gibiit is really should be a simple operation to update the owner of the allocation (of a full blown swap between two consumers but we don't need that right now)15:50
sean-k-mooneywell that you can do15:50
gibis/of/or/15:50
sean-k-mooneyyou can modify the consumer uuid15:50
sean-k-mooneywell the consumer but iint the allcotion uuid the instance uuid or migration uuid depending15:51
sean-k-mooneywe added consumer_type to the api at some point to track that15:51
gibiI don't think we have a single placement API operation where you can update the consumer uuid without a reallocation (like the POST call we do currently)15:52
gibidansmith: so for me I don't need a full blown swap operation between any two consumer. I'm OK to have just a PATCH on an existing allocation to update the consumer uuid in it and do nothing else.15:53
sean-k-mooneythis is the existing api we are talking about yes https://docs.openstack.org/api-ref/placement/#update-allocations15:54
dansmithgibi: yeah if we can do that cleanly that might be good.. I'll have to start digging in to determine, but yeah15:54
dansmithare you thinking PATCH /allocations/uuid1 with {consume_uuid: uuid2} ?15:54
dansmiththat will work, not sure how the REST police feel, but I'd think it's better than a POST /swap_action15:55
gibisean-k-mooney: we are talking about https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L2059 that is what we do today15:55
gibidansmith: yeah lets ask the REST gods about it but the PATCH feels simple to me15:55
dansmithack15:55
sean-k-mooneypatch is an update to an exiting resouce15:56
sean-k-mooneythe resouce is keyd by the consumer_uuid /allocations/{consumer_uuid}15:56
sean-k-mooneywe could supprot patch to chagne that15:56
sean-k-mooneywihtout conflicting with the existing put15:56
sean-k-mooneybut it feesl a littel od ot do /allocations/{instance_uuid} bode: consumer_uuid=<migration uuuid>15:57
gibiyeah we need to change the "key" that is why it feels problematic from REST perspective15:57
bauzasI started to pay attention to this channel but I failed getting the problem to solve15:57
sean-k-mooneyi dont hate it15:57
bauzascan someone summarize me the thread ?15:57
* bauzas becomes lazy now he's around to be past 4515:58
sean-k-mooneythe ohter option is somthign liek /allocations/{old uuid}/swap/{new uuid}15:58
dansmithbauzas: https://bugs.launchpad.net/nova/+bug/194319115:58
gibibauzas: move operations does not work if the source RP is over allocated or allocated and reserved15:58
sean-k-mooneythat would be a post i guess15:58
dansmithbauzas: that and several other bugs, plus my OTU spec15:58
bauzasI see15:59
sean-k-mooneygibi: i think if we document things properly iether could work i think POST /allocations/{consumer_uuid}/swap/{new_consumer_id} is clear honestly16:00
sean-k-mooneytha twould not have any body16:01
sean-k-mooneyso there is no posiblity of changing the allcotin or capsity useage and there for its ok to not do checks16:01
sean-k-mooneyif we go with patch we would have to ensure the resouces are not changed as well16:02
gibiyeah I'm not against either the PATCH or the POST //swap/ way16:07
sean-k-mooney i think we can all agree that if it alllow us to finally resovle thos bugs in nova either approch is better then what we have today. so if that the direction we head in im fine with say lets define that in the placement api spec16:10
gibiI agree16:13
dansmithyup16:14
melwittsean-k-mooney: not sure what you'll think of this but I uploaded a patch in regard to the oslo.limit endpoint discovery https://review.opendev.org/c/openstack/nova/+/943507 fyi17:06
sean-k-mooneyso we remove the end point check since oslo limists will do that for use internally17:17
sean-k-mooneyand we repalce the end point fuction with one that returns the limits for this service/region17:18
sean-k-mooneythese are the global limits right not the project specific ones17:19
sean-k-mooneymelwitt: without properly loadign context and looking at oslo limits its seams reasonable at first glance17:21
melwittsean-k-mooney: yeah the thing I think is controversial is the private attributes access17:23
sean-k-mooneythat not required with the latest version of oslo.limits right17:24
melwittmaybe in parallel propose some public accesses in oslo.limit and update it when available? dunno17:24
sean-k-mooneyhttps://review.opendev.org/c/openstack/oslo.limit/+/914783/3/oslo_limit/limit.py17:25
melwittwhat is there currently will work for both old and new versions17:26
melwittand underneath oslo.limit will do what it needs to 17:27
sean-k-mooneyright in the new version we can just call get_registered_limits17:27
sean-k-mooneyon the enforcer17:27
sean-k-mooneyhttps://github.com/openstack/oslo.limit/blob/8ada297e6a82f7e50bf746453b8dd21716743798/oslo_limit/limit.py#L18317:27
sean-k-mooneymelwitt: do you need to use the connection today17:29
sean-k-mooneyoh that funciton takes a set of resouces to check17:30
melwittsean-k-mooney: yeah I do that because I want to pull all the resourcs limits in one API call. oslo.limiit does them one API call per resource17:33
sean-k-mooneyit sound like we shoudl add a funciton to oslo limits to do that17:34
sean-k-mooneybut i think we can work around it in nova for now17:34
melwittmaybe. there's already one so not sure if we change the internal impl or make another alongside17:34
sean-k-mooneyyou could make None a seential value for all or use the sentenial object pattern17:35
melwittyeah17:36
sean-k-mooneyso gibi's main concern seams to be those are private filed and there for could break which is valid. if oslo were ok providing public access in some way i would be ok with this as a backportable approch as long as we update to using the public appropch in the future17:38
melwittsean-k-mooney: right. it's mine too, which is why I mentioned it in the commit message. yeah I think that sounds reasonable17:39
sean-k-mooneyi dont hate it but if we could just do ENFORCE.get_registered_limists(None)17:39
sean-k-mooneyi would be happier17:39
melwittsure17:39
sean-k-mooneythe other option for now17:39
melwittwill that work currently? /me looks17:40
sean-k-mooneyis kep doing it the old way and instead fo raising try and do it this way17:40
sean-k-mooneymelwitt: i was trying ot figure that out17:40
sean-k-mooneyyou can pass it i just dont know what happens if you do17:40
sean-k-mooneyits this right https://docs.openstack.org/api-ref/identity/v3/#list-registered-limits17:41
melwittit looks like it would do nothing17:41
melwitthttps://github.com/openstack/oslo.limit/blob/master/oslo_limit/limit.py#L35517:41
sean-k-mooneyit say resouce name is optional in the api17:41
sean-k-mooneyyou could try [""]17:41
melwittyes that's why I'm using the connection directly17:42
sean-k-mooneyi.e. ENFORCE.get_registered_limists([""])17:42
sean-k-mooneyya well modle.connection is public it seams17:42
sean-k-mooneyits just the endpoint id and service id that not right17:42
melwittyes and I'm using it in that patch :)17:42
melwittEnforcer.get_registered_limits() does "for resource_name in resource_names:" and will do nothing if you don't pass it a list of names and if you pass it a list of names it will pull each limit in a separate API call17:43
melwittsean-k-mooney: correct it's the endpoint id and service id that are not public17:44
melwittI could alternatively pull those separately the same way that oslo.limit does today17:45
sean-k-mooneyya im not seeing a better way to do it with the current api17:45
melwittI considered that might be more well liked17:45
melwittso maybe I should push a new PS that pulls the services and then gets the endpoint ID from it, that way no private attrs need to be used17:46
sean-k-mooneythats an incremental imporment. i fell lilke they didnt fully fix the usablity problem17:47
sean-k-mooneyas in they didnt really fix the usablity problem reported in https://launchpad.net/bugs/193187517:48
sean-k-mooneyif we have to port that code to nova17:48
melwittwell that works for 99% of users who have no need to query for the list of limits17:48
sean-k-mooneyya but athat a pretty fundemental thing17:49
melwittstrictly for limit enforcement what they did fixes it all17:49
sean-k-mooneyi guess oslo.limits is not inteded as a generic keyston clinet17:50
sean-k-mooneyyou could argue that we shoudl not be using ot get the registered limits and shoudl use the sdk instead17:51
melwittyeah.. it's not. that was what I felt conflicted about. like making it a passthrough to the underlying keystone client? when it already has a public connection attribute? I dunno. just seemed kind of weird17:51
melwittyeah. I think maybe they did that because the keystone API does not have the ability to filter a limit list by a set of resource names. but you could just filter it yourself after getting it. you'll get all of them in one API call17:52
sean-k-mooneyubuntu@sean-devstack-watcher-1:~/repos/nova$ curl -s -H "X-Auth-Token: $TOKEN" -H "OpenStack-API-Version: compute 2.99" http://192.168.50.140/identity/v3/registered_limits | jq | nc termbin.com 999917:53
sean-k-mooneyhttps://termbin.com/kqfj17:53
melwittso maybe the right thing to do is "fix" the internal impl of get_registered_limits() to just filter in python instead of doing N API calls17:54
sean-k-mooneywithout a regioun or service id you get ll registered limits if you call keystoen17:54
melwittI know lol17:54
sean-k-mooneythe region we know right17:55
melwittthat's literally what it's doing already17:55
melwittyes we know the region17:55
sean-k-mooneywell yes but is it just the service id we ned17:55
melwittwe don't know the endpoint id17:55
melwittyes17:55
melwitter sorry yeah it's service id that the call actually needs. I keep mixing it up with the conf options17:57
sean-k-mooneyso the sdk supprot the service17:57
sean-k-mooneyhttps://github.com/openstack/openstacksdk/blob/master/openstack/identity/v3/service.py17:57
sean-k-mooneyand you can query by type i.e.. nova 17:58
sean-k-mooney*compute17:58
melwittyes, that's what oslo.limit does for the discovery17:58
melwitthttps://github.com/openstack/oslo.limit/blob/master/oslo_limit/limit.py#L29417:58
sean-k-mooneyya so you were suggeting we coudl prot that to nova17:58
melwittso we can do that too obviously17:58
melwittyeah, if I do that then no more private access. I thought about it but figured I'd just throw the "most efficient" thing up to get discussion17:59
opendevreviewDan Smith proposed openstack/nova-specs master: Add one-time-use-devices spec  https://review.opendev.org/c/openstack/nova-specs/+/94348617:59
melwittthat would also get us endpoint discovery as a backport18:00
sean-k-mooneyya18:00
melwittprobably a better move. I'll push an update with that and see what yall think18:01
sean-k-mooneybefore you do 18:04
sean-k-mooneywhy are we even using oslo.limist for this when the openstack sdk has a function ot get registered limits 18:05
sean-k-mooneyhttps://github.com/openstack/openstacksdk/blob/master/openstack/identity/v3/_proxy.py#L1625-L163418:05
melwittsean-k-mooney: to make it use the [oslo.limit] config section. or do you suppose there's another way to do that?18:05
sean-k-mooneywell we coudl read those config varible oursleves18:06
sean-k-mooneyi.e. for region 18:06
sean-k-mooneylookup the service id our selves and then just call keystone directly18:07
melwittyeah. will need all the other auth stuff too and I was thinking it should match what nova is using for enforcing18:07
sean-k-mooneyor even use our keystoneauth section18:07
sean-k-mooneyi would agree if oslo.limit provided the apis we needed :)18:08
sean-k-mooneyok for now lets not boil the ocean18:08
sean-k-mooneyand focus on the smallest change to make it work18:08
melwitteh.. I was wary of that because the idea of using one avenue to get the limits and a different one to enforce. I'm not sure if there could be consequence for some part of the config being different18:08
melwittI was thinking it's likely less error prone to use the same exact thing oslo.limit is using18:08
melwittsame user, same password etc and not let there be the ability to make them different 18:09
melwittand creating a dummy enforcer seemed like the simplest way to do that. maybe I can make a direct sdk connection read the config section I want. there's probably a way to do that, I'll check18:11
melwittoh yeah, looks like that can be done easily. I'll try that and see what we think18:13
opendevreviewMerged openstack/nova master: api: Add response body schemas for for console auth token APIs (v2.99)  https://review.opendev.org/c/openstack/nova/+/94325220:37
*** haleyb is now known as haleyb|out22:48

Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!