| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-30 | |||
| 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. | |
| 18:12:18 | sean-k-mooney | the "and cinder has an attachment ID" part | |
| 18:13:10 | dansmith | what about it? | |
| 18:13:35 | sean-k-mooney | i was not expecting use to delete the BDM if the volume existiend but the atachment was gone | |
| 18:13:57 | dansmith | why not? that means it's been detached underneath us | |
| 18:14:10 | dansmith | (/me will go poof in a few minutes without warning) | |
| 18:14:10 | sean-k-mooney | whihc we dont allow so we shoudl repair it? | |
| 18:14:20 | sean-k-mooney | or error if you attached it to somethign else | |
| 18:14:36 | dansmith | we should heal ourselves and not require admin intervention | |
| 18:14:39 | dansmith | which is what is needed right now | |
| 18:15:20 | sean-k-mooney | in the case the volume is deleted i agree | |
| 18:15:35 | sean-k-mooney | im unconfrotabel with just the case wehre the attachment is deleted | |
| 18:15:44 | sean-k-mooney | perhaps that is valid | |
| 18:15:45 | dansmith | why? from our perspective it's the same | |
| 18:16:13 | dansmith | the state of the volume has been mucked with undereath us, so bail | |
| 18:16:31 | dansmith | don't try to hook it back up or recover in any other way than just avoiding our own problems in the future | |
| 18:16:40 | sean-k-mooney | so i woudl error in the attachment delete case personslly and say no you srewed with thsi you need ot manuall fix it | |
| 18:16:52 | dansmith | that's exactly what we're trying to avoid | |
| 18:17:30 | dansmith | because the user can't manually fix it | |
| 18:17:58 | dansmith | just like when we go to delete something and it's already deleted, we say "okay fair enough" and move on | |
| 18:18:16 | dansmith | these are distributed systems.. nova's DB could have been restored from ten minutes ago, etc | |
| 18:18:20 | sean-k-mooney | well the admin can with the attachemtn refesh command. i was debating if we shoudl do the refresh in the case the voluem is aviabale but i get you dont want that complexity | |
| 18:19:05 | sean-k-mooney | i guess the fact the attachment api needs service user | |
| 18:19:09 | sean-k-mooney | tokens now | |
| 18:19:26 | sean-k-mooney | fixes most of the concerns i have | |