[amazon-cloudwatch-agent-operator] Route SM/PM by cloudwatch.aws/scraper annotation - #399
Open
wenegiemepraise wants to merge 22 commits into
Open
[amazon-cloudwatch-agent-operator] Route SM/PM by cloudwatch.aws/scraper annotation#399wenegiemepraise wants to merge 22 commits into
wenegiemepraise wants to merge 22 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.
Add a per-node allocation strategy 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 that cannot be matched to a node-local agent (node-less endpoints, nodes without a Ready agent) fall back to consistent-hashing so they are never silently dropped. - allocation/per_node.go: perNodeAllocator (node index, consistent-hashing fallback ring, unassigned tracking, descriptive logging) + registration. - allocation/strategy.go: Collector.NodeName, NewCollector(name, node), WithFallbackStrategy option, targets_unassigned gauge. - collector/collector.go: capture pod.Spec.NodeName, skip empty-NodeName pods, handle watch.Modified so a collector's node is picked up once scheduled (fixes targets being stuck on the fallback after a DaemonSet rollout). - target/target.go: GetNodeName() from __meta_kubernetes_*_node_name labels. - config + main: FallbackAllocationStrategy wiring. - apis/v1alpha1 + CRD: allocationStrategy enum gains "per-node". - internal/manifests/targetallocator/configmap.go: emit per-node strategy and the consistent-hashing fallback from the CR.
# Conflicts: # cmd/amazon-cloudwatch-agent-target-allocator/main.go
Route ServiceMonitor/PodMonitor discovery across CloudWatch agents by the cloudwatch.aws/scraper annotation on the monitor CR. A monitor annotated cloudwatch.aws/scraper: cluster-scraper is scraped only by the cluster-scraper agent's Target Allocator; all others only by the per-node agent's. Add a scraperRole field to the Target Allocator prometheusCR config (spec.targetAllocator.prometheusCR.scraperRole) and a client-side annotation filter in the TA watcher (annotationRoleMatches) applied during monitor discovery. The cluster-scraper role keeps only annotated monitors; the default (empty) role keeps only unannotated ones, so the two roles partition monitors with no overlap and no gap.
Cover the remaining branches of the per-node strategy and its supporting code: filter application, unsupported/seeded/empty fallback, target and collector removal, unassigned and reassignment paths, collector-less bootstrapping, the already-tracked guard, GetNodeName label resolution, and the collector watcher skipping unscheduled pods then picking up the node on Modified.
Re-align the Config struct fields after adding FallbackAllocationStrategy so gofmt/CI passes, and add unit coverage for GetAllocationFallbackStrategy.
Add a LoadConfig-level test proving the scraper annotation filter is applied during monitor discovery (cluster-scraper and default roles keep/skip the right monitors), and a ConfigMap test asserting scraper_role is emitted when set on the prometheusCR.
Refactor the annotation filter into PrometheusCRWatcher.selectsMonitor and log each ServiceMonitor/PodMonitor the cluster-scraper agent claims via the cloudwatch.aws/scraper annotation (the override event). Add unit tests bringing the routing logic to 100% statement coverage: annotationRoleMatches and selectsMonitor (incl. asserting the override log fires only for the cluster-scraper role), a LoadConfig-level routing test, and a configmap scraper_role assertion.
musa-asad
reviewed
Jul 21, 2026
| // not so annotated. This lets a heavy/singleton monitor be routed to the central cluster-scraper | ||
| // agent while all others stay on the per-node agent. | ||
| // +optional | ||
| ScraperRole string `json:"scraperRole,omitempty"` |
Contributor
There was a problem hiding this comment.
scraperRole has no enum validation, so a typo like cluster-scaper quietly falls back to the default role and that monitor gets scraped by nobody. The chart hardcodes the right value today, but could we add +kubebuilder:validation:Enum=cluster-scraper and regenerate the CRD?
musa-asad
reviewed
Jul 21, 2026
| if !annotationRoleMatches(w.scraperRole, annotations) { | ||
| return false | ||
| } | ||
| if w.scraperRole == clusterScraperRole { |
Contributor
There was a problem hiding this comment.
selectsMonitor logs an Info line for every monitor the cluster-scraper claims, and LoadConfig reruns on each reconcile, so a big annotated set means a lot of repeated lines. Low risk, but could we log only on membership change or drop it to V(1)?
musa-asad
reviewed
Jul 21, 2026
| // TestLoadConfigScraperRouting exercises the annotation filter through the real LoadConfig path | ||
| // (matching TestLoadConfig's harness): an annotated ServiceMonitor is discovered by the | ||
| // cluster-scraper role and excluded by the default role. | ||
| func TestLoadConfigScraperRouting(t *testing.T) { |
Contributor
There was a problem hiding this comment.
The unannotated and PodMonitor cases never actually go through LoadConfig even though PodMonitor discovery mirrors ServiceMonitor, and the informer wait is a HasSynced spin loop with no deadline that hangs until CI kills it. Could we add both LoadConfig cases and use cache.WaitForCacheSync with a timeout?
musa-asad
reviewed
Jul 21, 2026
| // Allocator, based on its scraperRole. cluster-scraper role selects only monitors annotated | ||
| // cloudwatch.aws/scraper: cluster-scraper; the default role (empty) selects only monitors that are | ||
| // not so annotated, so the two roles partition monitors with no overlap and no gap. | ||
| func annotationRoleMatches(scraperRole string, annotations map[string]string) bool { |
Contributor
There was a problem hiding this comment.
Routing is binary here (only cluster-scraper routes, everything else falls to per-node), so a future gpu-scraper would silently land on per-node. Could we add a note that annotationRoleMatches needs to become an explicit role to value match before a third role shows up?
musa-asad
reviewed
Jul 21, 2026
| } | ||
|
|
||
| if len(params.OtelCol.Spec.TargetAllocator.PrometheusCR.ScraperRole) > 0 { | ||
| taConfig["scraper_role"] = params.OtelCol.Spec.TargetAllocator.PrometheusCR.ScraperRole |
Contributor
There was a problem hiding this comment.
The operator writes the scraper_role key, which an older Target Allocator image rejects as unknown and then CrashLoopBackOffs on, taking the cluster-scraper path down. Could we make sure the chart pins the operator and Target Allocator images from this commit or newer together?
newPerNodeAllocator leaves fallbackHasher nil until SetFallbackStrategy runs, so a hand-written per-node config with no fallback silently keeps node-less targets in the pool but never scrapes them (the operator's configmap always emits consistent-hashing, so production is unaffected). Emit a one-time warning when a target is left unassigned because no fallback is configured, pointing at the consistent-hashing fallback. Add TestPerNodeWarnsOnceWhenNoFallback.
collectorByNode was rebuilt with last-write-wins over a map, so if two collectors reported the same node (e.g. a transient maxSurge DaemonSet rollout with two pods on a node) ownership — and target placement — flapped with map iteration order. Keep the collector with the smaller pod name deterministically. Add TestPerNodeTwoCollectorsSameNodeTieBreak.
Document that SetCollectors intentionally does not clear the node index/target mappings when the collector set is momentarily empty (parity with consistentHashingAllocator); clearing would drop every target during a transient zero-collector window, and state is corrected on the next non-empty call.
A NodeName change on the same pod would produce no collector diff (so per-node's collectorByNode would not rebuild), but that cannot happen: the collector watch skips unscheduled pods (empty NodeName) until scheduled, and a scheduled pod's spec.NodeName is immutable. Document the invariant and note to hash Name+NodeName if it ever changes.
scraperRole had no validation, so a typo (e.g. cluster-scaper) silently fell back to the default role and that monitor was scraped by nobody. Add +kubebuilder:validation:Enum=cluster-scraper and the matching CRD enum so the API server rejects invalid values. The field stays optional/omitempty, so the default per-node agents (no scraperRole) are unaffected.
selectsMonitor logged an Info line for every monitor the cluster-scraper claims, and LoadConfig reruns each reconcile, so a large annotated set produced repeated log spam. Drop it to V(1) (debug). Bump the test's funcr verbosity to 1 so it still asserts the routing log.
…adConfig TestLoadConfigScraperRouting only exercised an annotated ServiceMonitor and waited on an unbounded HasSynced spin loop. Cover both kinds (PodMonitor mirrors ServiceMonitor) and both annotated/unannotated for each role through LoadConfig, and replace the spin loop with cache.WaitForCacheSync bounded by a timeout so a stuck informer fails fast.
annotationRoleMatches routes only cluster-scraper vs everything-else, so a future role (e.g. gpu-scraper) would silently fall through to the per-node bucket. Document that it must become an explicit role-to-annotation-value match before a third role is added.
musa-asad
previously approved these changes
Jul 24, 2026
The initial pod List skips pods with a DeletionTimestamp, but the watch's Added/Modified branch did not, so a terminating agent kept ownership of its node until the Deleted event landed. Under per-node that leaves the node's targets pinned to a collector that is shutting down, so they go unscraped for the rest of the grace period instead of being re-placed on the node's replacement agent or via the fallback. Apply the same DeletionTimestamp check on Added/Modified and drop the collector as soon as the pod is marked for deletion. Add Test_runWatch_TerminatingPodReleased.
A hand-written per-node config with no allocation_fallback_strategy left targets that carry no node label retained but assigned to no collector, so they were never scraped behind a one-time log line. The operator-generated configmap always emits consistent-hashing, so only hand-written configs were exposed, but silently-unscraped targets is not a safe default. Default the fallback to consistent-hashing in Config.GetAllocationFallbackStrategy whenever the per-node strategy is selected and no fallback is set, following the existing defaulting pattern in that file (GetAllocationStrategy). Setting allocation_fallback_strategy to an empty string remains an explicit opt-out, which is now the only way to reach the no-fallback log; reword it to point at the default and document the invariant on perNodeAllocator. Extend TestGetAllocationFallbackStrategy with the per-node default and opt-out cases.
The generated CRD lists both consistent-hashing and per-node, but the Go doc comment on TargetAllocatorSpec.AllocationStrategy still said the current option is consistent-hashing, so the next make manifests would have reverted the CRD description. Update the comment to match; make manifests now produces no diff for this field.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route ServiceMonitor/PodMonitor discovery across CloudWatch agents by the
cloudwatch.aws/scraperannotation on the monitor CR. A monitor annotatedcloudwatch.aws/scraper: cluster-scraperis scraped only by the cluster-scraper agent's TargetAllocator; all others only by the per-node agent's.
What
spec.targetAllocator.prometheusCR.scraperRolefield (Target Allocator configscraper_role).annotationRoleMatches) applied during monitordiscovery in
LoadConfig.cluster-scraperrole keeps only annotated monitors; the empty(default) role keeps only unannotated ones — complementary, so every monitor is owned by exactly
one agent (no double-scrape, no gap).
Why annotation (not label)
The routing key is behavioural config, not identity. Both agents' TAs already list all monitors
into their cache and filter client-side, and the two roles' selections union to the full set — so
server-side label filtering would gain nothing here. Filtering by annotation is a few lines in the
existing
ListAllcallback.Testing
go build ./...and unit tests pass, incl. newTestAnnotationRoleMatches(8 cases + partition invariant).cluster-scraper TA
/jobs; an unannotated one only in the per-node TA/jobs.GOPROXY=direct GOSUMDB=off(offline proxy); CI is the authoritative gate.Dependencies / stacking
Stacks on the per-node allocation operator work (#398,
ta-per-node-allocation). Companion helm PR:[amazon-cloudwatch-observability] Route SM/PM to cluster-scraper by annotation.