mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 18:47:04 +09:00
Aleksei Sviridkin
879d0f6db6
## What this PR does
The operator Deployment ran `replicas: 1` with `maxUnavailable: 1`, so
the Deployment controller took the minimum available replica count as `1
- 1 = 0` and the `Available` condition reported `True` with no running
pod at all. Measured against an operator whose image tag does not exist:
the condition was `True` with `reason: MinimumReplicasAvailable` while
the only pod sat in `ImagePullBackOff`, `unavailableReplicas: 1`, and no
`availableReplicas` key in the status.
That broke two separate things, so this fixes both.
The E2E install gate asked `kubectl wait --for=condition=Available` and
passed in 156 ms against that dead operator. The install then carried on
and died about two minutes later on the `packages.cozystack.io` CRD wait
further down the file, which sends you after CRD installation when the
operator never started. It now uses `kubectl rollout status`, which
reads updated and available replicas directly, and dumps the deployment,
pods, operator logs and namespace events when it fails. This half is
deliberately independent of the chart: `rollout status` is correct under
any rollout strategy, so a later change to that strategy cannot quietly
make the gate vacuous again.
The chart is the other half, and fixing the gate does not cover it.
`Available` is read by more than this test, and until the strategy
changes an alert on that condition, a health dashboard, or a person
reading `kubectl get deploy` all show green while the operator is dead.
The strategy moves to `maxSurge: 1` / `maxUnavailable: 0`, which
restores the ordinary meaning of the condition. Kubernetes refuses
`maxSurge: 0` and `maxUnavailable: 0` together, so the surge has to move
for the unavailability to reach zero.
That pair is worth reading twice, because it is not a configuration I
picked. At `replicas: 1` the Kubernetes default of `25%` / `25%`
resolves to exactly `maxSurge: 1` and `maxUnavailable: 0`, since the
surge rounds up and the unavailability rounds down. What this PR does is
stop overriding the default, not introduce a tuning of its own, and the
risk of the change should be judged on that basis.
Moving the surge is safe here, and I checked rather than assumed,
because the pair came from somewhere. It was carried over from the
earlier `cozystack` Deployment, where a sidecar published host port 8123
and `hostNetwork` left the surge pod unschedulable on a single-node
cluster. It got here in two steps, which is worth knowing if you go
looking for the reason: by the commit before this file existed the
sidecar was already gone and no port was declared, yet the pair stayed.
It had outlived its own reason before it was ever copied, which is why
there is no justification to find in this file's own history. On a
single-node cluster the surge pod is scheduled next to the outgoing one
and the rollout completes; adding a declared container port reproduces
the original `didn't have free ports for the requested pod ports`
failure exactly. Nothing else in the pod holds a port either: metrics
and health probes are bound to `0`, and the configured webhook server
never starts because the operator registers no webhook and never asks
the manager for the server.
The surge makes leader election load-bearing, though it covers less of
the overlap than it first appears. While `maxSurge: 0` capped total pods
at `replicas` a second manager process could not exist, so
`--leader-elect=true` was defence in depth. With a surge pod the lease
is what keeps the incoming manager's reconcile loops off the cluster
while the outgoing one is still leader, and because there is no
readiness probe the incoming pod counts as available as soon as its
container starts, so that overlap covers the whole image pull. It does
not cover everything: the operator installs CRDs, installs Flux and
creates the platform PackageSource before `mgr.Start()`, and the lease
is only taken inside `mgr.Start()`, so those three privileged installs
run outside leader election entirely. They are safe because both pods
write the same objects through server-side apply under the same field
manager, which is a different guarantee from the lease. The
`k8s-await-election` wrapper this operator replaced did hold the whole
process until it won leadership, so if that property sounds familiar it
belongs to the predecessor rather than to this binary.
The sibling `opensearch-operator` chart turns leader election off at one
replica for a real reason: a transient lease-renewal blip makes
controller-runtime exit the process, which at one replica reads as a
crashlooping Deployment. This operator installs first, when the
apiserver is at its shakiest, so that scenario applies here too. I am
accepting the risk knowingly, because after `maxSurge: 1` the
alternative is two managers reconciling the same cluster, which is
worse. The flag is pinned in the unit test so it cannot be turned off
without the test going red.
One consequence is permanent, so I am writing it down now instead of
leaving it to be found in a year. Because any bound port breaks the
surge pod under `hostNetwork`, an `httpGet` readiness probe is off the
table for this Deployment for good and only an `exec` probe could ever
work. So `Available` still answers "a container started", not "the
operator is healthy": an operator that starts and then dies partway
through installing CRDs is caught by the CRD waits below the gate, not
by the gate itself.
The surge also opens one new window. The operator installs CRDs and Flux
before the manager starts, which puts that work outside leader election,
so the incoming pod runs those applies while the outgoing pod is still
leader. Both apply the same objects server-side and the outgoing pod
exits moments later.
`--wait` is dropped from the helm step in the same change. Helm decides
a Deployment is ready with the same `replicas - maxUnavailable`
arithmetic, so on the old strategy it was satisfied at zero ready pods
and gated nothing, which is why the broken install got past it as well.
Once the strategy is honest it would gate for real, but on its own 2m
budget, and it would fail the install before the rollout gate ran and
hide the diagnostics behind a bare Helm timeout. Hooks are waited for
with or without the flag, and the release otherwise carries only the
Deployment and its RBAC objects, so the rollout gate is the better
single place to decide the operator is up.
A helm unittest suite pins the strategy and all four preconditions the
surge makes load-bearing: no container or initContainer declares a port,
both listener arguments stay disabled, leader election stays on, and no
probe uses `httpGet`. That last one follows from the two disabled
listeners leaving no port for a probe to reach, and it is the worst of
the four to get wrong, because the others wedge an upgrade while a
`livenessProbe` against a disabled listener crashloops the pod on every
cluster, fresh install included. Only the `httpGet` field is forbidden
rather than the probes themselves, so an `exec` probe stays available.
All four run against all three chart variants, because the template
branches on variant inside the container spec and a port added under one
branch would render for that variant alone.
I checked the suite fails for the right reasons instead of trusting it.
Reverting the strategy, declaring a `containerPort` on the operator
container, declaring one under a single variant branch, declaring one on
an initContainer, pointing a bind-address at a real address, turning
leader election off, and adding a readiness, liveness or startup probe
with `httpGet` each turns it red on its own assert. Inserting a
container ahead of the operator trips the container-name pin before the
argument asserts can start describing the wrong container. An `exec`
probe correctly stays green.
The rationale is split by audience. What an operator applying the
rendered manifest has to respect stays in plain comments and travels
into `_out/assets/cozystack-operator-*.yaml`: why the strategy is what
it is, that no port may be declared or bound, and that leader election
has to stay on. Everything that is only about this repository moves into
a `{{/* */}}` template comment that stops at the chart boundary. That
covers the pointer to the test and which assert pins what, where the
strategy was copied from, why the webhook server on 9443 is not a third
port binder, and why `opensearch-operator` reaches the opposite
conclusion on leader election. The other templates in this chart already
keep maintainer notes that way.
### Screenshots
Not applicable, no UI changes.
### Downstream repositories
I left the boxes empty on purpose rather than to skip the question,
because the honest answer is not one of them and someone else should
make the call.
Walking the trigger map in `docs/agents/contributing.md`, nothing fires.
The `hack/` triggers name `hack/e2e-prepare-cluster.bats`,
`hack/package.mk`, `hack/update-crd.sh` and moves or renames under
`hack/`, and this PR only edits the body of
`hack/e2e-install-cozystack.bats`. The `ansible-cozystack` trigger is
renaming an object the role waits on, and
`deployment/cozystack-operator` keeps its name.
The behaviour that role sees does change, though. It runs `kubectl wait
deployment/cozystack-operator --for=condition=Available`, the same
vacuous check this PR replaces here, so the chart change turns its wait
from one that returns instantly into one that means something. Its
`cozystack_operator_wait_timeout` defaults to 300 seconds, the same
budget I gave the gate here, so a cold image pull fits and I do not
think anything needs to change over there. No follow-up PR seemed
warranted for a change that only makes an existing check work, but that
is a judgement call rather than a trigger, which is why the boxes are
empty.
- [ ] 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(installer): the cozystack-operator Deployment no longer reports Available with zero running pods. It ran replicas: 1 with maxUnavailable: 1, which made the minimum available replica count 0, so the condition stayed True while the operator was dead and anything reading it, including the E2E install gate, saw green. The rollout strategy is now maxSurge: 1 / maxUnavailable: 0 and the install gate waits for the rollout instead of the condition.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved operator installation checks by monitoring deployment
rollouts and providing deployment, pod, log, and event details when a
rollout fails.
* Updated operator upgrades to maintain availability during rollout,
including in single-node environments.
* Documented rollout requirements and behavior across supported
installation configurations.
* **Tests**
* Added coverage for rollout settings and upgrade compatibility across
supported installation variants.
* Clarified Helm unit-test directory coverage documentation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->