Skip to content

Commit cf81d29

Browse files
lexfreimyasnikovdaniil
lexfrei
authored and
myasnikovdaniil
committed
fix(tenant): retry failed etcd and seaweedfs installs in place (#3630)
<!-- 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]>
1 parent 831b670 commit cf81d29

3 files changed

Lines changed: 162 additions & 0 deletions

File tree

‎packages/apps/tenant/templates/etcd.yaml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,40 @@ spec:
2828
namespace: cozy-system
2929
interval: 5m
3030
timeout: 30m
31+
{{- /* Flux's default install strategy remediates a failed install by
32+
uninstalling, and install remediation is an uninstall in every case
33+
Flux offers. This release does not disable hooks on it, so that
34+
uninstall runs the etcd chart's post-delete cleanup Job, which deletes
35+
the data-etcd-<ordinal> PVCs by name because the etcd-operator does not
36+
reclaim them when the EtcdCluster goes. An install that misses its
37+
budget therefore destroys the datastore the tenant control planes keep
38+
their state in, and the next attempt starts from an empty cluster
39+
rather than resuming the one already converging. RetryOnFailure keeps
40+
the manifests applied and retries the failed release, so no uninstall
41+
happens and the cleanup Job never runs. Both actions carry the strategy
42+
because the controller performs that retry as an upgrade, so on install
43+
alone the retry's own failure falls through to upgrade remediation.
44+
Two things are traded away. A genuine upgrade failure now retries
45+
rather than rolls back, which gives up no stable safety net: with
46+
retries: -1 and the default strategy, upgrade remediation already
47+
rolled back and retried without end. And the uninstall was the only
48+
automatic clean slate, so an install wedged on a bad resource state
49+
retries against that state until an operator removes the release. For
50+
a release that owns data that is the right exchange, because lost data
51+
does not come back and a wedged install does. The remediation blocks
52+
stay because an absent release consults
53+
install and an out-of-sync one consults upgrade, both by retry count
54+
rather than by strategy. */}}
3155
install:
56+
strategy:
57+
name: RetryOnFailure
58+
retryInterval: 30s
3259
remediation:
3360
retries: -1
3461
upgrade:
62+
strategy:
63+
name: RetryOnFailure
64+
retryInterval: 30s
3565
remediation:
3666
retries: -1
3767
valuesFrom:

‎packages/apps/tenant/templates/seaweedfs.yaml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,40 @@ spec:
1919
namespace: cozy-system
2020
interval: 5m
2121
timeout: 10m
22+
{{- /* Flux's default install strategy remediates a failed install by
23+
uninstalling, and install remediation is an uninstall in every case
24+
Flux offers. This release does not disable hooks on it, so that
25+
uninstall runs the seaweedfs chart's post-delete cleanup Job, which
26+
deletes the volume PVCs by label and explicitly reclaims the
27+
keep-protected CNPG Cluster holding the filer metadata, precisely
28+
because nothing else removes it. An install that misses its budget
29+
therefore destroys the object store's data and the metadata database
30+
that indexes it, and the next attempt starts from nothing rather than
31+
resuming. RetryOnFailure keeps the manifests applied and retries the
32+
failed release, so no uninstall happens and the cleanup Job never runs.
33+
Both actions carry the strategy because the controller performs that
34+
retry as an upgrade, so on install alone the retry's own failure falls
35+
through to upgrade remediation. Two things are traded away. A genuine
36+
upgrade failure now retries rather than rolls back, which gives up no
37+
stable safety net: with retries: -1 and the default strategy, upgrade
38+
remediation already rolled back and retried without end. And the
39+
uninstall was the only automatic clean slate, so an install wedged on a
40+
bad resource state retries against that state until an operator removes
41+
the release. For a release that owns data that is the right exchange,
42+
because lost data does not come back and a wedged install does. The
43+
remediation blocks stay because an absent release consults install and
44+
an out-of-sync one consults upgrade, both by retry count rather than by
45+
strategy. */}}
2246
install:
47+
strategy:
48+
name: RetryOnFailure
49+
retryInterval: 30s
2350
remediation:
2451
retries: -1
2552
upgrade:
53+
strategy:
54+
name: RetryOnFailure
55+
retryInterval: 30s
2656
remediation:
2757
retries: -1
2858
valuesFrom:

