Wednesday, 2025-02-12

opendevreviewDoug Goldstein proposed openstack/nova stable/2024.1: ironic: Fix ConflictException when deleting server  https://review.opendev.org/c/openstack/nova/+/94084601:07
gmannmikal: as you might have seen spice test running now but failing that is because of schema versions missing  in tempest change, I forgot to check it before. I commented in tempest change https://review.opendev.org/c/openstack/tempest/+/940943/2/tempest/lib/services/compute/servers_client.py02:51
gmannsean-k-mooney: ^^ i think you are debugging the failure 02:56
sean-k-mooneyya so tempest reponce schema rejected the respone03:00
sean-k-mooneythe schema looked ok03:01
sean-k-mooneyso i was wonderign is the patch missing something to apply it03:01
gmannyeah, it is missing this part https://review.opendev.org/c/openstack/tempest/+/940943/comment/4b1cc154_361f1c5e/03:01
sean-k-mooneyoh cool ya so it was not registered in the schema_versions_info list03:02
sean-k-mooneyi should proably go to bed since its like 3 am but this all looks liek it is pretty close03:03
sean-k-mooneyby the way if anyone fells like +2w ing https://review.opendev.org/c/openstack/nova/+/939476 or https://review.opendev.org/c/openstack/nova/+/938523 it would be nice to wake up with those merged :)03:05
opendevreviewRajesh Tailor proposed openstack/nova master: Add support for showing image properties in server show response  https://review.opendev.org/c/openstack/nova/+/93964905:29
mikalgmann / sean-k-mooney: I fix that tempest schema thing and am retesting now.07:11
opendevreviewVasyl Saienko proposed openstack/nova master: Add trunk subports to network metadata  https://review.opendev.org/c/openstack/nova/+/94122708:52
opendevreviewGhanshyam proposed openstack/nova master: Add project manager role in Nova API policy rule  https://review.opendev.org/c/openstack/nova/+/94134709:33
opendevreviewBalazs Gibizer proposed openstack/nova master: Update InstanceNUMACell version in more cases  https://review.opendev.org/c/openstack/nova/+/94095312:56
opendevreviewribaudr proposed openstack/nova master: Add managed flag to PCI device specification  https://review.opendev.org/c/openstack/nova/+/93764913:52
opendevreviewribaudr proposed openstack/nova master: Update driver to deal with managed flag  https://review.opendev.org/c/openstack/nova/+/93840513:52
opendevreviewribaudr proposed openstack/nova-specs master: Amend the specification about the data in LiveMigrate object  https://review.opendev.org/c/openstack/nova-specs/+/94141714:00
bauzasdansmith: doh, somehow items() is already used for o.vo objects https://4c92e32258abec426e9c-a1b3a735e9c0af824100c02f6885a5ce.ssl.cf1.rackcdn.com/940642/6/check/openstack-tox-py39/266ccf7/testr_results.html14:38
dansmithbauzas: for the dict-based one yes, that's my point14:38
bauzasso I need to provide another method name14:38
dansmithif this a dict-compat object? if so you already have items() :)14:38
bauzasdansmith: see, this is calling my own method14:38
bauzasinstead of the original dict() one14:38
bauzasitems() one14:38
dansmithwhy can't you use the base one?14:39
bauzasdansmith: I'll test this14:40
bauzasI haven't seen this method before tbh14:40
dansmithyour object inherits from https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L728 ?14:40
bauzaslooking14:41
bauzasthat's ImageMetaProps14:43
bauzaswhich inherits from the base NovaObject14:43
bauzaswhich apparently isn't using this mixin14:44
dansmithokay I'm not sure what you're saying.. you said your items conflicted with the base items right?14:45
dansmiththat error looks more like you're overriding obj_to_primitive?14:46
bauzasyeah, see the stacktrace14:46
bauzasnope, I'm not overriding it, I'm using it14:46
bauzashttps://review.opendev.org/c/openstack/nova/+/940642/6/nova/objects/base.py14:46
dansmithohh, I see, you're overriding items on the NovaObject base14:47
dansmithbut the dict compat is a mix-in,14:47
dansmithso you're messing up *other* objects I guess14:47
bauzasright14:47
bauzasyeah that14:47
bauzasfor the imageprops object, we're good14:47
bauzasbut when trying to call the original items() on some other object, then it calls out my own items()14:48
bauzasbecause it takes precedence14:48
dansmitheven still, obj_to_primitive().items() is not what you want right? that won't get you the actual fields of the object14:48
bauzaswell, I want to get the key/values of the fields that are set14:48
bauzasand obj_to_primitive gives me all I want14:49
dansmithright, did you look at what obj_to_primitive() returns?14:49
bauzasthe base one, not the one we use for passing over the bus14:49
dansmithit's a nested dict, but the keys in that dict will be metadata14:49
dansmith*object metadata, not your imagemeta fields14:49
dansmithlooks like this: https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L56514:50
dansmithanyway, so you can't override items in the base class because of the mixing in.. seems like the easiest thing is to just grab the fields in your weigher instead of add a new field/method to all our objects (especially since it duplicates what the dict-compat objects already have), but whatever you want 14:51
bauzasno, look, I'm calling our https://github.com/openstack/nova/blob/master/nova/objects/base.py#L33114:53
bauzasbut yeah, I can use that function in the weigher directly14:53
dansmithohh, the helper not the actual object.obj_to_primitive()? gotcha, sorry14:56
bauzasyeah14:56
bauzasor something else I'm thinking14:56
dansmithyeah I see, sorry.. that's old, totally forgot it was there, but fair enough14:57
bauzasdo you think I can just add the mixin directly in the ImageMetaProperties object ?14:57
bauzasI think we don't need to bump a version for this, right?14:57
dansmithyou don't but IIRC lots of work was done to remove it :)14:57
bauzasokay, then lemme use this function directly by the weigher14:58
bauzasthanks14:58
dansmithI would just move it to the weigher, yeh14:59
sean-k-mooneyyou know i provided an implentation of the method in my comment15:17
sean-k-mooneythat grabbed only the populated field form the obejct15:17
sean-k-mooneyand idd not need to primative15:17
sean-k-mooney https://review.opendev.org/c/openstack/nova/+/940642/comment/489497dc_ec3feaf3/15:18
sean-k-mooneyis there a reson we are not doing that15:18
sean-k-mooneyperhaps with a better name15:18
sean-k-mooneyif needed15:18
sean-k-mooneyi speicifcly suggested puting it in  ImageMetaProps https://github.com/openstack/nova/blob/master/nova/objects/image_meta.py#L15815:20
sean-k-mooneyso we coudl give it a good name and keep it targeted to the usecase we needed for the weigher15:20
sean-k-mooneythe implemenmtaion i descibed would work for any obejct too for what its worth so it could be in base which is why i used the generic name set_fields15:22
bauzassean-k-mooney: sorry was on a meeting16:00
bauzassean-k-mooney: there are many possibilities, what would be the one you would prefer ? either I use that method only in the weigher or I provide it in the ImageMetaProps object, as you want16:01
sean-k-mooneyso i wanted to put it in ImageMetaProps because i tought that was cleaner btu i dont want you to call obj_to_primitae at all16:03
sean-k-mooneyso if you want to do it in the wegiehr we could but i tough it was more useful on the object16:04
ratailor__sean-k-mooney, gibi could you please provide your feedback on https://review.opendev.org/q/topic:"bp/image-props-in-server-show" patches.16:04
sean-k-mooneyto me its better encapsulation of code16:04
bauzassean-k-mooney: https://github.com/openstack/nova/blob/master/nova/objects/base.py#L331 does all the magic that we need and I don't want to copy it, so I'd call that helper 16:19
sean-k-mooney it does more then you need and is ineefect16:23
sean-k-mooneyso i dont think you shoudl use that16:23
sean-k-mooneywehn you do obj_to_primitive it produces a dict which asl has all the ovo version info16:24
sean-k-mooneyunless that is diffent then obj.obj_to_primitive()16:25
sean-k-mooneyoh this is form nova object base so maybe it is diffent then the ovo version fo the funtion16:26
sean-k-mooneyok so its going to take this branch https://github.com/openstack/nova/blob/master/nova/objects/base.py#L340-L34416:26
sean-k-mooneybauzas: ok ya the nova obj_to_primitive does what you need16:27
bauzasyup, I just need to call it and adapt the response in order to get a set of tuples instead of a dict16:28
sean-k-mooneylightly more then you need in that it provides the full dict16:28
sean-k-mooneyso you shoudl call .keys() on teh result16:28
bauzasset(mydict.items()) would give me what I need16:28
sean-k-mooneyyou do not want items16:28
bauzasprovided mydict is returned by base.obj_to_prim16:29
bauzasI want the values of the properties, that's important16:29
bauzasI'll compare the tuples (key, value), not only the keys16:29
sean-k-mooneyi dont think the values shoudl be considered16:30
sean-k-mooneywell16:30
bauzasos_type values need to be considered :)16:30
sean-k-mooneyok so you want to mage on the key and value for think like os_type16:30
sean-k-mooneyok 16:30
sean-k-mooneyi see why you want both16:30
bauzasgibi's point was to optionally provide a conf option that would tell which keys to look at16:30
sean-k-mooneyok then ya itmes16:30
sean-k-mooneyya i really dont like that16:31
bauzasI leave that comment open16:31
dansmithI was really hoping we would not need to do that too16:31
sean-k-mooneyto me this is getting overly complex if we need per image prop wights16:31
dansmitheven though I gave a concrete example of how, I also gave a concrete example of how it would break down16:31
bauzasbut I'll repropose a new patch for fixing my items() method16:31
bauzasdansmith: sean-k-mooney: well, the weigher would be defaulting to check for all props, but we could optionally change its behaviour by looking at only a few of them 16:32
bauzasthat's the proposal I made on my last comment but I leave the comment open16:33
bauzasfeel free to chime in16:33
sean-k-mooneyfilterign properties is one thing16:33
sean-k-mooneyper property multiplers is another16:33
sean-k-mooneythat the bit that im not really fond of althogh im not super happy with either16:33
dansmithwell my point is that I feel like the minute you start making some properties worth more than others (filtered properties count as $weight, unfiltered properties count as zero) then,16:34
dansmiththe ask will be to make something important (os type) more important than something less (numa, or disk or something)16:34
sean-k-mooneyya16:34
sean-k-mooneyso ignoring the compelxity this was ment to be a very simple weigher with littel overhead16:35
sean-k-mooneyi.e. you could enable it with very littel conisdiertation to the schduilign performace impact16:35
dansmithI think the question is not about just simplicity, but also how well it works16:35
dansmiththe originally-proposed mechanism was very simple and also very weak, and I assumed that was the goal... to help clump some things, but not be very strong (which needs more complexity to be effective I think)16:36
dansmithhonestly, this might be the kind of thing where "fix it in post" via watcher is more effective16:36
sean-k-mooneyright os i have wanted to do soemthign similar but idffernt for  a number of years, so i propose that loose affintiy apprch"16:36
sean-k-mooneywhat i have wanted ot add is a diffent weigher that i call the boreign host wiether that  perfer hosts with less traitsand less complicate RP trees16:37
dansmithcorrect me if I'm wrong,16:38
dansmithbut if you want to clump your windows instances on windows machines you can set a trait on those hosts, and a requested-but-not-required trait on the image, right?16:38
sean-k-mooneybaiscly avoiding host with complciated things unless you ask for it. in this uescase they wanted to group similar isntances together based on the image properies16:38
sean-k-mooneydansmith: no16:38
dansmithit won't dynamically scale the pool, but otherwise that will be a strong but uncomplicated magnet right?16:39
sean-k-mooneywe dont have prefered traits 16:39
dansmithno?16:39
sean-k-mooneyif we did have a prefered trait wiegher for that it woudl be ideal16:39
sean-k-mooneybut we would also have to dynmical export the widnows trait once an instnace lands on the host in that case16:39
dansmithwe have trait=required but no trait=anythignelse?16:40
dansmithno we wouldn't16:40
dansmithwe could16:40
dansmithbut not strictly required, IMHO16:40
sean-k-mooneyi guess you could pre export it with provider yaml16:41
dansmithright16:41
sean-k-mooneyi.e. tag the nodes up front 16:41
dansmithI think I've made this mistake before, I guess because I assume if there's an =required there must be an =somethingelse :)16:41
dansmithright.. that doesn't make it fully automatic, but also makes it less complicated, predictable, and strong16:41
sean-k-mooneyso we coudl do both. the simple automatic clumping16:42
sean-k-mooneyand in the future add prefered traits supprot16:42
sean-k-mooneywith another weither16:42
sean-k-mooneyhttps://github.com/openstack/glance/blob/5cfecc79995c6a9524f0dbc4553b4a88761b382f/doc/source/admin/useful-image-properties.rst?plain=1#L277-L30616:42
sean-k-mooney^ we only supprot required but not forbiden or prefered in the image16:43
dansmithpreferred traits needs no placement support, right? just a weigher16:43
dansmithyep16:43
sean-k-mooneyjust a weigher and much of the work is done16:43
sean-k-mooneywe jsut need to put the provider summeries in the hoststate object16:43
dansmithis that not basically the same amount of work as this? why not just do that?16:43
dansmithoh, do we not have the ability to see traits on hosts from the weighers now16:44
sean-k-mooneyno but we already have the info in the host manger16:44
sean-k-mooneywe only pass the allcoations 16:44
sean-k-mooneyso we can see what you asked for but not what is on the host16:44
sean-k-mooneyso we can just pass both16:44
sean-k-mooneybecause we have both in the schduler alrady so no change to the placement request16:45
dansmithack, seems like that would be a non-trivial increase in the hoststate though16:45
sean-k-mooneyits more like a exnetion in the time before its garbage collected maybe16:46
sean-k-mooneyi think we end up keepign them in memory anyway because of the function call path but i might eb wrong16:46
dansmithso we already report we just don't expose it to the weighers?/16:46
sean-k-mooneyyes16:46
sean-k-mooneyso i was randomly hackign on https://review.opendev.org/c/openstack/nova/+/933478 a while ago and wanted to use them and found its not aviabel16:47
sean-k-mooneydansmith: we didnt have ether aviabel to the weiers untile pci in palcemnt and gibi only passed the allcoation since that is all they needed16:48
bauzasokay, I hear your voices, I'll keep that weigher stupidly simple16:49
dansmithmaybe we should have a gmeet amongst the four of us and make sure we have a path forward16:50
dansmithI don't think gibi is going to be happy with simple, and it may not address the actual concern according to him, so we might just want to get on the same page16:50
sean-k-mooneywe can if we want too16:51
sean-k-mooneybut i wonder if we should be rushing into the design16:51
sean-k-mooneyim find we havign a call to see if we have a common undersanding16:52
sean-k-mooneyi just wonder if we shoudl disucss this at the ptg instead. we can totally do it now if we think there is a useful veriosn we coudl deviler this cycle16:52
sean-k-mooneybut i dont want to merge somethign we think wont acutlly be useful16:53
dansmithI don't want to rush, but I also don't think this is complicated enough that we need to push it off... it doesn't affect rpc or api, which means getting it perfect the first time is not really a big deal, IMHO16:53
sean-k-mooneyack16:54
sean-k-mooneyfyi https://github.com/openstack/nova/blob/32326d48949525a995d132bddc90e710c229947f/nova/scheduler/manager.py#L195-L205 is where we have the allcoation and the provider summeries already16:54
sean-k-mooneywe also pass them into _schedule https://github.com/openstack/nova/blob/32326d48949525a995d132bddc90e710c229947f/nova/scheduler/manager.py#L195-L34316:55
sean-k-mooneywe are just not assocating them with the host or pasing them to get_sorted_hosts https://github.com/openstack/nova/blob/32326d48949525a995d132bddc90e710c229947f/nova/scheduler/manager.py#L42116:56
gibisorry I was not able to follow this discussion as I had parallel back to back meetings. At this point I only care about solving the real use case.17:28
gibiIf there is an agreement that the current proposal solves the real use case, or even solve a real use case, then I'm OK with it.17:31
opendevreviewSylvain Bauza proposed openstack/nova master: Add a new ImagePropertiesWeigher  https://review.opendev.org/c/openstack/nova/+/94064217:35
bauzasI just uploaded a new rev17:35
bauzasbut I'm fried for today17:35
bauzasI'll rather grab a beer and hardstop17:35
bauzas-ETOOMUCHCONTEXTSWITCHING17:36
sean-k-mooneyya im feeling really unproductive today for the same reason17:37
gibiindeed a lot of parallel things these days17:44
bauzasthis chan is logged and public, but you guys know and I'm just a sad panda to not have decent time for reviewing17:49
bauzastomorrow, I'll litterally block my agenda for such thing17:49
sean-k-mooneygibi: bauzas i comemted on the latest version. i think the best path forward would be to have a second patch to copy the metric where imple for matching on indiviual properteis if we really want to supprot that18:06
sean-k-mooneythat way we keep the syntax common between the two weigher and if we agree we want both then we can merge both pathces18:06
sean-k-mooneyif we think we just want to keep it simple then we merge the first one18:07
mikalWhat user do tempest tests run as? Because https://zuul.opendev.org/t/openstack/build/a2c28aa2ebc248509f5b586411172955 is getting {'code': 403, 'message': "Policy doesn't allow os_compute_api:os-console-auth-tokens to be performed."} which it shouldn't if its an admin user.18:43
dansmithdepends on the test and the client you use18:48
dansmiththere's like a self.client and self.admin_client on most test classes, but I think maybe not always if the setup didn't foresee the need18:49
dansmithbut that might give you a grep thread to follow18:49
sean-k-mooneyif you want to call admin apis you expelcity need to use an admin client in the test ya18:49
mikalHmmm, ok. I cargo culted the vnc tests, and its using "self.servers_client", whatever that is.18:50
sean-k-mooneyyou might be ablle to just pass admin=ture to that18:50
sean-k-mooneythere are diffent helpers in different parts to do that18:51
sean-k-mooneyhttps://github.com/openstack/tempest/blob/master/tempest/api/compute/base.py#L691C13-L691C3318:51
sean-k-mooneyyou can maybe just copy paste cls.admin_servers_client = cls.os_admin.servers_client18:52
mikalOh, I can just inherit from BaseV2ComputeAdminTest not BaseV2ComputeTest I think.18:53
sean-k-mooneythat would proably work too18:53
sean-k-mooneyif you do that you might want to move the test to https://github.com/openstack/tempest/tree/master/tempest/api/compute/admin18:54
dansmithjust curious though, why is admin required here?18:54
sean-k-mooneythe hypervior ip/ports are only shown to the admin18:54
mikalTurning a console token into hypervisor IP and port requires admin.18:54
dansmithoh because no proxy like normal I guess?18:55
sean-k-mooneyright so we allow kerbside to get them but not the normal user18:55
dansmithgotcha18:55
mikalYeah, a normal user can request a console but then has to ask kerbside to connect to it for them.18:56
mikalOh, which I guess means I need two clients in this test...18:56
mikalAttempt 11!18:56
sean-k-mooneywell you could cheat and jsut do everythin as admin but if you wanted to have a negitive test adn show normal user cant see this then ya18:58
opendevreviewAlan Bishop proposed openstack/nova master: Show swap_progress in attachment details  https://review.opendev.org/c/openstack/nova/+/94147619:01
opendevreviewMengyang Zhang proposed openstack/nova master: Add Burst Length Support to Cinder QoS  https://review.opendev.org/c/openstack/nova/+/93949020:10
opendevreviewMengyang Zhang proposed openstack/nova master: Add Burst Length Support to Cinder QoS  https://review.opendev.org/c/openstack/nova/+/93949020:15
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Add [libvirt]default_tpm_secret_security  https://review.opendev.org/c/openstack/nova/+/94019420:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Add [libvirt]supported_tpm_secret_security  https://review.opendev.org/c/openstack/nova/+/94019520:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Add hw_tpm_secret_security image property  https://review.opendev.org/c/openstack/nova/+/94019620:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Add hw:tpm_secret_security extra spec validation  https://review.opendev.org/c/openstack/nova/+/94019720:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Ensure secret security policy for TPM instances  https://review.opendev.org/c/openstack/nova/+/94106220:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Allow vTPM live migration for `deployment` secret security  https://review.opendev.org/c/openstack/nova/+/92577120:25
opendevreviewArtom Lifshitz proposed openstack/nova master: WIP: Modify vTPM LM block to block legacy instances  https://review.opendev.org/c/openstack/nova/+/94148320:25
mikaloh nice, because os-console-auth-tokens previously had zero testing, I also need to define what schema it should have in tempest.21:22
mikalDoes anyone have an opinion on if I should just add that schema in the new microversion, or try to work out when it was actually introduced and add it back then?21:23
melwittgmann: ^21:28
opendevreviewTobias Urdin proposed openstack/nova master: Add minute to instance_usage_audit_period  https://review.opendev.org/c/openstack/nova/+/94148821:45
opendevreviewMichael Still proposed openstack/nova master: libvirt: direct SPICE console object changes  https://review.opendev.org/c/openstack/nova/+/92687622:34
opendevreviewMichael Still proposed openstack/nova master: libvirt: direct SPICE console database changes  https://review.opendev.org/c/openstack/nova/+/92687722:34
opendevreviewMichael Still proposed openstack/nova master: libvirt: allow direct SPICE connections to qemu  https://review.opendev.org/c/openstack/nova/+/92484422:34

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