| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-02-12 | |||
| 14:00:56 | opendevreview | ribaudr proposed openstack/nova-specs master: Amend the specification about the data in LiveMigrate object https://review.opendev.org/c/openstack/nova-specs/+/941417 | |
| 14:38:05 | bauzas | dansmith: 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.html | |
| 14:38:22 | dansmith | bauzas: for the dict-based one yes, that's my point | |
| 14:38:25 | bauzas | so I need to provide another method name | |
| 14:38:34 | dansmith | if this a dict-compat object? if so you already have items() :) | |
| 14:38:37 | bauzas | dansmith: see, this is calling my own method | |
| 14:38:54 | bauzas | instead of the original dict() one | |
| 14:38:59 | bauzas | items() one | |
| 14:39:23 | dansmith | why can't you use the base one? | |
| 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 | sean-k-mooney | filterign properties is one thing | |
| 16:33:11 | bauzas | feel free to chime in | |