[amazon-cloudwatch-agent-operator] feat(target-allocator): add per-node allocation strategy - #398
wenegiemepraise wants to merge 5 commits into
Conversation
…er startup The target-allocator declared the enable-prometheus-cr-watcher flag name as a constant but never registered it on the flag set, while the operator passes --enable-prometheus-cr-watcher whenever PrometheusCR.enabled is true. Because args are parsed with pflag.ExitOnError, the unregistered flag caused the binary to print 'unknown flag' and exit(2), putting the target-allocator pod into CrashLoopBackOff. This change registers the flag and ORs it with the YAML prometheus_cr.enabled setting, then fixes three latent defects that were previously unreachable because the binary crashed first: - promOperator: set a non-empty Namespace on the synthetic Prometheus object so the prometheus-operator config generator no longer panics with 'namespace can't be empty' in store.ForNamespace. - promOperator: set EvaluationInterval so the generated config does not render an empty global.evaluation_interval, which the prometheus config parser rejects with 'empty duration string'. - main: create and register service-discovery metrics and pass them to discovery.NewManager; passing a nil sdMetrics map makes every SD provider fail to register, yielding zero discovered targets. RELEASE_NOTES updated.
Add a regression test asserting that loading a Target Allocator config whose static scrape job omits scrape_protocols still yields a non-empty ScrapeProtocols on every loaded scrape config. This is defaulted by the pinned Prometheus library during yaml.UnmarshalStrict into the prometheus Config type, so the distributed /scrape_configs payload is never empty and the agent's prometheus-receiver validation passes. The test fails fast if a future dependency or load-path change drops this defaulting.
The pod-template restart-trigger sha256 was computed from Spec.Config only, so a change to Spec.Prometheus (rendered into a separate ConfigMap) left the pod template byte-identical and the workload controller did not roll the pods. Fold the serialized Spec.Prometheus (PrometheusConfig.Yaml()) into the hash input when it is non-empty, so a Prometheus-only change bumps the pod-template annotation and triggers a rolling restart, matching agent-config behavior. When no Prometheus config is set the hash input is byte-identical to the agent config alone, leaving non-Prometheus agents unaffected.
| filter Filter | ||
| } | ||
|
|
||
| func newPerNodeAllocator(log logr.Logger, opts ...AllocationOption) Allocator { |
There was a problem hiding this comment.
newPerNodeAllocator leaves fallbackHasher nil unless SetFallbackStrategy is called, so a hand written per-node config with no fallback keeps targets that have no node label but never scrapes them. Safe today because configmap.go emits consistent-hashing, but could we default it or warn when per-node has no fallback?
| } | ||
|
|
||
| // Rebuild the node index from the current collector set. | ||
| pn.collectorByNode = make(map[string]*Collector) |
There was a problem hiding this comment.
collectorByNode[c.NodeName] = c has no tie break, so two collectors on the same NodeName means last one wins and its targets flap. Can't happen at one pod per node, but a maxSurge DaemonSet rollout could hit it. Could we add a tie break (say the smaller pod name) or note the one pod per node assumption?
| defer timer.ObserveDuration() | ||
|
|
||
| CollectorsAllocatable.WithLabelValues(perNodeStrategyName).Set(float64(len(collectors))) | ||
| if len(collectors) == 0 { |
There was a problem hiding this comment.
SetCollectors returns early on an empty set without clearing its maps, so GetTargetsForCollectorAndJob keeps serving dead collectors until the next non empty call. consistentHashingAllocator does the same, so I read it as intentional parity, but if you want per-node strictly correct we could clear the maps at zero.
| // NodeName is the Kubernetes node the collector pod runs on; it is used by the | ||
| // per-node allocation strategy to match targets to the collector on their node. | ||
| // This struct can be extended with information like annotations and labels in the future. | ||
| type Collector struct { |
There was a problem hiding this comment.
Collector.Hash() only returns Name, so a Modified event that changes NodeName on the same pod produces no diff and collectorByNode never rebuilds. It's safe because spec.NodeName is immutable and unscheduled pods are skipped, but that's subtle, so could we add a comment or hash Name plus NodeName?
|
|
||
| allocatorPrehook = prehook.New(cfg.GetTargetsFilterStrategy(), log) | ||
| allocator, err = allocation.New(cfg.GetAllocationStrategy(), log, allocation.WithFilter(allocatorPrehook)) | ||
| allocator, err = allocation.New(cfg.GetAllocationStrategy(), log, |
There was a problem hiding this comment.
Since this PR is what first registers per-node, an older allocator image without it hits allocation.New erroring and os.Exit(1), so it just CrashLoopBackOffs instead of degrading. Could the companion chart pin the Target Allocator image at this commit or newer when it sets the strategy?
| switch event.Type { //nolint:exhaustive | ||
| case watch.Added: | ||
| collectorMap[pod.Name] = allocation.NewCollector(pod.Name) | ||
| case watch.Added, watch.Modified: |
There was a problem hiding this comment.
The List path above skips pods with a DeletionTimestamp but this watch branch does not, so a terminating pod keeps its node ownership until the Deleted event lands. Could we add the same check here?
| } | ||
| if pn.fallbackHasher == nil && !pn.warnedNoFallback { | ||
| pn.warnedNoFallback = true | ||
| pn.log.Info("per-node: no fallback strategy configured; targets that cannot be matched to a " + |
There was a problem hiding this comment.
A hand-written per-node config without allocation_fallback_strategy leaves node-less targets unassigned behind this one-time Info log. Would defaulting the fallback here, or logging at Warn, be safer?
There was a problem hiding this comment.
we can default the fallback strategy 👍
| description: |- | ||
| AllocationStrategy determines which strategy the target allocator should use for allocation. | ||
| The current option is consistent-hashing. | ||
| The options are consistent-hashing and per-node. |
There was a problem hiding this comment.
The Go doc comment on this field in amazoncloudwatchagent_types.go still mentions only consistent-hashing, so the next make manifests will revert this text. Worth updating the comment too?
Pull in the three review fixes from aws#398 (terminating-collector release, per-node fallback default, allocationStrategy doc sync) so this stacked branch shows the same aws#398 code reviewers approved there. Conflict in config/config.go was the Config struct: both branches realigned it after aws#398 added FallbackAllocationStrategy, and this branch additionally adds ScraperRole. Kept ScraperRole with the shared alignment.
|
Checklist posted for the PR author to move into the PR description under a PR Checklist
|
bc4c37c to
38ed931
Compare
Adds a per-node allocation strategy so each CloudWatch Agent scrapes only the ServiceMonitor/PodMonitor targets on its own node, eliminating cross-node and cross-AZ scrape traffic. Targets with no resolvable node fall back to consistent-hashing so nothing is silently dropped, and unassigned targets are surfaced via a gauge. Also changes collector registration for ALL strategies, including the existing consistent-hashing default: the initial List now skips pods with an empty spec.NodeName, watch.Modified is handled so a pod scheduled after Add is picked up, and pods are dropped as soon as they carry a DeletionTimestamp rather than on Deleted. This releases a terminating agent's targets immediately instead of at the end of its grace period, at the cost of more target churn during rollouts. Verified: go build ./... clean; make impi and make checklicense pass; unit tests pass across the target-allocator and manifests packages.
38ed931 to
1595a87
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).
This branch has not been deployed
4 participants
Summary
Add a per-node allocation strategy to the Target Allocator so each CloudWatch Agent
(DaemonSet, one per node) scrapes only the ServiceMonitor/PodMonitor targets on its own
node — eliminating cross-node / cross-AZ scrape traffic. Targets without a resolvable node
fall back to consistent-hashing, so nothing is silently dropped.
Motivation
The fork registers only
consistent-hashing, a pure hash of the target URL with no nodeawareness — a pod on node A can be scraped by the agent on node B (inter-AZ data-transfer
cost + latency). This ports the upstream OpenTelemetry
per-nodestrategy into the fork.Changes
allocation/per_node.go— node-indexed allocator with consistent-hashing fallback andunassigned-target tracking
allocation/strategy.go— registerper-nodecollector/collector.go— captureCollector.NodeNamefrompod.Spec.NodeName(skip empty-NodeName pods; handle
watch.Modified)consistent-hashing | per-node)internal/manifests/targetallocator/configmap.go— emitallocation_strategy+allocation_fallback_strategyDependencies
commits. (Note: part of Fix Target Allocator startup crashes, PrometheusCR watcher, and Prometheus config pod restart #386 — the SD-metrics registration — has already landed on
main.)node-enrichment transforms). The chart default
per-noderequires this strategy registeredin the TA build.
Testing
allocationunit tests (per-node placement, fallback, unassigned tracking)consistent-hashing reproduces cross-node scrapes (validated live on EKS).