fix(installer): make operator readiness honest in the chart and the install gate (#3466)

## 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 -->
This commit is contained in:
Aleksei Sviridkin authored and GitHub committed 2026-08-06 15:25:39 +03:00
commit 879d0f6db6
4 files changed
+297 -8

No files matched your search

+33 -3
View File
@@ -117,7 +117,6 @@
--namespace cozy-system \
--create-namespace \
--set cozystackOperator.helmReleaseInterval=30s \
--wait \
--timeout 2m
# The pre-install hook (cozy-system-labeler) must have stamped the PSA and
@@ -127,8 +126,39 @@
kubectl get ns cozy-system -o jsonpath='{.metadata.labels.pod-security\.kubernetes\.io/enforce}' | grep -qx privileged
kubectl get ns cozy-system -o jsonpath='{.metadata.labels.cozystack\.io/system}' | grep -qx true
# Verify the operator deployment is available
kubectl wait deployment/cozystack-operator -n cozy-system --timeout=1m --for=condition=Available
# Wait for the ROLLOUT, not for condition=Available: once maxUnavailable
# reaches replicas the minimum available replica count is 0, so Available
# reports True with no pod running and a broken operator image passes in
# milliseconds, surfacing minutes later as a CRD timeout that blames the wrong
# component. `rollout status` reads updated and available replicas directly,
# and stays correct whatever strategy the chart carries.
#
# It still only proves a container started, not that the operator is healthy:
# there is no readiness probe, so one that starts and then dies mid-install
# passes here and is caught by the CRD waits below instead.
#
# This is the only readiness gate for the operator, since the helm step above
# drops --wait, so the timeout must cover scheduling and a cold pull of an
# image that is not in the prepull set.
kubectl rollout status deployment/cozystack-operator -n cozy-system --timeout=5m || {
echo "=== deployment/cozystack-operator rollout did not complete ==="
kubectl get deployment/cozystack-operator -n cozy-system -o yaml 2>&1 || true
echo "=== cozy-system pods ==="
kubectl get pods -n cozy-system -o wide 2>&1 || true
# The two shapes this gate exists to catch need different evidence, and
# neither is in `get pods -o wide`. ImagePullBackOff produces no logs at
# all, and CrashLoopBackOff puts the stack trace in the container that
# already died, so describe carries the first and --previous the second.
echo "=== cozystack-operator pod detail ==="
kubectl describe pod -n cozy-system -l app=cozystack-operator 2>&1 || true
echo "=== cozystack-operator logs ==="
kubectl logs -n cozy-system -l app=cozystack-operator --all-containers --tail=50 2>&1 || true
echo "=== cozystack-operator logs, previous container ==="
kubectl logs -n cozy-system -l app=cozystack-operator --all-containers --previous --tail=50 2>&1 || true
echo "=== cozy-system events ==="
kubectl get events -n cozy-system --sort-by=.lastTimestamp 2>&1 | tail -30 || true
false
}
# Wait for operator to install CRDs (happens at startup before reconcile loop).
# kubectl wait fails immediately if the CRD does not exist yet, so poll until it appears first.
+5 -3
View File
@@ -2,9 +2,11 @@
set -eu
# Script to run unit tests for all Helm charts.
# It iterates through directories in packages/apps, packages/extra,
# packages/system, and packages/library and runs the 'test' Makefile
# target if it exists.
# It iterates through directories in packages/apps, packages/core,
# packages/extra, packages/system, and packages/library and runs the 'test'
# Makefile target if it exists. Keep this list in step with the loop below:
# packages/core carries suites of its own, so dropping it to match a stale
# comment would silently stop running them.
FAILED_DIRS_FILE="$(mktemp)"
trap 'rm -f "$FAILED_DIRS_FILE"' EXIT
@@ -1,3 +1,86 @@
{{/*
Maintainer notes on the rollout strategy. The operator-facing constraints it
implies are in plain # comments further down, because those ship to whoever
applies the rendered _out/assets/cozystack-operator-*.yaml. Everything here
is about this repository and stays inside the template.
tests/rollout_strategy_test.yaml pins the strategy values and the four
preconditions below against every variant the chart ships, all on one shared
anchor, so gating any of them on a variant is caught by the variants it was
not gated to. Three preconditions are pinned airtight; precondition 2 is not,
and says where the gap is rather than leaving the reader to assume coverage
that is not there:
1. No container or initContainer declares a port. Asserted with a
containers[*] wildcard rather than containers[0], because the constraint
binds every container in the pod and not only the first: under hostNetwork
a declared containerPort defaults to the same hostPort, and the surge pod
then has nowhere to go on a single-node cluster.
2. Neither listener is bound at runtime. --metrics-bind-address=0 and
--health-probe-bind-address=0 are asserted explicitly, because the
operator's own defaults are :8080 and :8081. The arguments are the only
thing holding this, and the scheduler cannot see it either way. This is
the loosely pinned one, and the reason is worth stating precisely so
nobody trusts the wrong model of it. contains is a membership test with
no notion of position, while Go's flag package keeps the last occurrence,
so a duplicate appearing AFTER the disabled one wins and the assert still
passes; a duplicate before it loses, and the assert passes just the same.
Moving the disabled flag last therefore fixes the process but not the
test, which stays green either way. notContains on :8080 and :8081 closes
that for those two addresses, again regardless of position, and they are
the ones a values-gated block added to let users enable metrics would
reach for, since they are this binary's own flag defaults. Any other
address slips through: helm-unittest cannot express "this flag appears no
more than once". So changing a value in place is caught, and duplicating
one is caught only for those two addresses.
3. --leader-elect stays on. maxSurge: 1 means a second manager process
exists by construction on every upgrade, so the lease is what keeps the
incoming pod's reconcile loops off the cluster until the outgoing one is
gone. Turning it off lets two managers reconcile at once.
4. No probe uses httpGet. This follows from 2: with both listeners disabled
an httpGet probe has no port left to target. Only the httpGet field is
asserted absent, not the probes themselves, so an exec probe stays
legitimate — but under maxUnavailable: 0 an exec probe that cannot pass
while the outgoing pod is still alive wedges the rollout permanently
rather than transiently, since the outgoing pod is not retired until the
incoming one is available. A readiness gate that depends on the old
process being gone deadlocks against the strategy.
Read precondition 3 for exactly what it says and no more. The lease governs
the reconcile loops; it does not govern everything the two pods do at once,
and two overlaps sit outside it. The routine one happens on every healthy
upgrade and is bounded by the outgoing pod's termination grace: for that
moment an old binary keeps reconciling against schema the incoming pod has
already moved. The rare one needs the outgoing pod to be broken and
restarting, and it is the worse of the two, because one ordering of it
silently reverts the incoming pod's install phase and does not heal itself
afterwards. Both are known and accepted, not oversights. Unlike the binders
below, neither is hypothetical.
What pays for carrying them, since Recreate would make Available honest with
no second pod at all: when the replacement never starts — a surge pod that
cannot be scheduled, an image that cannot be pulled — maxUnavailable: 0 keeps
the outgoing operator running while the rollout wedges, where Recreate would
have deleted it first and left none. That is the whole of the gain, and it
covers only that class. A replacement that starts and then dies is already
counted available, so the outgoing pod is gone by then under either strategy.
The variant loop is not ceremony: the template branches on variant inside the
container spec, so a port added under one branch renders for that variant
alone and a single-variant suite would not see it. The container-name assert
pins containers[0] for the three argument asserts, which would otherwise start
describing a different container if one were inserted ahead of the operator.
The manager is also configured with a webhook server on 9443, which reads
like a third binder and is not one: controller-runtime adds that server to
its runnables only inside GetWebhookServer(), and this binary registers no
webhook and never calls it, so nothing listens. Registering one would bind
9443 in the host namespace and wedge the rollout the same way, and no
chart-level test can reach that — it is a Go-side change. PprofBindAddress is
a fourth binder of the same kind: unset today, with no flag exposing it and
no path to it from this chart, so it is inert for the same reason and would
wedge the rollout for the same reason if it were ever set.
*/}}
{{- $validVariants := list "talos" "generic" "hosted" -}}
{{- if not (has .Values.cozystackOperator.variant $validVariants) -}}
{{- fail (printf "Invalid cozystackOperator.variant %q: must be one of talos, generic, hosted" .Values.cozystackOperator.variant) -}}
@@ -35,8 +118,34 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
maxSurge: 0
maxUnavailable: 1
# This pair is what Kubernetes itself would pick here: the default
# 25% / 25% resolves at replicas: 1 to maxSurge 1 and maxUnavailable 0,
# because the surge rounds up and the unavailability rounds down. Spelling
# it out is not tuning, it is declining to override the default.
# maxUnavailable must stay 0. At replicas: 1 anything higher makes the
# minimum available replica count replicas - maxUnavailable = 0, and the
# Available condition then reports True with no running pod at all, which
# hides a dead operator from every consumer of that status.
# Kubernetes rejects maxUnavailable: 0 together with maxSurge: 0, so the
# surge cannot go the other way either. The surge pod shares the node
# network namespace (hostNetwork below), so nothing in this pod may need
# a port, and the two ways of needing one fail differently. Declaring a
# containerPort makes the surge pod unschedulable on a single-node
# cluster: it never runs, so maxUnavailable: 0 keeps the outgoing pod
# serving and the rollout simply wedges. Binding a port at runtime, by
# pointing --metrics-bind-address or --health-probe-bind-address at a
# real address, is worse, because this pod has no readiness probe and
# counts as available the moment its container starts. The outgoing pod
# is retired at that point, and the incoming one dies with EADDRINUSE
# afterwards, leaving no operator at all rather than the old one. Do not
# read maxUnavailable: 0 as protection against that second class; what
# catches it is whatever waits on the resources the operator is supposed
# to create.
# This also rules out an httpGet probe of any kind on this Deployment: the
# port it would target is one of the two disabled above. Only an exec
# probe can ever work here.
maxSurge: 1
maxUnavailable: 0
template:
metadata:
labels:
@@ -47,6 +156,10 @@ spec:
- name: cozystack-operator
image: "{{ .Values.cozystackOperator.image }}"
args:
# Must stay on. maxSurge: 1 above means a second manager pod exists on
# every upgrade, and the lease is what keeps the incoming one's
# reconcile loops off the cluster until the outgoing one is gone.
# Turning it off lets two managers reconcile the same cluster at once.
- --leader-elect=true
- --install-flux=true
# The operator applies embedded CRDs via server-side apply on every
@@ -0,0 +1,144 @@
suite: cozystack-operator rollout strategy
# Pins the rollout strategy and the four preconditions the surge pod makes
# load-bearing: no declared port, no bound listener, leader election on, no
# httpGet probe. Each is checked against every variant the chart ships. The
# bound-listener one is pinned only partially, for a reason recorded next to
# the asserts themselves; the other three are airtight.
#
# Why the strategy is what it is, why each precondition matters and how each one
# fails: the maintainer note at the top of templates/cozystack-operator.yaml.
# That note is the single owner of the reasoning; this file only asserts it.
templates:
- templates/cozystack-operator.yaml
tests:
- it: keeps the strategy and every surge precondition on the talos variant
set:
cozystackOperator.variant: talos
asserts: &surgeSafe
# The strategy rides the same anchor as the preconditions rather than
# sitting in a talos-only case of its own. It renders outside every
# variant branch today, so one case would pass, but that is a property
# of the template rather than a guarantee: move the block inside a
# branch and a single-variant assert stops seeing the other two.
- equal:
path: spec.replicas
value: 1
documentSelector:
path: kind
value: Deployment
# Pinned alongside the two values below because they are meaningless
# without it: MaxUnavailable() reports 0 for any strategy that is not
# RollingUpdate, so a switch to Recreate would satisfy the intent of this
# test by a different route while leaving the rollingUpdate block behind
# as dead configuration the API server then rejects at apply.
- equal:
path: spec.strategy.type
value: RollingUpdate
documentSelector:
path: kind
value: Deployment
- equal:
path: spec.strategy.rollingUpdate.maxUnavailable
value: 0
documentSelector:
path: kind
value: Deployment
- equal:
path: spec.strategy.rollingUpdate.maxSurge
value: 1
documentSelector:
path: kind
value: Deployment
- equal:
path: spec.template.spec.hostNetwork
value: true
documentSelector:
path: kind
value: Deployment
- notExists:
path: spec.template.spec.containers[*].ports
documentSelector:
path: kind
value: Deployment
- notExists:
path: spec.template.spec.initContainers[*].ports
documentSelector:
path: kind
value: Deployment
# The three argument asserts read containers[0], so pin which container
# that is. A container inserted ahead of the operator would otherwise
# shift the index and leave them asserting about something else.
- equal:
path: spec.template.spec.containers[0].name
value: cozystack-operator
documentSelector:
path: kind
value: Deployment
# contains is a membership test with no notion of position, while Go's
# flag package keeps the last occurrence, so these two alone stay green
# whether a duplicate lands before the =0 and loses or after it and binds
# the port — they cannot tell the two apart. Moving the =0 flag last
# would fix the process without making these asserts catch anything. The
# notContains pair below closes that for the two addresses this binary
# defaults its own flags to; any other address still slips through.
- contains:
path: spec.template.spec.containers[0].args
content: --metrics-bind-address=0
documentSelector:
path: kind
value: Deployment
- notContains:
path: spec.template.spec.containers[0].args
content: --metrics-bind-address=:8080
documentSelector:
path: kind
value: Deployment
- contains:
path: spec.template.spec.containers[0].args
content: --health-probe-bind-address=0
documentSelector:
path: kind
value: Deployment
- notContains:
path: spec.template.spec.containers[0].args
content: --health-probe-bind-address=:8081
documentSelector:
path: kind
value: Deployment
- contains:
path: spec.template.spec.containers[0].args
content: --leader-elect=true
documentSelector:
path: kind
value: Deployment
# Only the httpGet field is asserted absent, not the probes themselves, so
# an exec probe stays legitimate.
- notExists:
path: spec.template.spec.containers[*].readinessProbe.httpGet
documentSelector:
path: kind
value: Deployment
- notExists:
path: spec.template.spec.containers[*].livenessProbe.httpGet
documentSelector:
path: kind
value: Deployment
- notExists:
path: spec.template.spec.containers[*].startupProbe.httpGet
documentSelector:
path: kind
value: Deployment
- it: keeps the strategy and every surge precondition on the generic variant
set:
cozystackOperator.variant: generic
cozystack.apiServerHost: api.example.com
asserts: *surgeSafe
- it: keeps the strategy and every surge precondition on the hosted variant
set:
cozystackOperator.variant: hosted
asserts: *surgeSafe