fix(opensearch-operator): gate leader election on replica count - #3040

…reate

#3040 set the single-replica operator Deployment to strategy.type:
Recreate to avoid two uncoordinated managers overlapping during a
rollout while leader election is gated off. That breaks Helm upgrades of
existing releases: the upgrade fails validation immediately,
helm-controller retries it (x19 over 11m in Actions run 29480724982),
and the HelmRelease never becomes Ready:

  Deployment.apps "opensearch-operator-controller-manager" is invalid:
  spec.strategy.rollingUpdate: Forbidden: may not be specified when
  strategy type is Recreate

The trigger is 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.
helm-controller applies server-side by default (Install.ServerSideApply
defaults to true, Upgrade.ServerSideApply to "auto"; the operator's
package_reconciler.go sets neither), and SSA does not remove a field the
applier never owned merely because the new intent omits it -- so sending
type: Recreate leaves the defaulted rollingUpdate in place and the
apiserver rejects the merged object.

The client-side path is unaffected: DeploymentSpec.Strategy carries
patchStrategy:"retainKeys", so a 3-way merge emits
{"$retainKeys":["type"],"type":"Recreate"} and does clear rollingUpdate.
That is why this surfaces through helm-controller's server-side apply
but not a local 'helm upgrade'.

Keep type: RollingUpdate and set rollingUpdate.maxSurge: 0 /
maxUnavailable: 1 on the single-replica path instead. maxSurge: 0 caps
total pods at replicas, so the old manager is scaled down before the new
one is scaled up, rather than starting a surge pod while the old manager
is still fully live as the default 25% would. It is not Recreate's
exclusivity -- the deployment controller orders the ReplicaSet desired
counts, not the pod lifecycles, so a draining old manager can still
overlap the new pod -- but it keeps the type transition legal on every
apply path, and recovers releases already on an rc build with Recreate.
This matches the existing kube-ovn idiom (central-deploy.yaml).

Also add a 'test' target to the package Makefile:
hack/helm-unit-tests.sh runs a package's suite only when its Makefile
defines one, so tests/leader_election_test.yaml had never executed.

Multi-replica installs keep the default RollingUpdate (leader election
on), unchanged. The change is carried in patches/leaderElection.diff so
it survives 'make update'.

Verified on kind v1.33.1: SSA of the v1.5.x manifest followed by SSA of
the Recreate manifest reproduces the error, while the same pair applied
client-side succeeds; SSA of the maxSurge:0 manifest succeeds both from
v1.5.x and from a live Recreate Deployment.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

…reate

#3040 set the single-replica operator Deployment to strategy.type:
Recreate to avoid two uncoordinated managers overlapping during a
rollout while leader election is gated off. That breaks Helm upgrades of
existing releases: the upgrade fails validation immediately,
helm-controller retries it (x19 over 11m in Actions run 29480724982),
and the HelmRelease never becomes Ready:

  Deployment.apps "opensearch-operator-controller-manager" is invalid:
  spec.strategy.rollingUpdate: Forbidden: may not be specified when
  strategy type is Recreate

The trigger is 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.
helm-controller applies server-side by default (Install.ServerSideApply
defaults to true, Upgrade.ServerSideApply to "auto"; the operator's
package_reconciler.go sets neither), and SSA does not remove a field the
applier never owned merely because the new intent omits it -- so sending
type: Recreate leaves the defaulted rollingUpdate in place and the
apiserver rejects the merged object.

The client-side path is unaffected: DeploymentSpec.Strategy carries
patchStrategy:"retainKeys", so a 3-way merge emits
{"$retainKeys":["type"],"type":"Recreate"} and does clear rollingUpdate.
That is why this surfaces through helm-controller's server-side apply
but not a local 'helm upgrade'.

Keep type: RollingUpdate and set rollingUpdate.maxSurge: 0 /
maxUnavailable: 1 on the single-replica path instead. maxSurge: 0 caps
total pods at replicas, so the old manager is scaled down before the new
one is scaled up, rather than starting a surge pod while the old manager
is still fully live as the default 25% would. It is not Recreate's
exclusivity -- the deployment controller orders the ReplicaSet desired
counts, not the pod lifecycles, so a draining old manager can still
overlap the new pod -- but it keeps the type transition legal on every
apply path, and recovers releases already on an rc build with Recreate.
This matches the existing kube-ovn idiom (central-deploy.yaml).

Also add a 'test' target to the package Makefile:
hack/helm-unit-tests.sh runs a package's suite only when its Makefile
defines one, so tests/leader_election_test.yaml had never executed.

Multi-replica installs keep the default RollingUpdate (leader election
on), unchanged. The change is carried in patches/leaderElection.diff so
it survives 'make update'.

Verified on kind v1.33.1: SSA of the v1.5.x manifest followed by SSA of
the Recreate manifest reproduces the error, while the same pair applied
client-side succeeds; SSA of the maxSurge:0 manifest succeeds both from
v1.5.x and from a live Recreate Deployment.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

…reate (#3319)

## 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 db797b1) 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](#3319 (review)).
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 -->