| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-30 | |||
| 09:59:52 | bauzas | auniyal: not if you play with mutables | |
| 10:00:01 | bauzas | bdms is a list, right? | |
| 10:00:09 | auniyal | yes | |
| 10:04:05 | bauzas | auniyal: https://paste.opendev.org/show/bBaeGvpA8Bx5sSF3wKgZ/ | |
| 10:05:02 | bauzas | in my example, tamper_bdms is altering a mutable object, which is a list | |
| 10:05:20 | bauzas | that's semantically identical to your _delete_dangling_bdms() method | |
| 10:05:30 | bauzas | (provided you alter it) | |
| 10:19:42 | auniyal | bauzas, I might be wrong but this is what I tested - https://paste.opendev.org/show/bnMHd0uJuFYaSWSuP3Gu/ | |
| 10:20:44 | auniyal | the first obj was bdms, then I destroyed one of bdm from it, there is no change in bdms, then later when I retrieved new bdms list, it did not have deleted bdm | |
| 10:20:55 | bauzas | auniyal: because you haven't deleted the local copy | |
| 10:21:05 | auniyal | yes, | |
| 10:21:08 | bauzas | bdm.destroy() doesn't remove the object from the list | |
| 10:21:30 | bauzas | you somehow need to either pop it from the list, or just delete it by hand | |
| 10:21:37 | auniyal | yes, so the same I was trying to say, so instead of deleted their, we ca have fresh copy | |
| 10:22:12 | auniyal | will it affect performance | |
| 10:22:35 | bauzas | auniyal: sure, I don't disagree, but it's just doing an extra RPC roundtrip for some calculation we already made | |
| 10:23:12 | bauzas | auniyal: if you compare your new bdm list after the cleanup with the old one, you should in theory see that only the destroyed BDMs are left | |
| 10:23:33 | bauzas | auniyal: well, it's optimization | |
| 10:23:53 | bauzas | you know what? I'll switch my vote to +1 and I'll leave others chime in | |
| 10:28:44 | auniyal | if it may affect, after destroy I can remove bdm from local copy and return same one. | |
| 10:28:53 | auniyal | I think only one line change | |
| 10:39:18 | bauzas | you don't need to return anything, bdms is a mutable | |
| 10:39:46 | bauzas | take my pastebin as a reference | |
| 10:39:47 | auniyal | yes, just pass bdm, update the list after bdm destroy | |
| 11:14:06 | auniyal | bauzas, TypeError: 'BlockDeviceMappingList' object does not support item deletion, neither remove | |
| 11:14:38 | auniyal | it has these - https://paste.opendev.org/show/bnDCZUcY00oiKOYIreMs/ | |
| 11:16:34 | auniyal | its inherited from ObjectListBase which is again from ovoo_base.ObjectListBase | |
| 11:17:03 | auniyal | this is the oslo one, is there a way to del bdms obj with oslo ? | |
| 12:23:51 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 12:32:13 | auniyal | bauzas, I have updated the patch can you please have another look | |
| 12:46:22 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 13:43:00 | bauzas | auniyal: sec, finding a quick alternative | |
| 14:04:43 | bauzas | sorry, I needed to leave for a sec, back | |
| 14:20:12 | bauzas | dansmith: I'm just discussing with auniyal | |
| 14:20:53 | dansmith | about removing items from the BDM list? I think he resolved it no? | |
| 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 | sean-k-mooney | its not a rule no i just find it tend to make the code less intuitive to read | |
| 14:31:56 | dansmith | bauzas: yes and that's good | |
| 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 | |