Repository navigation
feat(target-allocator): per-node allocation and annotation-based SM/PM routing - #415
Conversation
| // cloudwatch.aws/scraper: cluster-scraper is scraped only by the cluster-scraper agent's Target | ||
| // Allocator; all others are scraped only by the per-node agent's Target Allocator. | ||
| const ( | ||
| ScraperAnnotationKey = "cloudwatch.aws/scraper" |
There was a problem hiding this comment.
The operator's other CloudWatch-specific annotations (auto-annotate-, inject-jmx-, restartedAt) and the CRD API group all use cloudwatch.aws.amazon.com/, and I can't find any existing key under cloudwatch.aws/ in the operator or the chart. Customers will set this one on their own ServiceMonitors and PodMonitors, so it's hard to rename after release. Was cloudwatch.aws/ intentional, or should it be cloudwatch.aws.amazon.com/scraper?
There was a problem hiding this comment.
Not intentional. I'll switch to cloudwatch.aws.amazon.com/scraper across #415, aws-observability/helm-charts#376 and aws/amazon-cloudwatch-agent-test#773 unless anyone objects.
There was a problem hiding this comment.
Done: renamed to cloudwatch.aws.amazon.com/scraper in #415 (89d4f7f), aws-observability/helm-charts#376 and aws/amazon-cloudwatch-agent-test#773.
|
|
||
| // AmazonCloudWatchAgentTargetAllocatorAllocationStrategyPerNode targets will be allocated to the collector running on the same node as the target. | ||
| // Targets without a resolvable node fall back to the configured fallback strategy (consistent-hashing). | ||
| AmazonCloudWatchAgentTargetAllocatorAllocationStrategyPerNode AmazonCloudWatchAgentTargetAllocatorAllocationStrategy = "per-node" |
There was a problem hiding this comment.
Should the webhook reject per-node when mode isn't daemonset? Upstream does (internal/webhook/collector_webhook.go#L338 (https://github.com/open-telemetry/opentelemetry-operator/blob/main/internal/webhook/collector_webhook.go)). Here it's accepted, and on a Deployment or StatefulSet each
replica takes every target on its own node while the rest go through the fallback, so load ends up uneven.
Related: with helm-charts#376 the DaemonSet (per-node) and the Deployment cluster-scraper both enable the TA, so the existing warning in collector_webhook.go ("we do not recommend enabling Target Allocator when not running as a StatefulSet") will fire on every apply. What are our plans for that?
There was a problem hiding this comment.
Agreed, I'll reject per-node unless mode: daemonset, matching upstream. For the warning, I'd skip it for per-node on a DaemonSet or when scraperRole is set, and keep it otherwise. OK?
There was a problem hiding this comment.
Done in e6fb8f4: per-node is rejected unless mode: daemonset, and the warning is skipped for per-node DaemonSets and agents with a scraperRole. Checked with server-side dry runs on a live cluster.
| } | ||
| sort.Strings(mapping) | ||
| noNode := len(pn.collectors) - len(pn.collectorByNode) | ||
| pn.log.Info("per-node: collector node index rebuilt", |
There was a problem hiding this comment.
This logs every node-to-collector pair at Info on every collector change. On a large cluster with frequent autoscaling, that's a very long log line on every DaemonSet pod add or remove. Could the full mapping go to V(1) and Info keep just the counts?
There was a problem hiding this comment.
Done in f8f0ac6: Info keeps the counts, the full mapping is at V(1).
6574dcf to
f8f0ac6
Compare
e6fb8f4 to
f62b427
Compare
f62b427 to
1c907b8
Compare
|
Review order: #414 is merged, and this PR is rebased onto |
b9ea44d to
d87c0b5
Compare
| collectorMap[pod.Name] = allocation.NewCollector(pod.Name, pod.Spec.NodeName) | ||
| } | ||
| } | ||
| fn(collectorMap) |
There was a problem hiding this comment.
This List is the only full reconciliation collectorMap ever gets. restartWatch (line 83) re-opens the watch every watcherTimeout, and whenever the apiserver closes it, with no ResourceVersion and no re-list. The new watch replays synthetic ADDED events for pods that exist now, but nothing removes a pod that was deleted while no watch was open — its DELETED event is never delivered.
That gap exists on main too, but under consistent-hashing a ghost entry only holds its share of the ring. Under per-node it can hold a whole node. If agent cwagent-abc on node-1 is deleted in the gap and replaced by cwagent-xyz, both stay in the map with node-1, and the tie-break at per_node.go:270 keeps the smaller name, cwagent-abc. Every node-1 target is then assigned to a pod that no longer exists and goes unscraped until the TA restarts. Nothing reports it: those targets have a CollectorName, so recordUnassigned doesn't count them. Autoscaler scale-in on large clusters makes the gap easier to hit.
Suggest replacing the map from a fresh List on every watch restart, or moving to a SharedInformer and rebuilding the collector set from store.List() on each event — that also puts the List and watch predicates in one place. A test that deletes a pod between two watch sessions would cover it; nothing does today.
There was a problem hiding this comment.
Agreed, fixed in the per-node commit (765fcb8). Each watch session now starts from a fresh List that replaces the collector set, and the watch resumes from that List's resourceVersion. TestWatchDropsPodDeletedBetweenWatchSessions deletes a pod between two sessions and fails without the change.
| // needing a Target Allocator restart after agent (re)scheduling. | ||
| if pod.GetObjectMeta().GetDeletionTimestamp() != nil { | ||
| delete(collectorMap, pod.Name) | ||
| } else if pod.Spec.NodeName != "" { |
There was a problem hiding this comment.
Registration here, and on the initial List at line 76, only checks DeletionTimestamp and NodeName — not pod phase or readiness. A pod in CrashLoopBackOff, stuck on ImagePullBackOff, or otherwise not Ready stays registered as its node's collector.
Under per-node, collectorByNode still maps the node to that pod, so its targets are placed by node and never reach the consistent-hashing fallback. The whole node goes unscraped, and because those targets have a CollectorName, the unassigned-targets gauge reads 0 — nothing to alarm on. Crash-looping or OOM-killed agents on busy nodes are a common DaemonSet failure, so this is the per-node case most likely to show up in production. Failed pods (e.g. evicted) behave the same: they keep NodeName, have no DeletionTimestamp, and stay until pod GC.
The same gap exists on main, but there a bad pod costs ~1/N of the hashed targets; here it costs every target on the node.
Suggest registering a collector only when Status.Phase == Running and the Ready condition is true, and deleting it from the map when either stops holding. Since Modified is now handled, the readiness transition already arrives here. Sharing one predicate with the List path keeps the two in sync.
There was a problem hiding this comment.
Agreed, also in 765fcb8. One check shared by the List and watch paths registers a pod only when it is scheduled, not being deleted, Running and Ready, and removes it when that stops being true. On a test cluster, marking an agent not-Ready moved its node's targets to the fallback within about 2 seconds, and they came back once it was Ready again.
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 every target is still scraped, 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. Agent pods are tracked so that a node's targets never stay with an agent that can't scrape them. A pod counts as a collector only when it is scheduled, not being deleted, Running and Ready; the initial List and the watch events share that check, and a pod that stops meeting it is removed. Every watch session starts from a fresh List that replaces the collector set and resumes from its resourceVersion, so a pod deleted while no watch was open does not stay registered.
…per annotation Partitions ServiceMonitor/PodMonitor discovery across CloudWatch Agents by the cloudwatch.aws.amazon.com/scraper annotation on the monitor CR. A monitor annotated cluster-scraper is scraped only by the cluster-scraper agent's Target Allocator; all others only by the per-node agent's. The two roles are complementary, so every monitor is owned by exactly one agent with no double-scrape and no gap. Adds spec.targetAllocator.prometheusCR.scraperRole (Target Allocator config key scraper_role) and a client-side annotation filter applied during monitor discovery in LoadConfig. Note: scraperRole is constrained by an enum with no empty member, so the default per-node role is expressed by omitting the field; setting scraperRole: "" explicitly is rejected by the API server. Companion: aws-observability/helm-charts#339. Verified: go build ./... clean; make impi and make checklicense pass; unit tests pass across the target-allocator and manifests packages.
per-node assigns each target to the collector on the target's node, so it needs one collector per node. Reject allocationStrategy per-node unless mode is daemonset, as upstream does. Also stop warning about running the Target Allocator outside a StatefulSet for the two supported setups: per-node on a DaemonSet, and an agent with a prometheusCR scraperRole. Other non-StatefulSet setups still warn.
da26e56 to
f918e30
Compare
Summary
Two Target Allocator (TA) capabilities for ServiceMonitor/PodMonitor scraping:
per-nodeallocation strategy that assigns each target to the agent on the same node as the scraped pod, so scrape traffic stays node-local instead of crossing nodes and AZs.cloudwatch.aws.amazon.com/scraper: cluster-scraperis scraped only by the cluster-scraper agent's TA; every other monitor only by the per-node agent's TA.Supersedes #398 and #399. #399 already contained all of #398. Rebased onto current
main, which now includes #414 (and with it all of #386), so this PR is only its own 3 commits.What
Per-node allocation:
per-nodestrategy inallocation/: assigns each target to the collector on its node. Node-less targets fall back to a configurable strategy, or stay unassigned with a one-time warning if no fallback is set.Annotation routing:
spec.targetAllocator.prometheusCR.scraperRolefield (TA config keyscraper_role).annotationRoleMatches,selectsMonitor), applied during monitor discovery inLoadConfig. Thecluster-scraperrole keeps only annotated monitors and the default (empty) role keeps only unannotated ones, so every monitor is owned by exactly one agent.Docs:
docs/api.mdis regenerated (make generate) for theper-nodeallocation strategy and thescraperRolefield.Webhook:
allocationStrategy: per-nodeis rejected unlessmode: daemonset(as upstream does), since it needs one collector per node.scraperRole. Other non-StatefulSet setups still warn.Builds on #414
#414 made the SM/PM informers optional (each starts when its CRD exists). The routing filter here,
selectsMonitorinLoadConfig, is applied only to informers that are running, so both behaviours hold: a missing CRD type is skipped and the routing filter applies to the types that are present. The routing test starts its informers through #414'sstartMonitorInformer.Testing
Unit tests, added in this PR:
TestTargetAllocatorModeValidation(per-node accepted on daemonset, rejected on deployment and statefulset; no warning for per-node daemonset or a scraper role; warning kept for consistent-hashing outside a StatefulSet)TestPerNodeAssignsToMatchingNode,TestPerNodeFallbackAssignsNodelessTarget,TestPerNodeLeavesNodelessTargetUnassigned,TestPerNodeReallocatesWhenCollectorAppears,TestPerNodeCollectorRemovalReassigns,TestPerNodeTwoCollectorsSameNodeTieBreak,TestPerNodeFallbackDuringCollectorChange, plus 11 moreTestPerNode*cases;Test_runWatch_UnscheduledThenScheduled,Test_runWatch_TerminatingPodReleased,TestGetNodeNameTestWatchDropsPodDeletedBetweenWatchSessions,TestRunWatchTracksReadiness(both fail without the fix),TestCollectorEligibilityTestAnnotationRoleMatches(8 cases plus the partition invariant),TestSelectsMonitor,TestLoadConfigScraperRoutingAlso run on an EKS 1.34 test cluster with images built from this code: aws/amazon-cloudwatch-agent-test#773 passed 7/8. The exception,
TestServiceMonitorPodMonitorCRDsBundled, fails as expected there because kube-prometheus-stack owns the CRDs. Every ServiceMonitor and PodMonitor (6 SMs, 9 PMs) had fresh series in CloudWatch, and each monitor was scraped by exactly one of the two agents.PR Checklist
makepasses locally -go build ./...(each commit),go vet, the full unit test suite with envtest,-raceon the Target Allocator andapispackages, andgolangci-lint(0 issues) pass.make manifestsshows no drift.f918e30f): PR Build and Operator Integration Test.TestPerNodeAllocation,TestPerNodeCoverageAcrossNodes,TestScraperRoleWiring,TestClusterScraperTargetAllocatorHealthy,TestAnnotationRoutingPartition.per-nodeis opt-in (the TA default staysconsistent-hashing), andscraperRoleis a new optional field. Behaviour changes: a TA on the default role skips monitors annotatedcloudwatch.aws.amazon.com/scraper: cluster-scraper(unannotated monitors are handled as before); the webhook now rejectsper-nodeoutsidemode: daemonset; and the StatefulSet warning is no longer shown for per-node DaemonSets or agents with ascraperRole. New: theper-nodestrategy, thescraperRolefield, and thecloudwatch.aws.amazon.com/scraperannotation.