Repository navigation
[amazon-cloudwatch-observability] feat: support per-node Target Allocator allocation - #330
wenegiemepraise wants to merge 1 commit into
Conversation
| - "--auto-instrumentation-nodejs-image={{ template "auto-instrumentation-nodejs.image" . }}" | ||
| - "--target-allocator-image={{ template "target-allocator.image" (merge .Values.agent.prometheus.targetAllocator.image (dict "region" $.Values.region)) }}" | ||
| - "--feature-gates=operator.autoinstrumentation.multiinstrumentation,operator.autoinstrumentation.multiinstrumentation.skipcontainervalidation" | ||
| - "--feature-gates=operator.autoinstrumentation.multi-instrumentation,operator.autoinstrumentation.multi-instrumentation.skip-container-validation" |
| # agent on the same node (avoids cross-node/cross-AZ scrape traffic); targets | ||
| # without a resolvable node fall back to consistent-hashing. "consistent-hashing" | ||
| # spreads targets across all agents by hash. | ||
| allocationStrategy: per-node |
There was a problem hiding this comment.
Defaulting allocationStrategy to per-node turns it on for every Target Allocator, and it's a silent no op that doesn't fail safe under image skew (a newer operator against an older allocator just CrashLoopBackOffs). Could we keep consistent-hashing as the default and make per-node opt in until the images support it?
There was a problem hiding this comment.
Per-node is intentionally the default, and I'd like to keep it that way rather than ship the feature opt-in. Per-node allocation is the whole point of this change: each DaemonSet agent scrapes only the targets on its own node, which eliminates the cross-node/cross-AZ scrape traffic that consistent-hashing incurs. Defaulting to consistent-hashing would mean the headline behavior is off unless a user knows to flip it, the opposite of "works out of the box."
But if this is a major blocker I could change
| image: {{ template "target-allocator.image" (merge $agent.prometheus.targetAllocator.image (dict "region" $.Values.region)) }} | ||
| allocationStrategy: "consistent-hashing" | ||
| {{- if $agent.prometheus.targetAllocator.prometheusCR.enabled }} | ||
| allocationStrategy: {{ $agent.prometheus.targetAllocator.allocationStrategy | default "per-node" | quote }} |
There was a problem hiding this comment.
This allocationStrategy emit is under $taEnabled but not $otelCIScrape, so it also flips the static scrape_configs path to per-node, which is beyond this PR's scope. Could we gate per-node on $otelCIScrape and leave the rest on consistent-hashing? The | default looks redundant too since values.yaml already sets it.
There was a problem hiding this comment.
allocationStrategy now defaults to per-node only on the $otelCIScrape path and consistent-hashing otherwise
| The options are consistent-hashing and per-node. | ||
| enum: | ||
| - consistent-hashing | ||
| - per-node |
There was a problem hiding this comment.
The per-node enum only lands in crds/, which Helm applies on install but not on helm upgrade, so upgrading an existing release leaves the old CRD in place and it rejects per-node. Could we move the CRD into templates/ with helm.sh/resource-policy: keep?
There was a problem hiding this comment.
Yeah, moving it to templates/ works great for brand-new CRDs — that's actually why the ServiceMonitor/PodMonitor ones already live there. The problem is AmazonCloudWatchAgent isn't new; it's shipped via crds/ for a long time, so it's already on every existing install without Helm's ownership labels. The moment we move that same CRD into templates/, helm upgrade sees an object it doesn't "own" and hard-fails with an invalid-ownership-metadata error — which is worse than the silent field-pruning we're trying to fix (and resource-policy: keep only affects uninstall, so it doesn't help). So for this one I'm going to document the manual kubectl apply on upgrade (Helm's own recommendation for crds/) and track the proper fix — a pre-upgrade hook that applies the CRD — as a separate change we can actually test against a live upgrade.
| receivers: [prometheus/cw_k8s_ci_v0_prometheuscr] | ||
| # Phase 1 base chain + node enrichment: set_node_name/promote_node_name stamp | ||
| # resource.attributes["k8s.node.name"] = the scraping agent's own node | ||
| # (${env:K8S_NODE_NAME}). Combined with the target_node label from the SM/PM |
There was a problem hiding this comment.
This note says per-node is verified end to end via target_node matching k8s.node.name, but the chart stamps k8s.node.name and never actually produces target_node (that has to come from the user's SM/PM relabelings). Could we reword it as a prerequisite the user supplies rather than something the chart outputs?
There was a problem hiding this comment.
Rather than just reword it, I removed the node-stamping from the SM/PM path entirely — since per-node placement is already proven via the Target Allocator logs, the set_node_name/promote_node_name stamp on the prometheusCR pipeline wasn't earning its keep. As you noted, it unconditionally overwrote k8s.node.name with the scraping agent's node, which is only correct when per-node placement succeeds (and mislabels/clobbers under fallback). The shared transform definitions stay for the node-level pipelines (cadvisor, node_exporter, etc.) where the agent's node genuinely is the metric's node. That also drops the misleading "verified end-to-end" note.
…node-allocation Re-sync aws-observability#330 onto the updated scraping-path branch: inherit the feature-gate ID fix, co-gated CRD RBAC render-once ClusterRole/Binding, the $$$1 comment correction, and the prometheuscr scope-processor guard, alongside aws-observability#330's per-node work.
…bundling Re-sync aws-observability#331 onto aws-observability#330 so the stack is linear (aws-observability#329 -> aws-observability#330 -> aws-observability#331): inherit the per-node allocationStrategy scoping/gating, the SM/PM node-stamping removal, and the context-aware allocationStrategy default, on top of aws-observability#331's CRD bundling.
…cture Re-sync aws-observability#332 onto the updated stack (aws-observability#329->aws-observability#330->aws-observability#331). Resolutions favor the settled older-PR fixes applied onto aws-observability#332's prometheusScrape.* restructure: - prometheusCRDsEnabled: keep aws-observability#331's explicit auto branch + invalid-value fail, gated on aws-observability#332's otelContainerInsights.enabled && prometheusScrape.enabled. - allocationStrategy: use aws-observability#332's precomputed $allocationStrategy (per-node on the otelCI path via prometheusScrape.allocationStrategy, consistent-hashing for direct-TA) -- supersedes aws-observability#330's ternary while preserving its intent. - Fix a stale old-key guard (otelContainerInsights.serviceMonitor.enabled) left by auto-merge in the prometheuscr scope-processor to prometheusScrape.enabled. - Keep aws-observability#330's SM/PM node-stamping removal; port aws-observability#331's GitOps opt-out caveat into the new prometheusScrape.crds doc. Verified: lint clean, CRD matrix 21/0, flag matrix 8/8.
…bundling Brings in aws-observability#330, aws-observability#329's latest commits and origin/main. The values.yaml conflict was two non-overlapping additions at the end of the file: main's nested otelContainerInsights.solutions block and this branch's top-level prometheusCRDs block. Both kept, with solutions left inside otelContainerInsights and prometheusCRDs at top level.
…cture Completes the chain: brings in aws-observability#331, aws-observability#330 and aws-observability#329's latest commits. Conflicts resolved in favour of this branch's restructure, which is its purpose: the $allocationStrategy variable form is kept, and the top-level prometheusCRDs block is intentionally NOT re-added since this PR moves it to otelContainerInsights.prometheusScrape.crds.
…ion-scraper-routing Brings in the full chain (aws-observability#332, aws-observability#331, aws-observability#330, aws-observability#329) and origin/main. The cluster-scraper OTEL config conflicted structurally: main added the Karpenter/KEDA solution pipelines (aws-observability#325, aws-observability#345) in the same receivers / processors / pipelines sections where this branch adds its prometheuscr blocks, and git interleaved the {{- if }}/{{- end }} boundaries. Rather than untangle the markers, the file was reconstructed from main's version with this branch's three self-contained, individually-gated prometheuscr blocks re-inserted at their anchors (after each kubeStateMetrics block). Verified: all five cluster-scraper pipelines render (prometheuscr, karpenter, keda, apiserver, kube_state_metrics), scraperRole is still set only on the cluster-scraper, and the TA ClusterRole/Binding still render exactly once.
|
Checklist posted for the PR author to move into the PR description under a PR Checklist
|
c89e074 to
917917a
Compare
Defaults the Target Allocator allocation strategy to per-node on the OTEL Container Insights scraping path (overridable) and widens the CRD allocation-strategy enum to include per-node. Consistent-hashing remains the fallback for targets with no resolvable node. Requires the per-node strategy registered in the operator/Target Allocator build (aws/amazon-cloudwatch-agent-operator#398).
917917a to
3223948
Compare
Summary
Add per-node Target Allocator allocation support to the chart: default the allocation
strategy to
per-nodeon the OTEL Container Insights scraping path, and add node-enrichmenttransforms (
set_node_name/promote_node_name) that stamp the scraping agent's own node ask8s.node.name. Together with thetarget_noderelabel on the SM/PM path, this lets per-nodescraping be verified end-to-end (
target_node == k8s.node.name) and avoids cross-node/cross-AZscrape traffic.
Scope
allocationStrategydefaults toper-nodeon the CI scraping path (overridable)set_node_name/promote_node_nametransforms on theprometheuscrmetrics pipelineper-nodeTesting
helm templatewithotelContainerInsights.enabled=true:allocationStrategy: "per-node"set_node_name/promote_node_nametransforms on the prometheuscr pipelinehelm lintclean (pre-existingfluent-bit-windowsregion warning is unrelated).