Don't fail active bundles with expunged Nexuses (reassign only) #9286
Conversation
| // If the bundle was marked "active", we need to | ||
| // re-assign the "owning Nexus" so it can later | ||
| // be deleted, but the bundle itself has already | ||
| // been stored on a sled. | ||
| // | ||
| // There's no need to fail it. | ||
| SupportBundleState::Active | | ||
| // If the bundle has already been marked | ||
| // "Destroying/Failing/Failed", it's already | ||
| // irreversibly marked for deletion. | ||
| SupportBundleState::Destroying | | ||
| SupportBundleState::Failing | | ||
| SupportBundleState::Failed => None, |
There was a problem hiding this comment.
indentation here feels weird but I guess that's what rustfmt wanted... i kinda wonder if it would be nicer to split the two patterns with separate comments, so it doesn't feel like the second comment about deleted bundles is indented "under" the first comment?
this is persnickety and it does not matter.
| // If the bundle was marked "active", we need to | |
| // re-assign the "owning Nexus" so it can later | |
| // be deleted, but the bundle itself has already | |
| // been stored on a sled. | |
| // | |
| // There's no need to fail it. | |
| SupportBundleState::Active | | |
| // If the bundle has already been marked | |
| // "Destroying/Failing/Failed", it's already | |
| // irreversibly marked for deletion. | |
| SupportBundleState::Destroying | | |
| SupportBundleState::Failing | | |
| SupportBundleState::Failed => None, | |
| // If the bundle was marked "active", we need to | |
| // re-assign the "owning Nexus" so it can later | |
| // be deleted, but the bundle itself has already | |
| // been stored on a sled. | |
| // | |
| // There's no need to fail it. | |
| SupportBundleState::Active => None, | |
| // If the bundle has already been marked | |
| // "Destroying/Failing/Failed", it's already | |
| // irreversibly marked for deletion. | |
| SupportBundleState::Destroying | | |
| SupportBundleState::Failing | | |
| SupportBundleState::Failed => None, |
| if !bundles_to_reassign.is_empty() { | ||
| // Reassign bundles that haven't been marked "fully failed" | ||
| // to ourselves, so we can free their storage if they have | ||
| // been provisioned on a sled. | ||
| report.bundles_reassigned = | ||
| diesel::update(dsl::support_bundle) | ||
| .filter(dsl::id.eq_any(bundles_to_reassign)) | ||
| .set( | ||
| dsl::assigned_nexus | ||
| .eq(our_nexus_id.into_untyped_uuid()), | ||
| ) | ||
| .execute_async(conn) | ||
| .await?; |
There was a problem hiding this comment.
took me a moment to realize that the only diff here is that we changed the if from an early return to just making the update query conditional --- but i agree that this feels less convoluted than the early return, especially if we wanted to add additional code that runs after this query later.
There was a problem hiding this comment.
yeah it felt like "I didn't need an extra comment to explain an early return", because it's self-explanatory that we'd only want to update rows if there are rows to be updated
Previously, the upgrade pathway (involving expunging and adding new Nexuses) would cause all support bundles to be marked "failed". For fully-collected bundles, this is not necessary, and fixed by this PR.
Fixes #9257