fix(tenant): retry failed etcd and seaweedfs installs in place - #3630
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe tenant etcd and SeaweedFS HelmReleases now retry failed installations and upgrades every 30 seconds with Flux ChangesTenant HelmRelease retry behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The tenant chart renders both releases with Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither release disables hooks on it, so a remediated install runs each chart's post-delete cleanup Job, and those Jobs delete data on purpose. The etcd one removes the data-etcd-<ordinal> PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, precisely because nothing else removes it. Both are correct for a real uninstall. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not merely restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data this is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The chart's monitoring release destroys its storage on a remediated install as well and is outside this change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
a71f16a to
6d403c3
Compare
What this PR does
The tenant chart renders the etcd and seaweedfs releases with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither release sets
spec.uninstall.disableHooks, which defaults to false, so that uninstall runs the app chart'spost-deletecleanup Job. Nothing unusual is configured anywhere along that chain: fail early, uninstall, hooks.Those Jobs delete data on purpose. The etcd one removes the
data-etcd-<ordinal>PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, because nothing else would. Both are right for a real uninstall, and neither hook is at fault here. Remediation is what calls them, on a release that is only starting slowly.So an install that misses its budget does not restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing.
That budget is not a comfortable margin. The seaweedfs release has
timeout: 10m, andhack/e2e-install-cozystack.batsalready records that on a loaded runner the tenant stack only starts creating pods around 9 to 10 minutes in. The window CI observes and the window that triggers this are the same window.RetryOnFailurekeeps the manifests applied and retries the failed release as an upgrade. No uninstall happens, so neither cleanup Job runs and the install already under way carries on instead of starting over.Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy.
One trade is that a real upgrade failure retries instead of rolling back. That costs no stable safety net. With
retries: -1and the default strategy, upgrade remediation was already rolling back and retrying without end, so the release never rested in the rolled-back state either.The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data that is the right exchange, because lost data does not come back and a wedged install does.
This covers two of the tenant's data-owning releases. The monitoring release carries the same default and a cleanup Job that deletes its PVCs as well, and is left for its own change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Same shape as #3552 and #3581. Relates to #3555 and #3576.
Screenshots
Downstream repositories
Release note
Summary by CodeRabbit
Reliability Improvements
Tests