Skip to content

fix(tenant): retry failed etcd and seaweedfs installs in place - #3630

Merged
Aleksei Sviridkin (lexfrei) merged 1 commit into
mainfrom
fix/tenant-volume-owners-retry-in-place
Aug 8, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 1 commit into
mainfrom
fix/tenant-volume-owners-retry-in-place

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

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's post-delete cleanup 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, and hack/e2e-install-cozystack.bats already 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.

RetryOnFailure keeps 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: -1 and 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

fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database

Summary by CodeRabbit

  • Reliability Improvements

    • Improved installation and upgrade recovery for etcd and SeaweedFS by automatically retrying failed operations every 30 seconds.
    • Preserved ongoing remediation attempts to help services recover without uninstalling or rolling back after failures.
  • Tests

    • Added coverage to verify retry behavior and remediation settings for both services.

@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files area/tenant Issues or PRs related to the tenant chart and multi-tenancy kind/bug Categorizes issue or PR as related to a bug labels Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cae1c14-1282-4091-8a6a-e8cf8a5b54da

📥 Commits

Reviewing files that changed from the base of the PR and between a71f16a and 6d403c3.

📒 Files selected for processing (1)
  • packages/apps/tenant/tests/volume_owner_install_retry_test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/apps/tenant/tests/volume_owner_install_retry_test.yaml

📝 Walkthrough

Walkthrough

The tenant etcd and SeaweedFS HelmReleases now retry failed installations and upgrades every 30 seconds with Flux RetryOnFailure. New Helm unit tests verify these strategies and preserve unlimited remediation retries.

Changes

Tenant HelmRelease retry behavior

Layer / File(s) Summary
HelmRelease retry configuration
packages/apps/tenant/templates/etcd.yaml, packages/apps/tenant/templates/seaweedfs.yaml
Installation and upgrade actions use RetryOnFailure with a 30-second interval. Unlimited remediation retries remain configured.
Retry strategy validation
packages/apps/tenant/tests/volume_owner_install_retry_test.yaml
Tests enable both releases and verify install and upgrade strategies, retry intervals, and unlimited remediation retries.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • Issue 3624 — Configures tenant HelmReleases to use RetryOnFailure instead of uninstall-based install remediation.

Possibly related PRs

Suggested labels: area/storage

Suggested reviewers: ivanhunters

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: failed etcd and SeaweedFS installations retry in place.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/tenant-volume-owners-retry-in-place

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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 <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/tenant-volume-owners-retry-in-place branch from a71f16a to 6d403c3 Compare August 7, 2026 23:26
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 36f83fa into main Aug 8, 2026
15 of 16 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the fix/tenant-volume-owners-retry-in-place branch August 8, 2026 11:04
myasnikovdaniil pushed a commit that referenced this pull request Oct 26, 2026
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## 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's `post-delete` cleanup 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`, and `hack/e2e-install-cozystack.bats` already 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.

`RetryOnFailure` keeps 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: -1` and 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

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database
```

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Reliability Improvements**
* Improved installation and upgrade recovery for etcd and SeaweedFS by
automatically retrying failed operations every 30 seconds.
* Preserved ongoing remediation attempts to help services recover
without uninstalling or rolling back after failures.

* **Tests**
* Added coverage to verify retry behavior and remediation settings for
both services.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

(cherry picked from commit 36f83fa)
Signed-off-by: Myasnikov Daniil <[email protected]>
myasnikovdaniil added a commit that referenced this pull request Nov 2, 2026
## What this PR does

Backports to release-1.6, picked from main with `cherry-pick -x` in
merge order. Each commit keeps the original message and its `(cherry
picked from commit …)` line.

- [#3581](#3581)
fix(kubernetes): retry the tenant CSI install instead of uninstalling it
- [#3621](#3621) fix(harbor):
retry the nested system install instead of uninstalling it
- [#3630](#3630) fix(tenant):
retry failed etcd and seaweedfs installs in place
- [#3735](#3735)
fix(vm-instance): disable helm readiness wait on the VMInstance release
- [#4799](#4799) fix: typo in
monitoring-agents values.yaml

### Conflicts resolved by hand

- [#3735](#3735): only
`pkg/config/config_test.go` conflicted. release-1.6 has no
`HelmServerSideApplyAnnotation`, so that test entry is left out, and two
comments in `pkg/config/config.go` and
`internal/controller/applicationdefinition_controller.go` are adjusted
to what exists on this line. The rest is the original as is.

Checked on this branch before opening: `helm template` of
`monitoring-agents` and `vm-instance-rd`, `go build`, `go vet`, `go
test` and the commit trailer check pass. Not run locally: `helm
unittest` (plugin not installed) and `helm template` of `harbor`,
`kubernetes` and `tenant`, which need values to render at base and head
alike. CI runs the unit suites.

### Screenshots

Not a UI change.

### Downstream repositories

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
fix(kubernetes): the tenant CSI HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant's CSI node plugin and its propagated StorageClasses away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible: a failed CSI upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release.

fix(harbor): retry a failed Harbor install instead of uninstalling the release, so a slow first install no longer deletes the Redis volume and starts over

fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database

fix(vm-instance): VMInstance HelmReleases no longer hang InProgress or roll back healthy VMs; the release no longer waits on VirtualMachine readiness, which helm-controller's kstatus poller cannot assess correctly for KubeVirt VMs. Existing VMInstance releases are brought in line automatically on upgrade.

fix: typo in monitoring-agents values.yaml
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tenant Issues or PRs related to the tenant chart and multi-tenancy kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant