| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-30 | |||
| 14:20:59 | bauzas | dansmith: I wonder why oslo.versionedobjects doesn't support directly deleting a list attribute | |
| 14:21:13 | bauzas | is it by design or just a miss ? | |
| 14:21:24 | dansmith | but I definitely agree with you that fetching again to refresh is not good, and I thought I made that point already, but that might have been *another* patch | |
| 14:21:28 | bauzas | we have many builtins but not this one | |
| 14:21:37 | dansmith | bauzas: I think it works by index, didn't he say? | |
| 14:21:57 | bauzas | dansmith: not exactly, see my comment : https://review.opendev.org/c/openstack/nova/+/882284 | |
| 14:21:59 | bauzas | shit | |
| 14:22:02 | bauzas | wrong url | |
| 14:22:08 | bauzas | dansmith: https://review.opendev.org/c/openstack/nova/+/882284/comment/8a1291a9_e0291cdc/ | |
| 14:22:42 | bauzas | I just simplified the loop | |
| 14:22:47 | sean-k-mooney | generally its preferable to not use del by the way | |
| 14:22:59 | bauzas | but, the fact is, we can access the index, but we can't directly del it | |
| 14:23:00 | sean-k-mooney | i know it works but it raises if its not present | |
| 14:23:24 | bauzas | so we need to access the somehow hidden objects internal field of the CoercedList objet | |
| 14:23:57 | bauzas | sean-k-mooney: I don't disagree on the general case, but in this case, I want to prevent an extra RPC roundtrip for something easy to cleanup | |
| 14:24:08 | sean-k-mooney | the bdms is a list of objects filed right | |
| 14:24:09 | sean-k-mooney | https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/fields.py#L1179-L1183 | |
| 14:24:10 | dansmith | bauzas: your suggestion is to modify the list while iterating right? that's not good. | |
| 14:24:44 | bauzas | dansmith: well, yeah you're right, it's a bad pattern | |
| 14:24:46 | sean-k-mooney | we should not need to fetch again | |
| 14:24:59 | sean-k-mooney | just recreate the bdms object wihtout the relevent bdms | |
| 14:25:07 | bauzas | okay, so I'll remove this comment and just ask auniyal to access the index the other wazy | |
| 14:26:06 | dansmith | the current approach that makes a list to remove and then does bdms.objects.remove() for each seems fine to kme | |
| 14:26:17 | sean-k-mooney | cant we effectivly jsut do a list comprehaent and create a new bdms object by iterating over the old one and filter out the things we dont want | |
| 14:26:28 | dansmith | yep, same same | |
| 14:27:07 | bauzas | dansmith: yeah, I'll remove my comment and just leave one comment which is a nit : we can iterate the bdms by 'for bdm in bdms' without requiring to iterate like he wrote with 'for bdm in bdms.objects' | |
| 14:27:26 | bauzas | there are many ways to do it | |
| 14:27:39 | bauzas | this is just a class python filtering over a list | |
| 14:27:44 | bauzas | classic* | |
| 14:28:37 | sean-k-mooney | honestly i woudl prefer if this funciton did not modify the passed in bdms object in place | |
| 14:28:46 | sean-k-mooney | and instead just constucted a new one and returned that | |
| 14:29:02 | dansmith | well, that's not very good either, | |
| 14:29:15 | dansmith | because it would need to re-associate the context | |
| 14:29:38 | bauzas | yeah | |
| 14:29:42 | sean-k-mooney | sorry trying to parse what tha tmeans | |
| 14:29:47 | dansmith | I don't remember if obj_clone() keeps the context, so if it does, then you could do that, but .. the point here is that it is _intending_ to modify the list | |
| 14:29:48 | dansmith | and save it | |
| 14:30:01 | bauzas | the workflow is : | |
| 14:30:08 | bauzas | 1/ call RPC to get the list | |
| 14:30:22 | bauzas | 2/ sanitize the list and eventually remove stale ones | |
| 14:30:24 | sean-k-mooney | right but we rarely use the fact that python supprot pass by pointer | |
| 14:30:36 | bauzas | 3/ do the rest of bdm checking that was existing | |
| 14:30:51 | sean-k-mooney | so the fact we are modifying an imput arge at all is kind of odd based on our normal code style | |
| 14:30:55 | bauzas | sean-k-mooney: well, I actually like python because of its mutables | |
| 14:31:11 | bauzas | but this is just a code pattern | |
| 14:31:27 | dansmith | I don't think it's a rule or even convention that we don't modify the input arguments in these RPC calls | |
| 14:31:39 | bauzas | I just wanted originally to prevent an unnecessary extra RPC roundtrip | |
| 14:31:55 | bauzas | dansmith: well, this is after the RPC call | |
| 14:31:56 | dansmith | bauzas: yes and that's good | |
| 14:31:56 | sean-k-mooney | its not a rule no i just find it tend to make the code less intuitive to read | |
| 14:32:24 | sean-k-mooney | as you cant tell form the call site if it modfies the object or not | |
| 14:32:33 | dansmith | sean-k-mooney: if we're going to modify it and save it, then modifying the copy we got makes more sense to me anyway, even though nobody else has a pointer to this actual instance other than us :) | |
| 14:33:22 | bauzas | n-cpu gets the list, then n-cpu reduces the list | |
| 14:33:27 | sean-k-mooney | i guess either works i just like the pattere of x = do_stuff(x) | |
| 14:33:37 | bauzas | then n-cpu can call as many saves as it wants | |
| 14:33:40 | sean-k-mooney | hostly either works | |
| 14:33:58 | sean-k-mooney | the only time i really care is if a funciton modifed inputs and returns something | |
| 14:34:15 | sean-k-mooney | i really dont like that partern but if it does one or the other its generally ok | |
| 14:34:16 | bauzas | sean-k-mooney: yeah and that's because your brain is set on functional programming while I'm more imperative :p | |
| 14:34:24 | sean-k-mooney | this only modifies the input | |
| 14:35:21 | sean-k-mooney | bauzas: not really i just have been bitten bug bugs where inputs were modifed in nova and we were not aware of it | |
| 14:35:52 | sean-k-mooney | im thinkign of the requst spec in the schudler specificaly | |
| 14:36:13 | bauzas | yeah, I do understand | |
| 14:36:18 | bauzas | here, it's a filtering case | |
| 14:36:55 | bauzas | but I'm fine with both approaches, we can ask amit to return the sanitized list | |
| 14:37:12 | bauzas | if you don't like the implicit approach of tampering a mutable | |
| 14:37:27 | bauzas | (which I do understand is somehow risky) | |
| 14:37:52 | sean-k-mooney | its not that its risky just less common in our code base | |
| 14:37:57 | sean-k-mooney | the end result | |
| 14:38:02 | sean-k-mooney | we want in ether case | |
| 14:38:05 | sean-k-mooney | is a single rpc | |
| 14:38:13 | bauzas | I just don't want us to push this feature under the FeatureFreeze bus because of a code pattern argument :) | |
| 14:38:16 | sean-k-mooney | so aslong as we get ther its fine | |
| 14:38:38 | bauzas | which is the case in the current latest PS | |
| 14:38:45 | bauzas | so, +2 | |
| 14:38:54 | sean-k-mooney | ack | |
| 14:56:10 | sean-k-mooney | elodilles: by the way the os-vif ci has been pretty green https://zuul.openstack.org/builds?project=openstack%2Fos-vif&skip=0 bu twe also dont see many executions there either | |
| 14:57:23 | sean-k-mooney | sounds like you are concerredn about hte os-vif-ovs-iptables job sepcifically which has had a few failures https://zuul.openstack.org/builds?job_name=os-vif-ovs-iptables&project=openstack%2Fos-vif | |
| 15:07:33 | elodilles | sean-k-mooney: yepp, os-vif-ovs-iptables failed 2 times on the generated 'ci health checking' patch, but i saw that it passed recently | |
| 15:08:19 | elodilles | i'm not really concerned, but wanted to see that the DNM patch is passing so that i can abandon it o:) | |
| 16:02:50 | sean-k-mooney | elodilles: i think this failrues were mainly related to volumes | |
| 16:03:02 | sean-k-mooney | so ya we will see what happens iwht the dnm | |
| 16:03:13 | sean-k-mooney | honestly we likely sould disable the volume tests in os-vif | |
| 16:04:24 | sean-k-mooney | its also possible that those test are not configured for verification | |
| 16:05:27 | sean-k-mooney | its not disabled expleictly however so i dont thin kthis is the issue | |
| 16:54:08 | bauzas | folks, I'm about to EOB, but I'd appreciate if some cores around may look in particular at https://review.opendev.org/c/openstack/nova/+/873221 , https://review.opendev.org/q/topic:dangling-volumes+project:openstack/nova+is:open and https://review.opendev.org/q/topic:ironic-shards+project:openstack/nova+is:open which both have my +2 (mostly, or just asking for nits) | |
| 16:54:38 | bauzas | as a reminder, FF is tomorrow and I'd very appreciate if we could somehow give a go to those series | |
| 16:59:12 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 17:01:06 | auniyal | bauzas, dansmith, sean-k-mooney can you please have another look dangling patches | |
| 17:07:46 | dansmith | bauzas: there are so many typos in that bottom ironic sharing patch including doc text and renos, I think it's worth a respin.. what do you think? | |
| 18:01:32 | sean-k-mooney | auniyal: im -1 on the reproducer https://review.opendev.org/c/openstack/nova/+/889947/6/nova/tests/functional/compute/test_attached_volumes.py | |
| 18:08:31 | sean-k-mooney | bauzas: dansmith im confilcited on what to do with the dangeleing volume seriese | |
| 18:09:07 | dansmith | I gotta run in a sec, but I don't think it's anything approaching critical | |
| 18:09:28 | dansmith | and it doesn't yet address concerns about the cinder volume being detached and reattached outside nova, AFAIK | |
| 18:09:46 | sean-k-mooney | right so i didnt think we wanted to suport that | |
| 18:09:55 | sean-k-mooney | this would actully enabel detach to work | |
| 18:10:04 | sean-k-mooney | albeit with a hard reboot | |
| 18:10:13 | sean-k-mooney | adn needing a service user token | |
| 18:10:57 | dansmith | I mean it doesn't catch that case, but should | |
| 18:11:54 | sean-k-mooney | so the bit im conflicted by is Verify if volume exists and cinder has an attachment ID else delete its BDMS data from nova DB and vice versa. | |