| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-30 | |||
| 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 | |
| 18:19:50 | sean-k-mooney | as long as we are still very clear that if you detach a volume form cidner or a prot form neutorn your isntance is tainted | |
| 18:20:13 | sean-k-mooney | and all we are doing is minimal efffort to boot again | |
| 18:20:16 | sean-k-mooney | i guess that is ok | |
| 18:25:57 | sean-k-mooney | ill look at this again tomorooww but without the service user chagne i dont this woudl have been the correct approch to take. | |
| 18:35:11 | dansmith | sean-k-mooney: right the point in this is very specifically to *not* require the admin to intervene when the only thing they can reasonably do is run a command to do the same thing, which is eliminate the BDM from nova's side | |
| 18:35:22 | dansmith | we wouldn't want the admin to "fix" it (IMHO) | |
| 18:36:04 | dansmith | and especially with the cinder policy change, the only real thing that could cause this workflow-wise would be a database restore from checkpoint such that the two sides are out of sync, or an admin operation on the cinder side that just then needs to be mirrored with admin operations on the nova side | |
| 18:36:30 | dansmith | it's not to _enable_ any sort of bad behavior by the user, it's to avoid the user being stuck and unable to get their instance back | |
| 18:36:55 | dansmith | for self-service reasons, I'd much prefer my volume to just become detached and me still be able to work on my instance without waiting in an admin's queue | |
| 18:46:12 | opendevreview | Merged openstack/nova master: Add a new NumInstancesWeigher https://review.opendev.org/c/openstack/nova/+/886232 | |
| 19:23:52 | opendevreview | melanie witt proposed openstack/nova master: Add documentation for unified limits https://review.opendev.org/c/openstack/nova/+/893127 | |
| 19:23:52 | opendevreview | melanie witt proposed openstack/nova master: nova-manage: Add 'limits migrate_to_unified_limits' https://review.opendev.org/c/openstack/nova/+/891646 | |
| 19:34:05 | opendevreview | melanie witt proposed openstack/nova master: Add documentation for unified limits https://review.opendev.org/c/openstack/nova/+/893127 | |
| 19:36:11 | sean-k-mooney | dansmith: ok i can bye that. i kind of feel like in addtion to this we show have a external event form cidner saying its detach or something like that to make it properly automatic but again with the service token change that shoudl not be reuqired either. | |
| 19:36:46 | dansmith | to be clear, I do not think we should _ever_ tolerate detach via cinder, thus no need for an event | |
| 19:36:58 | dansmith | this is a corruption recovery mechanism and nothing else | |
| 19:36:59 | sean-k-mooney | so ill remove my -1 on the repoducer. i still have not looked at the actual final patch however | |
| 19:37:14 | sean-k-mooney | ack | |
| #openstack-nova - 2023-08-31 | |||
| 02:00:15 | opendevreview | melanie witt proposed openstack/nova master: Enable use of native threads in scatter_gather_cells https://review.opendev.org/c/openstack/nova/+/650172 | |
| 02:47:02 | opendevreview | Merged openstack/nova master: libvirt: Add 'COMPUTE_ADDRESS_SPACE_*' traits support https://review.opendev.org/c/openstack/nova/+/873221 | |
| 04:58:50 | opendevreview | OpenStack Proposal Bot proposed openstack/nova master: Imported Translations from Zanata https://review.opendev.org/c/openstack/nova/+/891648 | |
| 06:11:37 | opendevreview | Amit Uniyal proposed openstack/nova master: Reproducer for dangling bdms https://review.opendev.org/c/openstack/nova/+/889947 | |