mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-29 16:17:20 +09:00
myasnikovdaniil
741a5acc4a
## What this PR does
Fixes a Helm-upgrade failure in the `opensearch-operator` chart that
blocks the new release-upgrade E2E lane (added in #3276).
Upgrading from a chart that shipped no explicit strategy (v1.5.x) to a
v1.6.0-rc build fails validation immediately, helm-controller retries
it, and the HelmRelease never becomes Ready:
```
Helm upgrade failed for release cozy-opensearch-operator/opensearch-operator:
Deployment.apps "opensearch-operator-controller-manager" is invalid:
spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate
```
Observed in the ["Upgrade E2E Test" job of run
29480724982](https://github.com/cozystack/cozystack/actions/runs/29480724982)
— `UpgradeFailed ... (x19 over 11m)`, with the HelmRelease never
reaching Ready.
### Root cause
#3040 (commit db797b1d) set the single-replica operator Deployment to
`strategy.type: Recreate` — a reasonable goal: with leader election
gated off on one replica, a default `RollingUpdate` briefly runs two
uncoordinated managers (`maxSurge`) during a rollout, and `Recreate`
closes that window.
The problem surfaces only on **upgrade from an older chart, under
server-side apply**:
- A v1.5.x release shipped no strategy at all, so the apiserver
**defaulted** `spec.strategy.rollingUpdate` to `25%/25%`, and the
`helm-controller` field manager never owned that block. Its
`managedFields` entry covers `f:replicas`, `f:selector` and
`f:template`, but not `f:strategy`.
- helm-controller applies **server-side** by default
(`Install.ServerSideApply` defaults to true, `Upgrade.ServerSideApply`
to `auto`; `internal/operator/package_reconciler.go` sets neither), and
SSA does not remove a field the applier never owned merely because the
new intent omits it.
- So the v1.6.0-rc manifest's `type: Recreate` merges onto a live object
that still carries the defaulted `rollingUpdate`, and the apiserver
rejects the result with `FieldValueForbidden`.
The **client-side** path is unaffected, which is why this surfaces
through helm-controller's SSA but not a local `helm upgrade`:
`DeploymentSpec.Strategy` carries `patchStrategy:"retainKeys"`
(`k8s.io/api@v0.34.1/apps/v1/types.go:395`), so a 3-way merge emits
`{"spec":{"strategy":{"$retainKeys":["type"],"type":"Recreate"}}}`,
which merges to a valid `{"strategy":{"type":"Recreate"}}` and does
clear the block.
This is a genuine product upgrade bug, not a test-harness issue — #3276
only adds the upgrade lane that exposes it; it does not touch
`opensearch-operator`.
> An earlier revision of this PR attributed the failure to Helm's
client-side 3-way merge. That was wrong, and the correction is
@lexfrei's — see [his
review](https://github.com/cozystack/cozystack/pull/3319#pullrequestreview-4713156867).
The mechanism above is now reproduced end to end (below).
### The fix
Keep `type: RollingUpdate` on the single-replica path and set
`rollingUpdate.maxSurge: 0 / maxUnavailable: 1` instead of switching to
`Recreate`:
- `maxSurge: 0` caps total pods at `replicas`, so the deployment
controller must scale the old manager down before it can scale the new
one up — unlike the default 25% surge, which starts a second manager
while the first is still fully live.
- This is **not** `Recreate`'s exclusivity, and the PR no longer claims
it is. `rolloutRecreate` gates scale-up on `oldPodsRunning()`, which
counts non-terminal `Status.Phase`, so `Recreate` waits for the old pod
to be gone. `rolloutRolling` has no such gate: `NewRSNewReplicas`
derives `currentPodCount` from `GetReplicaCountForReplicaSets`, which
sums `rs.Spec.Replicas` (desired, not live). The ordering that is
guaranteed is over ReplicaSet desired counts, not pod lifecycles, so a
draining old manager can still overlap the new pod within
`terminationGracePeriodSeconds` (10s here).
- Because `strategy.type` never becomes `Recreate`, the forbidden merge
cannot occur on any apply path. It also recovers anyone already on an rc
build with `Recreate`, since `Recreate -> RollingUpdate` is permitted.
- This matches the idiom already vendored in this repo:
`packages/system/kubeovn/charts/kube-ovn/templates/central-deploy.yaml:11-15`
uses `RollingUpdate` with `maxSurge: 0 / maxUnavailable: 1`.
Multi-replica installs (`manager.replicaCount > 1`) are unchanged: they
keep the default `RollingUpdate` with leader election enabled.
The change is carried in `patches/leaderElection.diff` so it survives a
chart re-vendor via `make update`; the patch applies to pristine
upstream 2.8.0 with no fuzz and reproduces the committed strategy block
exactly.
### Alternatives considered
| Option | Why not |
| --- | --- |
| Keep `Recreate`, but make the chart own `rollingUpdate` first (ship an
explicit block, then switch `type` in a later release) | This is the
only option that preserves real exclusivity, but it needs two releases
and only works once every install has reconciled the intermediate one.
Disproportionate for a single-replica operator whose residual overlap is
a draining manager. |
| Pre-upgrade hook / Job that patches or deletes the Deployment strategy
before the upgrade | Heavy: needs a kubectl image, a ServiceAccount and
RBAC to `patch`/`delete` Deployments, and hook ordering; runs on every
upgrade even when unneeded; adds standing attack surface —
disproportionate for a scalar field transition. |
| `helm.sh/resource-policy` / force-replace | `resource-policy: keep`
only governs deletion on uninstall (irrelevant). Helm has no
per-resource "force recreate" annotation; `--force` / HelmRelease
`spec.upgrade.force` is global and disruptive and not controllable from
within the chart. |
| Plain revert to default `RollingUpdate` (25%/25%) | Fixes the
rejection but starts a surge pod while the old manager is still fully
live — the window #3040 set out to close. `maxSurge: 0` narrows it to a
draining old pod without leaving `RollingUpdate`. |
### Verification
The mechanism and the fix were reproduced on kind v1.33.1 (a bare
apiserver is enough — the failure is defaulting + SSA + validation):
| Path | Result |
| --- | --- |
| SSA install of the v1.5.x manifest (no strategy) | apiserver defaults
`rollingUpdate: 25%/25%`; `managedFields` shows `helm-controller` does
not own `f:strategy` |
| SSA upgrade to `type: Recreate` | fails with the exact production
error above |
| Client-side apply of the same pair | succeeds; `rollingUpdate` is
cleared, live strategy becomes `{"type":"Recreate"}` |
| SSA upgrade to `maxSurge: 0` (this PR) | succeeds |
| SSA of `maxSurge: 0` onto a live `Recreate` Deployment (rc recovery) |
succeeds |
| Rollout under `maxSurge: 0` vs `Recreate` | `maxSurge: 0`: two pods
coexisted within 1s (old Terminating, new ContainerCreating).
`Recreate`: one pod, waited the full drain. This is the evidence for the
"not exclusivity" wording above. |
### Note on regression coverage
`tests/leader_election_test.yaml` was never executed:
`hack/helm-unit-tests.sh` runs a package's suite only when the package
Makefile defines a `test:` target, and this one did not — so the
coverage the previous revision of this PR claimed did not exist. This PR
adds the target (matching `packages/system/etcd-operator/Makefile`),
which revives the suite; the repo-wide runner now picks the package up
and a revert to `Recreate` fails it. Thanks to @lexfrei for catching
that.
The apiserver-side merge that produces the invalid object still needs a
live cluster, so the behavioural regression test remains the
release-upgrade E2E lane in #3276 (which this fix unblocks).
### Follow-ups filed
- #3324 — `packages/system/ouroboros` has the same latent shape
(`Recreate` gated on `controller.mode == "external-dns"`, while
`controller.mode` defaults to `coredns`). Not fixed here, and it likely
needs a different fix, since that mode genuinely depends on exclusivity.
- #3325 — `make update` does not reproduce this package's vendored
template: three hand-edits are not captured in `patches/`, and one of
them breaks rendering when `manager.extraEnv` is set. Pre-existing on
`main`; an earlier revision of this body claimed the regeneration was
exact, which was wrong.
### Downstream repositories
- [x] No downstream repository is affected by this change
<!-- Walked the trigger map in docs/agents/contributing.md against the
diff: the change touches only a vendored packages/system operator chart
template + its patch + its helm-unittest suite + the package Makefile
test target. No packages/apps or packages/extra add/rename/remove, no
values.schema.json / enum / default change, no node contract, no
annotations/labels, no hack/ or ApplicationDefinition change.
opensearch-k8s-operator is a third-party upstream (not a cozystack
downstream), and the fix lives in patches/leaderElection.diff so it
survives make update. -->
### Release note
```release-note
fix(opensearch-operator): fix a Helm upgrade failure on single-replica installs. Upgrading a release that predates an explicit Deployment strategy failed under server-side apply, because switching strategy.type to Recreate does not clear the apiserver-defaulted spec.strategy.rollingUpdate block that the chart never owned, and the merged object is rejected. The operator now uses RollingUpdate with maxSurge=0/maxUnavailable=1, which keeps the transition legal on every apply path and recovers releases already stuck on Recreate.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Updated OpenSearch Operator controller rollout behavior for
single-replica installs to use a safer rolling update configuration.
* Leader election is now applied only when multiple replicas are
configured, reducing the chance of conflicting controller activity.
* **Tests**
* Expanded Helm unittest assertions for the controller deployment
rollout strategy and leader-election behavior for single- and
multi-replica scenarios.
* **Chores**
* Added a `make test` target to run Helm unit tests.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->