diff --git a/helm/templates/_helpers.tpl b/helm/templates/_helpers.tpl index 4689baa08..1fa6d0670 100644 --- a/helm/templates/_helpers.tpl +++ b/helm/templates/_helpers.tpl @@ -1893,3 +1893,35 @@ Returns: "10800s" {{- $scaled := mulf $seconds $multiplier | int -}} {{- printf "%ds" $scaled -}} {{- end -}} + +{{/* +Name of the headless service used for Alloy cluster peer discovery. + +Separate from the regular server service because peer discovery needs a +headless service: it must resolve to the individual replica pod IPs rather than +to a single virtual ClusterIP, so each Alloy replica can gossip with every +other one. +*/}} +{{- define "cloudzero-agent.server.clusterServiceName" -}} +{{- /* This name must never equal the regular server Service name. They are + different Service contracts, so colliding means a duplicate resource on + install, or an upgrade asking Kubernetes to convert an existing ClusterIP + Service into a headless one, which it rejects as an immutable field change. + + Reserving room for the suffix is not enough, because appending to a + truncated base can rebuild the base. A name ending in "-cluster" whose + length puts the cut inside that suffix reconstructs itself: both a + 63-character and a 62-character name do. Truncating further just repeats + the problem at another length, so shortening cannot be the answer. + + When the derived name matches, fall back to a different suffix instead. + That is distinct by construction rather than by luck: the branch is only + reached when the base ends in "-cluster", and the fallback ends in + "-peers", so the two cannot be equal for any input. */ -}} +{{- $base := include "cloudzero-agent.server.fullname" . -}} +{{- $name := printf "%s-cluster" ($base | trunc 55 | trimSuffix "-") -}} +{{- if eq $name $base -}} + {{- $name = printf "%s-peers" ($base | trunc 57 | trimSuffix "-") -}} +{{- end -}} +{{- $name -}} +{{- end -}} diff --git a/helm/templates/agent-cluster-service.yaml b/helm/templates/agent-cluster-service.yaml new file mode 100644 index 000000000..9fd6918a0 --- /dev/null +++ b/helm/templates/agent-cluster-service.yaml @@ -0,0 +1,54 @@ +{{- /* +Headless service for Alloy cluster peer discovery. + +Only rendered in clustered mode, where the Alloy container runs. Alloy replicas +find each other through --cluster.join-addresses (see agent-deploy.yaml), which +resolves this name via DNS and expects one A record per replica. + +Two properties matter and are not interchangeable with the regular server +service in agent-service.yaml: + + - clusterIP: None. A normal ClusterIP service load-balances to a single + backend, so a replica would discover at most one arbitrary peer. Headless + DNS returns every pod IP, which is what forms a complete ring. + + - publishNotReadyAddresses: true. A starting replica has to be discoverable + before it passes its readiness probe, otherwise a full restart of the + deployment deadlocks: no pod is in the service until it is ready, and the + ring cannot form from pods that cannot see each other. + +The port must match --server.http.listen-addr in agent-deploy.yaml; Alloy +gossips over its HTTP listen port rather than a dedicated one. +*/ -}} +{{- if eq (include "cloudzero-agent.Values.components.agent.mode" .) "clustered" }} +apiVersion: v1 +kind: Service +metadata: + name: {{ include "cloudzero-agent.server.clusterServiceName" . }} + namespace: {{ .Release.Namespace }} + {{- include "cloudzero-agent.generateLabels" (dict + "root" . + "name" "server" + "labels" (list + .Values.defaults.labels + .Values.commonMetaLabels + ) + ) | nindent 2 }} + {{- include "cloudzero-agent.generateAnnotations" (dict + "root" . + "annotations" (list + .Values.defaults.annotations + ) + ) | nindent 2 }} +spec: + type: ClusterIP + clusterIP: None + publishNotReadyAddresses: true + ports: + - port: 9090 + targetPort: 9090 + name: http-metrics + protocol: TCP + selector: + {{- include "cloudzero-agent.server.matchLabels" . | nindent 4 }} +{{- end }} diff --git a/helm/templates/agent-deploy.yaml b/helm/templates/agent-deploy.yaml index 59ec7c3ad..40b3b2d32 100644 --- a/helm/templates/agent-deploy.yaml +++ b/helm/templates/agent-deploy.yaml @@ -269,10 +269,38 @@ spec: {{- if .Values.components.agent.kubeState.enabled }} - --stability.level=public-preview {{- end }} - {{- if eq (include "cloudzero-agent.Values.components.agent.mode" .) "federated" }} + {{- /* + Clustering. The River config sets `clustering { enabled = true }` on + every pipeline, but those blocks are inert unless the process itself + is started clustered, so these flags are what actually make the + replicas shard work between them. + + --cluster.join-addresses points at the headless service in + agent-cluster-service.yaml. Alloy gossips over its HTTP listen port + (the cluster service is built with ListenAddress = + --server.http.listen-addr), so the port here must track the + listen-addr above. A bare DNS name is handled by the dnsAResolver in + Alloy's join-address resolver chain, which returns one A record per + replica pod. + + This is fail-safe: cluster.Ready() is unconditionally true unless + --cluster.wait-for-size is also set, which we deliberately do not + set. If peers cannot be reached, each replica falls back to a + single-member ring and processes everything -- degraded to duplicate + work, never to silence. + */}} - --cluster.enabled=true - - --cluster.name={{ .Values.clusterName }} - {{- end }} + {{- /* + The cluster name is a gossip label: a node refuses to join peers + whose label differs. Derive it from the server resource name rather + than .Values.clusterName -- that value identifies the Kubernetes + cluster to CloudZero for cost attribution, is optional, and defaults + to empty, so using it here would label most installations with + nothing. The server name is release-scoped and always present, which + is what a ring identity needs. + */}} + - --cluster.name={{ include "cloudzero-agent.server.fullname" . }} + - --cluster.join-addresses={{ include "cloudzero-agent.server.clusterServiceName" . }}.{{ .Release.Namespace }}.svc.cluster.local:9090 {{- include "cloudzero-agent.generateEnv" (dict "env" (list .Values.defaults.env diff --git a/helm/tests/alloy_cluster_service_test.yaml b/helm/tests/alloy_cluster_service_test.yaml new file mode 100644 index 000000000..2593c8271 --- /dev/null +++ b/helm/tests/alloy_cluster_service_test.yaml @@ -0,0 +1,108 @@ +suite: test Alloy cluster peer-discovery service +templates: + - templates/agent-cluster-service.yaml +tests: + # The service exists only to let Alloy replicas find each other, so it is + # pointless outside clustered mode, where no Alloy container runs. + - it: should not be rendered when Alloy is not in use + set: + components.agent.mode: agent + asserts: + - hasDocuments: + count: 0 + + - it: should not be rendered in server mode + set: + components.agent.mode: server + asserts: + - hasDocuments: + count: 0 + + - it: should be rendered in clustered mode + set: + components.agent.mode: clustered + asserts: + - hasDocuments: + count: 1 + - isKind: + of: Service + - equal: + path: metadata.name + value: RELEASE-NAME-cz-server-cluster + + # These two fields are the entire point of the service. A ClusterIP would + # load-balance discovery to a single arbitrary replica, and omitting + # publishNotReadyAddresses deadlocks a full restart, since a pod would have to + # be ready before its peers could find it. + - it: should be headless and publish not-ready addresses + set: + components.agent.mode: clustered + asserts: + - equal: + path: spec.clusterIP + value: None + - equal: + path: spec.publishNotReadyAddresses + value: true + + # Alloy gossips over its HTTP listen port, so this must track + # --server.http.listen-addr in agent-deploy.yaml. + - it: should expose the Alloy HTTP listen port + set: + components.agent.mode: clustered + asserts: + - equal: + path: spec.ports[0].port + value: 9090 + - equal: + path: spec.ports[0].targetPort + value: 9090 + + # Discovery only works if the selector matches the Alloy pods; a mismatch + # yields an empty DNS response and silently degrades to single-member rings. + - it: should select the server pods + set: + components.agent.mode: clustered + asserts: + - equal: + path: spec.selector["app.kubernetes.io/name"] + value: server + + # The suffix is what makes this Service's name distinct from the regular + # server Service. Appending it and only then truncating to 63 would cut the + # suffix away for a name already at the limit, giving two Services one name: + # a duplicate resource on install, and on upgrade an attempt to convert an + # existing ClusterIP Service to a headless one, which Kubernetes rejects. + - it: should not collide with the server service name at the length limit + set: + components.agent.mode: clustered + server.fullnameOverride: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + asserts: + - equal: + path: metadata.name + value: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-cluster + + # A server name that already ends in the suffix rebuilds itself: trimming to + # 55 and appending "-cluster" reproduces the original exactly. Shortening + # further does not help, because the same reconstruction happens at other + # lengths -- hence the different fallback suffix. + - it: should not collide when the server name already ends in the suffix + set: + components.agent.mode: clustered + server.fullnameOverride: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-cluster" + asserts: + - notEqual: + path: metadata.name + value: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-cluster + + # The 62-character variant, where trimming to 55 cuts the separator and the + # trailing dash is trimmed away, so a naive fallback that only shortens + # reconstructs the base a second time. + - it: should not collide at the length where shortening also reconstructs + set: + components.agent.mode: clustered + server.fullnameOverride: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-cluster" + asserts: + - notEqual: + path: metadata.name + value: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-cluster diff --git a/helm/tests/alloy_deployment_test.yaml b/helm/tests/alloy_deployment_test.yaml index 57a7668b7..c23861723 100644 --- a/helm/tests/alloy_deployment_test.yaml +++ b/helm/tests/alloy_deployment_test.yaml @@ -106,19 +106,34 @@ tests: template: templates/agent-deploy.yaml # Test Alloy clustering mode - # Note: Alloy mode and federated mode are mutually exclusive, clustering is always disabled - - it: should not enable Alloy clustering (Alloy mode doesn't use federation) + # + # Clustering must be enabled whenever Alloy runs. The River config emits + # `clustering { enabled = true }` on every pipeline, but that block is inert + # unless the process is also started with --cluster.enabled, and the replicas + # can only shard work between them if they can discover each other. Without + # both, every replica forms a single-member ring, owns 100% of targets, and + # duplicates the entire cluster's collection and remote_write. + - it: should enable Alloy clustering with peer discovery set: components.agent.mode: clustered clusterName: test-cluster asserts: - - notContains: + - contains: path: spec.template.spec.containers[1].args content: --cluster.enabled=true template: templates/agent-deploy.yaml - - notContains: + - contains: + path: spec.template.spec.containers[1].args + content: --cluster.name=RELEASE-NAME-cz-server + template: templates/agent-deploy.yaml + # Peer discovery. Alloy gossips over its HTTP listen port (the cluster + # service is constructed with ListenAddress = --server.http.listen-addr), + # so this resolves to the headless service on 9090. A bare DNS name is + # resolved via the dnsAResolver in the join-address resolver chain, which + # returns every replica's pod IP. + - contains: path: spec.template.spec.containers[1].args - content: --cluster.name=test-cluster + content: --cluster.join-addresses=RELEASE-NAME-cz-server-cluster.NAMESPACE.svc.cluster.local:9090 template: templates/agent-deploy.yaml # Test Alloy volume mounts @@ -203,3 +218,17 @@ tests: path: spec.template.spec.containers[1].livenessProbe.httpGet.port value: 9090 template: templates/agent-deploy.yaml + + # The ring identity must not come from .Values.clusterName: that value is the + # CloudZero cost-attribution identifier, is optional, and defaults to empty, + # so a ring keyed on it would label most installations with nothing. Deriving + # it from the server resource name keeps it release-scoped and always present. + - it: should derive the cluster name from the server resource name + set: + components.agent.mode: clustered + clusterName: "" + asserts: + - contains: + path: spec.template.spec.containers[1].args + content: --cluster.name=RELEASE-NAME-cz-server + template: templates/agent-deploy.yaml diff --git a/tests/helm/template/alloy.yaml b/tests/helm/template/alloy.yaml index c2d22e791..ab734290c 100644 --- a/tests/helm/template/alloy.yaml +++ b/tests/helm/template/alloy.yaml @@ -2336,6 +2336,33 @@ spec: app.kubernetes.io/name: ksm app.kubernetes.io/instance: cz-agent --- +# Source: cloudzero-agent/templates/agent-cluster-service.yaml +apiVersion: v1 +kind: Service +metadata: + name: cz-agent-cz-server-cluster + namespace: cz-agent + labels: + app.kubernetes.io/instance: cz-agent + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: server + app.kubernetes.io/part-of: cloudzero-agent + app.kubernetes.io/version: v3.10.0 + helm.sh/chart: cloudzero-agent-1.1.0-dev + +spec: + type: ClusterIP + clusterIP: None + publishNotReadyAddresses: true + ports: + - port: 9090 + targetPort: 9090 + name: http-metrics + protocol: TCP + selector: + app.kubernetes.io/name: server + app.kubernetes.io/instance: cz-agent +--- # Source: cloudzero-agent/templates/agent-service.yaml apiVersion: v1 kind: Service @@ -2688,6 +2715,9 @@ spec: - /etc/alloy/alloy-config.river - --server.http.listen-addr=0.0.0.0:9090 - --storage.path=/tmp/alloy + - --cluster.enabled=true + - --cluster.name=cz-agent-cz-server + - --cluster.join-addresses=cz-agent-cz-server-cluster.cz-agent.svc.cluster.local:9090 env: - name: K8S_NAMESPACE value: cz-agent diff --git a/tests/helm/template/kubestate.yaml b/tests/helm/template/kubestate.yaml index c0980b7be..c3d25e38c 100644 --- a/tests/helm/template/kubestate.yaml +++ b/tests/helm/template/kubestate.yaml @@ -1903,6 +1903,33 @@ roleRef: kind: ClusterRole name: cz-agent-cz-webhook-init-cert --- +# Source: cloudzero-agent/templates/agent-cluster-service.yaml +apiVersion: v1 +kind: Service +metadata: + name: cz-agent-cz-server-cluster + namespace: cz-agent + labels: + app.kubernetes.io/instance: cz-agent + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: server + app.kubernetes.io/part-of: cloudzero-agent + app.kubernetes.io/version: v3.10.0 + helm.sh/chart: cloudzero-agent-1.1.0-dev + +spec: + type: ClusterIP + clusterIP: None + publishNotReadyAddresses: true + ports: + - port: 9090 + targetPort: 9090 + name: http-metrics + protocol: TCP + selector: + app.kubernetes.io/name: server + app.kubernetes.io/instance: cz-agent +--- # Source: cloudzero-agent/templates/agent-service.yaml apiVersion: v1 kind: Service @@ -2165,6 +2192,9 @@ spec: - --server.http.listen-addr=0.0.0.0:9090 - --storage.path=/tmp/alloy - --stability.level=public-preview + - --cluster.enabled=true + - --cluster.name=cz-agent-cz-server + - --cluster.join-addresses=cz-agent-cz-server-cluster.cz-agent.svc.cluster.local:9090 env: - name: K8S_NAMESPACE value: cz-agent diff --git a/tests/kuttl/alloy-clustered-test/steps/01-install-clustered-chart.yaml b/tests/kuttl/alloy-clustered-test/steps/01-install-clustered-chart.yaml index 2c8095ae0..aedf13f2b 100644 --- a/tests/kuttl/alloy-clustered-test/steps/01-install-clustered-chart.yaml +++ b/tests/kuttl/alloy-clustered-test/steps/01-install-clustered-chart.yaml @@ -13,21 +13,56 @@ commands: # disturb the default release other kuttl suites run against. # The dev image tag mirrors the `make helm-install-current` target. # - # `timeout:` must exceed `helm --wait --timeout=3m` (180s). kuttl applies a 30s + # `timeout:` must exceed the `helm --wait` timeout below. kuttl applies a 30s # per-command timeout here (this suite's kuttl-test.yaml sets `spec.timeout`, which # kuttl ignores — the timeout field is top-level — so the 30s default applies), and # that would kill the install mid-wait. The explicit per-command `timeout:` overrides it. - - timeout: 240 + # On failure, dump why. kuttl deletes the namespace as soon as a step fails, + # so anything not captured here is gone: every past failure of this suite has + # produced a bare "context deadline exceeded" with no indication of which + # container never became ready, which is why it could not be diagnosed from + # CI logs. + # + # The wait is 8m rather than 3m. Clustered mode starts an Alloy container that + # must load the freshly built image, form its gossip ring, and pass a readiness + # probe on a cold KIND node; at three minutes a slow start and a broken install + # are indistinguishable. + - timeout: 600 script: | set -e ROOT="$PWD" while [ "$ROOT" != "/" ] && [ ! -f "$ROOT/helm/Chart.yaml" ]; do ROOT=$(dirname "$ROOT"); done cd "$ROOT" DEV_TAG="dev-$(git rev-parse HEAD)" + NS=cz-alloy-clustered-test + + dump_state() { + echo "=== clustered install did not become ready; dumping state ===" + kubectl -n "$NS" get pods -o wide || true + echo "--- container states ---" + kubectl -n "$NS" get pods -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{range .status.containerStatuses[*]} {.name} ready={.ready} restarts={.restartCount} {.state}{"\n"}{end}{end}' 2>/dev/null || true + echo "" + echo "--- recent events ---" + kubectl -n "$NS" get events --sort-by=.lastTimestamp 2>/dev/null | tail -30 || true + echo "--- alloy logs ---" + for P in $(kubectl -n "$NS" get pods -l app.kubernetes.io/name=server -o name 2>/dev/null); do + echo " ---- $P" + kubectl -n "$NS" logs "$P" -c cloudzero-agent-alloy --tail=80 2>&1 || true + done + } + trap dump_state EXIT + .tools/bin/helm upgrade --install cz-alloy-ct ./helm \ - --namespace cz-alloy-clustered-test \ + --namespace "$NS" \ --create-namespace \ --values clusters/kind-overrides.yaml \ + --values tests/kuttl/webhookserver-min-footprint.yaml \ --set components.agent.mode=clustered \ --set components.agent.image.tag="$DEV_TAG" \ - --wait --timeout=3m + --set components.agent.replicas=2 \ + --set components.agent.kubeState.enabled=true \ + --set kubeStateMetrics.enabled=false \ + --wait --timeout=8m + + trap - EXIT + echo "clustered release installed and ready" diff --git a/tests/kuttl/alloy-clustered-test/steps/03-verify-clustering.yaml b/tests/kuttl/alloy-clustered-test/steps/03-verify-clustering.yaml new file mode 100644 index 000000000..bf299ff95 --- /dev/null +++ b/tests/kuttl/alloy-clustered-test/steps/03-verify-clustering.yaml @@ -0,0 +1,104 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +metadata: + name: verify-clustering +# Asserts that replicas actually divide work, which nothing else here can catch. +# +# Alloy is started without --cluster.wait-for-size on purpose, so cluster.Ready() +# stays true even when a replica reaches no peers. That is deliberate -- a broken +# ring degrades to every replica collecting everything rather than to collecting +# nothing -- but it also means readiness probes, rendered-template assertions, and +# the image scan all stay green while sharding is completely broken. The defect +# this suite now guards against survived from the day clustering was introduced +# precisely because no test looked at runtime behaviour. +# +# Two independent signals, both read from the Alloy logs: +# +# peers_count > 1 the gossip ring actually formed across replicas +# local < total this replica owns a strict subset of the cluster's pods +# +# The second is the one that matters: peers could be discovered while ownership +# still failed to divide. +# +# kuttl command steps use top-level `commands:` with `script:`; TestStep has no +# `spec` field, and scripts run with CWD = this step directory. +commands: + - timeout: 700 + script: | + set -e + NS=cz-alloy-clustered-test + + # Re-sharding is driven by membership changes, so allow time for the + # second replica to join and for the ring to settle before reading logs. + for i in $(seq 1 30); do + PODS=$(kubectl -n "$NS" get pods -l app.kubernetes.io/name=server \ + --field-selector=status.phase=Running -o name 2>/dev/null | wc -l | tr -d ' ') + [ "$PODS" -ge 2 ] && break + sleep 10 + done + if [ "$PODS" -lt 2 ]; then + echo "expected at least 2 running server replicas, found $PODS" >&2 + kubectl -n "$NS" get pods -l app.kubernetes.io/name=server >&2 + exit 1 + fi + + # The ownership assertion below reads the discovery.kubestate re-shard + # line, which only exists when that component runs -- the install step + # therefore enables kubeState explicitly. It is also the component whose + # sharding actually matters: it is what emits per-pod metrics, so it is + # what duplicates the whole cluster when the ring fails to form. + # + # Collect logs from every replica; either may be the one that logs the + # membership change first. + # + # Wait for the re-shard line, not just for peer discovery. peers_count is + # logged when the cluster node starts, which happens before any ownership + # is assigned, so breaking on it alone samples the logs too early and the + # ownership check then finds nothing. Re-sharding is driven by a + # membership change and by the kubestate component's first pass, both of + # which land later. + LOGS="" + for i in $(seq 1 45); do + LOGS="" + for P in $(kubectl -n "$NS" get pods -l app.kubernetes.io/name=server -o name); do + POD_LOGS=$(kubectl -n "$NS" logs "$P" -c cloudzero-agent-alloy --tail=2000 2>/dev/null || true) + LOGS=$(printf '%s\n%s' "$LOGS" "$POD_LOGS") + done + if echo "$LOGS" | grep -q "peers_count=" && echo "$LOGS" | grep -qE "total=[0-9]+ local=[0-9]+"; then + break + fi + sleep 10 + done + + echo "$LOGS" | grep -E "peers_count=|re-sharded" | tail -20 || true + + # 1. The ring formed across more than one member. + PEERS=$(echo "$LOGS" | grep -oE "peers_count=[0-9]+" | tail -1 | cut -d= -f2) + if [ -z "$PEERS" ]; then + echo "no peers_count line found; peer discovery never ran" >&2 + exit 1 + fi + if [ "$PEERS" -lt 2 ]; then + echo "peers_count=$PEERS: replicas did not form a multi-member ring" >&2 + exit 1 + fi + + # 2. Ownership is a strict subset. A single-member ring reports local=total, + # which is exactly the failure mode this step exists to catch. + SHARD=$(echo "$LOGS" | grep -oE "total=[0-9]+ local=[0-9]+" | tail -1) + if [ -z "$SHARD" ]; then + echo "no re-shard line found; cannot confirm ownership was divided" >&2 + exit 1 + fi + TOTAL=$(echo "$SHARD" | grep -oE "total=[0-9]+" | cut -d= -f2) + LOCAL=$(echo "$SHARD" | grep -oE "local=[0-9]+" | cut -d= -f2) + if [ "$TOTAL" -le 0 ]; then + echo "total=$TOTAL: nothing was discovered, so sharding cannot be judged" >&2 + exit 1 + fi + if [ "$LOCAL" -ge "$TOTAL" ]; then + echo "local=$LOCAL total=$TOTAL: this replica owns everything, sharding is not working" >&2 + exit 1 + fi + + echo "clustering verified: peers_count=$PEERS, local=$LOCAL of total=$TOTAL"