diff --git a/hack/e2e-install-cozystack.bats b/hack/e2e-install-cozystack.bats index 0ca2997cb..b43d0a104 100644 --- a/hack/e2e-install-cozystack.bats +++ b/hack/e2e-install-cozystack.bats @@ -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. diff --git a/hack/helm-unit-tests.sh b/hack/helm-unit-tests.sh index 915f13a86..560108211 100755 --- a/hack/helm-unit-tests.sh +++ b/hack/helm-unit-tests.sh @@ -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 diff --git a/packages/core/installer/templates/cozystack-operator.yaml b/packages/core/installer/templates/cozystack-operator.yaml index a3e87370e..9b86170ad 100644 --- a/packages/core/installer/templates/cozystack-operator.yaml +++ b/packages/core/installer/templates/cozystack-operator.yaml @@ -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 diff --git a/packages/core/installer/tests/rollout_strategy_test.yaml b/packages/core/installer/tests/rollout_strategy_test.yaml new file mode 100644 index 000000000..a544149f7 --- /dev/null +++ b/packages/core/installer/tests/rollout_strategy_test.yaml @@ -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