Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-02-12
14:40:09 bauzas dansmith: I'll test this
14:40:32 bauzas I haven't seen this method before tbh
14:40:54 dansmith your object inherits from https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L728 ?
14:41:56 bauzas looking
14:43:25 bauzas that's ImageMetaProps
14:43:40 bauzas which inherits from the base NovaObject
14:44:53 bauzas which apparently isn't using this mixin
14:45:54 dansmith okay I'm not sure what you're saying.. you said your items conflicted with the base items right?
14:46:30 dansmith that error looks more like you're overriding obj_to_primitive?
14:46:32 bauzas yeah, see the stacktrace
14:46:56 bauzas nope, I'm not overriding it, I'm using it
14:46:59 bauzas https://review.opendev.org/c/openstack/nova/+/940642/6/nova/objects/base.py
14:47:14 dansmith ohh, I see, you're overriding items on the NovaObject base
14:47:24 dansmith but the dict compat is a mix-in,
14:47:41 dansmith so you're messing up *other* objects I guess
14:47:42 bauzas right
14:47:46 bauzas yeah that
14:47:56 bauzas for the imageprops object, we're good
14:48:16 bauzas but when trying to call the original items() on some other object, then it calls out my own items()
14:48:23 bauzas because it takes precedence
14:48:26 dansmith even still, obj_to_primitive().items() is not what you want right? that won't get you the actual fields of the object
14:48:49 bauzas well, I want to get the key/values of the fields that are set
14:49:02 bauzas and obj_to_primitive gives me all I want
14:49:05 dansmith right, did you look at what obj_to_primitive() returns?
14:49:17 bauzas the base one, not the one we use for passing over the bus
14:49:20 dansmith it's a nested dict, but the keys in that dict will be metadata
14:49:37 dansmith *object metadata, not your imagemeta fields
14:50:16 dansmith looks like this: https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L565
14:51:18 dansmith anyway, 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:53:13 bauzas no, look, I'm calling our https://github.com/openstack/nova/blob/master/nova/objects/base.py#L331
14:53:43 bauzas but yeah, I can use that function in the weigher directly
14:56:33 dansmith ohh, the helper not the actual object.obj_to_primitive()? gotcha, sorry
14:56:47 bauzas yeah
14:56:54 bauzas or something else I'm thinking
14:57:06 dansmith yeah I see, sorry.. that's old, totally forgot it was there, but fair enough
14:57:12 bauzas do you think I can just add the mixin directly in the ImageMetaProperties object ?
14:57:29 bauzas I think we don't need to bump a version for this, right?
14:57:48 dansmith you don't but IIRC lots of work was done to remove it :)
14:58:28 bauzas okay, then lemme use this function directly by the weigher
14:58:40 bauzas thanks
14:59:18 dansmith I would just move it to the weigher, yeh
15:17:05 sean-k-mooney you know i provided an implentation of the method in my comment
15:17:20 sean-k-mooney that grabbed only the populated field form the obejct
15:17:24 sean-k-mooney and idd not need to primative
15:18:13 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/940642/comment/489497dc_ec3feaf3/
15:18:34 sean-k-mooney is there a reson we are not doing that
15:18:42 sean-k-mooney perhaps with a better name
15:18:46 sean-k-mooney if needed
15:20:00 sean-k-mooney i speicifcly suggested puting it in ImageMetaProps https://github.com/openstack/nova/blob/master/nova/objects/image_meta.py#L158
15:20:20 sean-k-mooney so we coudl give it a good name and keep it targeted to the usecase we needed for the weigher
15:22:06 sean-k-mooney the 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_fields
16:00:51 bauzas sean-k-mooney: sorry was on a meeting
16:01:52 bauzas sean-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 want
16:03:39 sean-k-mooney so i wanted to put it in ImageMetaProps because i tought that was cleaner btu i dont want you to call obj_to_primitae at all
16:04:03 sean-k-mooney so if you want to do it in the wegiehr we could but i tough it was more useful on the object
16:04:30 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:31 sean-k-mooney to me its better encapsulation of code
16:19:30 bauzas sean-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:23:38 sean-k-mooney it does more then you need and is ineefect
16:23:46 sean-k-mooney so i dont think you shoudl use that
16:24:37 sean-k-mooney wehn you do obj_to_primitive it produces a dict which asl has all the ovo version info
16:25:38 sean-k-mooney unless that is diffent then obj.obj_to_primitive()
16:26:06 sean-k-mooney oh this is form nova object base so maybe it is diffent then the ovo version fo the funtion
16:26:50 sean-k-mooney ok so its going to take this branch https://github.com/openstack/nova/blob/master/nova/objects/base.py#L340-L344
16:27:21 sean-k-mooney bauzas: ok ya the nova obj_to_primitive does what you need
16:28:01 bauzas yup, I just need to call it and adapt the response in order to get a set of tuples instead of a dict
16:28:08 sean-k-mooney lightly more then you need in that it provides the full dict
16:28:20 sean-k-mooney so you shoudl call .keys() on teh result
16:28:41 bauzas set(mydict.items()) would give me what I need
16:28:59 sean-k-mooney you do not want items
16:29:01 bauzas provided mydict is returned by base.obj_to_prim
16:29:26 bauzas I want the values of the properties, that's important
16:29:44 bauzas I'll compare the tuples (key, value), not only the keys
16:30:01 sean-k-mooney i dont think the values shoudl be considered
16:30:10 sean-k-mooney well
16:30:20 bauzas os_type values need to be considered :)
16:30:26 sean-k-mooney ok so you want to mage on the key and value for think like os_type
16:30:28 sean-k-mooney ok
16:30:33 sean-k-mooney i see why you want both
16:30:48 bauzas gibi's point was to optionally provide a conf option that would tell which keys to look at
16:30:53 sean-k-mooney ok then ya itmes
16:31:00 sean-k-mooney ya i really dont like that
16:31:11 bauzas I leave that comment open
16:31:22 dansmith I was really hoping we would not need to do that too
16:31:38 sean-k-mooney to me this is getting overly complex if we need per image prop wights
16:31:40 dansmith even though I gave a concrete example of how, I also gave a concrete example of how it would break down
16:31:50 bauzas but I'll repropose a new patch for fixing my items() method
16:32:49 bauzas dansmith: 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:33:05 bauzas that's the proposal I made on my last comment but I leave the comment open
16:33:11 bauzas feel free to chime in
16:33:11 sean-k-mooney filterign properties is one thing
16:33:22 sean-k-mooney per property multiplers is another
16:33:54 sean-k-mooney that the bit that im not really fond of althogh im not super happy with either
16:34:09 dansmith well 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:24 dansmith the ask will be to make something important (os type) more important than something less (numa, or disk or something)
16:34:36 sean-k-mooney ya
16:35:01 sean-k-mooney so ignoring the compelxity this was ment to be a very simple weigher with littel overhead
16:35:23 sean-k-mooney i.e. you could enable it with very littel conisdiertation to the schduilign performace impact
16:35:25 dansmith I think the question is not about just simplicity, but also how well it works
16:36:03 dansmith the 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)

Earlier   Later