mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 18:47:04 +09:00
myasnikovdaniil
7520967d8d
## Why
The `cozystack-operator` Package reconciler builds every `HelmRelease`
it generates with a handful of hardcoded values. Three of those have
caused recurring pain across the project:
- **`Spec.Interval = 5m`** (line 220 before this PR) — too long for E2E
install where dependent HRs reconcile out of order; previously addressed
by a manual `flux reconcile --force` workaround in
`hack/e2e-install-cozystack.bats`.
- **`Install.Remediation{Retries: -1}` + `Upgrade.Remediation{Retries:
-1}`** — semantically "retry forever, never remediate", but the
retry-on-failure timing was tied to `Spec.Interval` (5m). Audit of the
last 30 successful PR runs found that **every** run hit `kubectl wait
hr/seaweedfs-system` timeouts at the 2-min mark, recovering only after
the inline `flux reconcile --force` workaround.
- **`Install.Timeout = 10m` / `Upgrade.Timeout = 10m`** — directly
contradicts the per-Application timeout work in `pkg/config/config.go` +
`pkg/registry/apps/application/rest.go` (commit `7b146cbe`).
`packages/apps/tenant/templates/etcd.yaml` hand-rolls its HR YAML to
bypass this for cert rotation; harbor has had multiple commits chasing
the same problem.
- **`Spec.MaxHistory`** unset → Helm default 5. Test clusters bounce HRs
frequently and accumulate release-history Secrets.
This PR exposes all four as operator flags + helm chart values, with
defaults that preserve current behavior.
## What
### New operator flags
| Flag | Default | HR field |
|---|---|---|
| `--helmrelease-interval` | `5m` | `Spec.Interval` |
| `--helmrelease-retry-interval` | `30s` |
`Spec.{Install,Upgrade}.Strategy.RetryInterval` |
| `--helmrelease-install-timeout` | `10m` | `Spec.Install.Timeout` |
| `--helmrelease-upgrade-timeout` | `10m` | `Spec.Upgrade.Timeout` |
| `--helmrelease-max-history` | `5` | `Spec.MaxHistory` |
### Strategy switch
`Install.Strategy.Name` and `Upgrade.Strategy.Name` are now set to
`RetryOnFailure` (instead of leaving the default `RemediateOnFailure`),
with `RetryInterval` set from the new flag. Functionally equivalent to
the previous `Remediation{Retries:-1}` ("retry forever") since
remediation never fired anyway, but **decouples retry-on-failure timing
from `Spec.Interval`** so failed releases recover at 30s without polling
healthy releases at the same fast cadence.
### Chart exposure
`packages/core/installer/values.yaml` gets five new optional values
(`cozystackOperator.helmRelease{Interval,RetryInterval,InstallTimeout,UpgradeTimeout,MaxHistory}`),
each defaulting to empty so the operator's own default applies. The
chart conditionally renders the corresponding flag only when the value
is set.
## Production safety
- All static defaults match the previous hardcoded values (5m / 10m /
10m / 5). No new code path runs until an admin explicitly sets a flag.
- **One intentional behavior change: failed-release retry cadence**
drops from ~5m (previously coupled to `Spec.Interval`) to 30s
(`RetryInterval` default). This is the whole motivation for the strategy
switch — the recurring `kubectl wait hr/seaweedfs-system` flakes
documented above were caused by the previous coupling. Healthy-release
reconcile cadence is unchanged at 5m in production; CI overrides this to
30s via `--set cozystackOperator.helmReleaseInterval=30s` in
`hack/e2e-install-cozystack.bats` for faster convergence on E2E
clusters.
- `RetryOnFailure` is functionally equivalent to `RemediateOnFailure +
Retries:-1` since neither ever triggers remediation.
- Empty value in the chart → flag not rendered → operator falls back to
its built-in default. (`helmReleaseMaxHistory: 0` — unlimited history
per Helm semantics — is also honored; the chart guards on `ne (toString
…) ""` rather than truthiness so int `0` is not silently dropped.)
## Verification
- `go build ./...` clean
- `go test ./internal/operator/...` passes (covers `Interval`,
`MaxHistory`, `Install/Upgrade.Timeout`, `Strategy.Name`,
`RetryInterval`, `ChartRef` propagation, plus a regression test for
`MaxHistory=0`)
- `helm template installer packages/core/installer` renders cleanly with
default and overridden values; `--set
cozystackOperator.helmReleaseMaxHistory=0` correctly emits
`--helmrelease-max-history=0`
## Followups (deliberately not bundled)
- **Per-component timeout overrides** on `ComponentInstall` (mirroring
the `UpgradeCRDs` pattern from #2427). Charts like etcd that need a 30m
timeout would set it on their `Component` rather than tuning a global
flag. Touches the Package CR API + codegen — bigger lift, separate PR.
- **`Remediation.Retries` as a flag.** No current pain; skip.
- **`createOrUpdateHelmRelease` field preservation.** Currently
preserves `Suspend` but not `Test`/`Rollback`/`Uninstall`. Would
silently overwrite an SRE's manual edit. Minor cleanliness, separate PR.
## Test plan
- [ ] CI passes E2E with `helmReleaseInterval=30s` set in install bats
(existing T1 test path)
- [ ] No regression on production install path (default values)
- [ ] HR objects in a healthy cluster render with
`Strategy.Name=RetryOnFailure` and `RetryInterval=30s`
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Operator and Helm chart now accept CLI flags and Helm values to
configure HelmRelease reconciliation intervals, retry behavior,
install/upgrade timeouts, and history retention.
* **Bug Fixes**
* Operator now validates HelmRelease timing/history inputs on startup
and will exit if values are invalid or non-positive.
* **Tests**
* Added unit and e2e coverage to verify HelmRelease configuration and a
chart install path using a custom reconciliation interval.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->