‎packages/apps/tenant/tests/volume_owner_install_retry_test.yaml‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
suite: etcd and seaweedfs tenant HelmReleases retry a failed install in place
2+
3+
# Named after the two releases rather than after the class they belong to. The
4+
# tenant's monitoring release owns storage as well and its own post-delete Job
5+
# deletes it, and that release is outside this suite, so a class-shaped title
6+
# would print as PASS over members it never looked at.
7+
#
8+
# Restrict rendering to the two templates asserted on, so a template this suite
9+
# makes no claim about cannot fail it. That is not hypothetical: at the time of
10+
# writing, dropping the restriction reds all four tests on keycloakgroups.yaml,
11+
# which errors with "index of untyped nil" on the `.Values._cluster` the
12+
# platform injects and this suite has no reason to set. Adding a template to
13+
# the list below is safe.
14+
templates:
15+
- templates/etcd.yaml
16+
- templates/seaweedfs.yaml
17+
18+
release:
19+
name: tenant-test
20+
namespace: tenant-test
21+
22+
set:
23+
etcd: true
24+
seaweedfs: true
25+
host: ""
26+
27+
tests:
28+
# Install remediation is an uninstall in every case Flux offers, and neither
29+
# release disables hooks on it, so a remediated install runs each chart's
30+
# post-delete cleanup Job. Those Jobs delete data on purpose: the etcd one
31+
# removes the data-etcd-<ordinal> PVCs by name, because the etcd-operator does
32+
# not reclaim them when the EtcdCluster goes. An install that misses its
33+
# budget therefore destroys the datastore the tenant control planes keep their
34+
# state in, rather than merely restarting.
35+
- it: retries a failed etcd install instead of deleting the datastore
36+
asserts:
37+
- template: templates/etcd.yaml
38+
equal:
39+
path: spec.install.strategy.name
40+
value: RetryOnFailure
41+
- template: templates/etcd.yaml
42+
equal:
43+
path: spec.install.strategy.retryInterval
44+
value: 30s
45+
46+
# The seaweedfs cleanup Job deletes the volume PVCs by label and explicitly
47+
# reclaims the keep-protected CNPG Cluster holding the filer metadata, which
48+
# nothing else removes. Same consequence for a remediated install.
49+
- it: retries a failed seaweedfs install instead of deleting the filer metadata
50+
asserts:
51+
- template: templates/seaweedfs.yaml
52+
equal:
53+
path: spec.install.strategy.name
54+
value: RetryOnFailure
55+
- template: templates/seaweedfs.yaml
56+
equal:
57+
path: spec.install.strategy.retryInterval
58+
value: 30s
59+
60+
# The controller performs the retry of a failed install as an upgrade, so
61+
# without a strategy here that retry's own failure falls through to upgrade
62+
# remediation instead of retrying again.
63+
- it: carries the strategy on the upgrade action the retry runs as
64+
asserts:
65+
- template: templates/etcd.yaml
66+
equal:
67+
path: spec.upgrade.strategy.name
68+
value: RetryOnFailure
69+
- template: templates/etcd.yaml
70+
equal:
71+
path: spec.upgrade.strategy.retryInterval
72+
value: 30s
73+
- template: templates/seaweedfs.yaml
74+
equal:
75+
path: spec.upgrade.strategy.name
76+
value: RetryOnFailure
77+
- template: templates/seaweedfs.yaml
78+
equal:
79+
path: spec.upgrade.strategy.retryInterval
80+
value: 30s
81+
82+
# The strategy governs a failed release. An absent release and an out-of-sync
83+
# one take different branches, which consult the retry count instead and stop
84+
# acting once it is exhausted.
85+
- it: keeps unlimited retries for the branches the strategy does not reach
86+
asserts:
87+
- template: templates/etcd.yaml
88+
equal:
89+
path: spec.install.remediation.retries
90+
value: -1
91+
- template: templates/etcd.yaml
92+
equal:
93+
path: spec.upgrade.remediation.retries
94+
value: -1
95+
- template: templates/seaweedfs.yaml
96+
equal:
97+
path: spec.install.remediation.retries
98+
value: -1
99+
- template: templates/seaweedfs.yaml
100+
equal:
101+
path: spec.upgrade.remediation.retries
102+
value: -1

0 commit comments

Comments
 (0)