Repository navigation
Description
PR #10389 fixed a bug where instance_update sagas could not proceed because instance_delete_dpd_config() would return an error if the attempt to update the network configuration in either switch encountered an error, making the entire saga fail. I noticed that the delete_dpd_config_by_entry() function also calls notify_dendrite_nat_state() and returns any Err from that function, just like what instance_delete_dpd_config() was doing prior to this change:
omicron/nexus/src/app/instance_network.rs
Line 986 in 3681845
| notify_dendrite_nat_state(datastore, log, resolver, opctx_alloc, None).await |
I thought that this also seemed sketchy, so I went to check the the one place where this function is used, an unwinding action in the instance_ip_attach saga. At first glance, this looked scary, because we map_err it into an ActionFailed error, which made me worry that the saga might get stuck if it unwinds while a switch is down...but then I noticed that after we map it into an ActionFailed error, we just log a warning and return Ok(()):
omicron/nexus/src/app/sagas/instance_ip_attach.rs
Lines 243 to 250 in cb99c45
| if let Err(e) = osagactx | |
| .nexus() | |
| .delete_dpd_config_by_entry(&opctx, &nat_entry) | |
| .await | |
| .map_err(saga_action_failed) | |
| { | |
| error!(log, "siia_nat_undo: failed to notify DPD: {e}"); | |
| } |
So, I guess that's fine, even though I had a brief moment of panic. I kind of feel like maybe we should nonetheless change delete_dpd_config_by_entry to not return an error if the notify_dendrite_nat_state() call fails, and log it instead, so that we can have a convention that all the instance_networking functions don't return an error if they can't talk to a switch zone. This wouldn't actually change any current behavior but it would make me feel less worried about a future change using delete_dpd_config_by_entry() someplace else and slap a ? on it. Maybe that's worth doing in a separate branch?
Originally posted by @hawkw in #10389 (comment)
Activity