Fix Target Allocator startup crashes, PrometheusCR watcher, and Prometheus config pod restart by musa-asad · Pull Request #386 · aws/amazon-cloudwatch-agent-operator

musa-asad · 2026-06-16T09:22:45Z

Summary

Multiple defects prevent the Target Allocator (TA) from working correctly in both Helm and EKS Add-On deployments:

TA CrashLoopBackOff: The TA binary never registered the --enable-prometheus-cr-watcher flag the operator passes, so pflag.ExitOnError calls os.Exit(2) immediately.
Three latent defects (unreachable until fix §1): synthetic Prometheus empty-namespace panic, empty evaluation_interval parse failure, nil SD metrics → 0 targets discovered.
Prometheus config changes don't restart agent pods: Only spec.config changes triggered restarts — spec.prometheus changes were ignored.
scrape_protocols regression risk: No test guarded that the Prometheus dependency defaults scrape_protocols on config load.

Changes

TA binary (commit 1):

config/flags.go — register enable-prometheus-cr-watcher Bool flag + getter
config/config.go — OR the flag into Config.PrometheusCR.Enabled
watcher/promOperator.go — set ObjectMeta.Namespace from OTELCOL_NAMESPACE env; set EvaluationInterval to ScrapeInterval
main.go — initialize sdMetrics with discovery.CreateAndRegisterSDMetrics()

Pod restart (commit 3):

internal/manifests/collector/annotations.go — configHashInput() appends serialized Spec.Prometheus to the hash, gated by Prometheus.IsEmpty()

Regression test (commit 2):

config/scrape_protocols_regression_test.go — TestScrapeProtocolsDefaultedOnLoad

Test Output

A/B Comparison — Upstream OTel vs CWA Helm (fixed) vs CWA Add-On (fixed)

All 3 environments deployed on the same EKS cluster, same OTEL version, same scrape targets. The fixed images were deployed on both the Helm chart path AND the EKS managed Add-On path (add-on installed via aws eks create-addon, then TA/operator deployments patched to the fixed images).

Check	upstream-otel	cwa-helm (fixed)	cwa-addon (fixed)
TA pod Running	Running 14h, 0 restarts	Running 21m, 0 restarts	Running 11h, 0 restarts
/livez	200 OK	200 (mTLS)	200 (mTLS)
/readyz	200 OK	200 (mTLS)	200 (mTLS)
Collectors registered	both collector-0 & -1 receive targets	both agent-0 & -1 receive targets	both agent-0 & -1 receive targets
Jobs discovered	4 (2 static + 2 prometheusCR)	5 (3 static + 2 prometheusCR)	4 (2 static + 2 prometheusCR)
scrape_protocols present	present on all 4 jobs, 0 MISSING	present on all 5 jobs, 0 MISSING	present on all 4 jobs, 0 MISSING
Agent pods Running (0 restarts)	2/2 Running, 0 restarts	2/2 Running, 0 restarts	2/2 Running, 0 restarts
Error strings (unknown flag / scrape_protocols / Failed to apply / See you next time)	0/0/0/0	0/0/0/0	0/0/0/0
Config change rolls pods	N/A	YES — both UIDs changed	YES — both UIDs changed; TA also rolled

Conclusion: Both fixed paths match upstream on every check. The --enable-prometheus-cr-watcher flag is accepted (no crash), prometheusCR jobs are discovered, scrape_protocols is present, targets are sharded across collectors, and prometheus config changes trigger pod restarts.

Raw output — Helm path (StatefulSet, 2 replicas, TA + prometheusCR enabled)

$ kubectl get pods -n <test-ns-helm>
cloudwatch-agent-0                                                1/1     Running   0          2m38s
cloudwatch-agent-1                                                1/1     Running   0          2m38s
cloudwatch-agent-target-allocator-5c5b6bd49d-smsb5                1/1     Running   0          2m38s

$ kubectl logs <ta-pod> --tail=10 | grep -E 'flag|error|Starting'
{"level":"info","ts":...,"msg":"Starting target allocator"}
{"level":"info","ts":...,"msg":"Starting target watcher","strategy":"consistent-hashing"}

# TA args confirm flag accepted:
$ kubectl get pod <ta-pod> -o jsonpath='{.spec.containers[0].args}'
["--enable-prometheus-cr-watcher"]

# Error string counts (all 0):
$ for p in cloudwatch-agent-0 cloudwatch-agent-1; do echo "$p:"; kubectl logs $p | grep -c 'unknown flag'; kubectl logs $p | grep -c 'scrape_protocols cannot be empty'; done
cloudwatch-agent-0: 0 0
cloudwatch-agent-1: 0 0

# Config change pod restart proof:
BEFORE: pod UIDs c9fa...  cbe0...
(changed prometheus relabel value)
AFTER:  pod UIDs 066f...  1310...   (ALL NEW — pods rolled)

Raw output — EKS Add-On path (add-on installed, images patched)

$ kubectl get pods -n <test-ns-addon>
amazon-cloudwatch-observability-controller-manager-f6cdf4fqs4bp   1/1     Running   0          21m
cloudwatch-agent-0                                                1/1     Running   0          20m
cloudwatch-agent-1                                                1/1     Running   0          20m
cloudwatch-agent-target-allocator-77b76f576d-r688k                1/1     Running   0          11h

# TA args confirm flag accepted on add-on path:
$ kubectl get pod <ta-pod> -n <test-ns-addon> -o jsonpath='{.spec.containers[0].args}'
["--enable-prometheus-cr-watcher"]

# Error string counts (all 0):
$ for p in cloudwatch-agent-0 cloudwatch-agent-1; do echo "$p:"; kubectl logs -n <test-ns-addon> $p | grep -c 'unknown flag'; kubectl logs -n <test-ns-addon> $p | grep -c 'scrape_protocols cannot be empty'; done
cloudwatch-agent-0: 0 0
cloudwatch-agent-1: 0 0

# Config change pod restart proof:
BEFORE: pod UIDs ad31...  912d...
(changed prometheus relabel value)
AFTER:  pod UIDs 9586...  814a...   (ALL NEW — pods rolled); TA also rolled

Raw output — Upstream OTel (control)

$ kubectl get pods -n <test-ns-upstream>
otel-targetallocator-0                     1/1     Running   0          14h
otel-collector-0                           1/1     Running   0          14h
otel-collector-1                           1/1     Running   0          14h

# Same targets discovered and allocated:
$ curl -s http://localhost:8080/jobs | python3 -m json.tool | grep job_name
"kubernetes-pods-annotated"
"serviceMonitor/upstream-otel/nginx/0"
"podMonitor/upstream-otel/node-exporter/0"
"prometheus-sample-app"

Unit tests

$ go test ./cmd/amazon-cloudwatch-agent-target-allocator/... -count=1
ok   .../config     0.297s
ok   .../allocation 0.149s
ok   .../watcher    0.402s

$ go test ./internal/manifests/collector/... -count=1
ok   .../collector   0.027s

Commits

1376451 — Register enable-prometheus-cr-watcher flag and fix PrometheusCR watcher startup
0405f4d — test(target-allocator): guard scrape_protocols defaulting on config load
0318a47 — fix(collector): roll pods when Prometheus config changes

…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.

wenegiemepraise

lgtm!

…eus-cr-watcher-flag

configHashInput's error branch is unreachable from the CRD path: Spec.Prometheus.Config is an *AnyConfig whose Object is a map[string]interface{} populated by JSON decoding, and every type that produces is encodable by gopkg.in/yaml.v3. Logging a warning there implied a runtime condition operators should watch for and act on, which is misleading. Replace the slog.Warn with a comment that records three things a reader cannot recover from the code: that the branch is unreachable from the CRD path, why the sentinel is deliberately a constant rather than derived from the error (two different unserializable specs hash equal, so a Prometheus-only change would not roll pods while the error persists), and that the hash's stability across operator restarts rests on gopkg.in/yaml.v3 v3.0.1 sorting map keys -- a yaml library preserving insertion order here would roll every agent pod on restart. Removing the call also removes the log/slog import, which was the package's only use of it. The non-error path, the IsEmpty() gate and the \x00 separator are untouched, so the pinned config-hash values asserted in annotations_test.go are unchanged.

The namespace fed to the Prometheus config generator was taken verbatim from OTELCOL_NAMESPACE, or from the service account namespace file when that env var was empty. Neither source was trimmed, so a trailing newline or surrounding whitespace yielded a string that is not a valid Kubernetes namespace. The file branch also gated on len(ns) > 0, which treats a whitespace-only file as a real value and so skips the defaultCollectorNamespace fallback. Both sources are now whitespace-trimmed, and a value that is blank after trimming falls through to the next source: env var, then the service account file, then defaultCollectorNamespace. The resolution moves out of NewPrometheusCRWatcher into resolveCollectorNamespace, with the service account path held in a package variable so a test can redirect the read. The log line still fires exactly once when the env var is unset, and still reports the resolved value.

…fault Adds coverage for the two behaviors NewPrometheusCRWatcher depends on and that nothing else in the watcher package exercises. TestResolveCollectorNamespace pins the resolution chain across seven cases: OTELCOL_NAMESPACE wins when set, both the env var and the service account namespace file are whitespace-trimmed, a whitespace-only value in either source falls through to the next source rather than becoming the namespace, and an absent or blank file falls back to the default. This guards against a future edit dropping a TrimSpace, reverting to a length-only emptiness check, or reordering the fallback chain -- any of which yields an empty or blank namespace, and the config generator panics on a non-empty namespace requirement. TestNewPrometheusCRWatcherDefaultsEvaluationInterval pins EvaluationInterval defaulting to ScrapeInterval. Without that assignment the generated config carries an empty evaluation_interval, which fails to parse. The tests live in a new file because promOperator_test.go imports neither k8s.io/client-go/rest, the allocator config package, nor logr. The test redirects serviceAccountNamespacePath and restores it via t.Cleanup, so the package's later test files do not inherit a path into a removed temp dir. The constructor is pointed at an unroutable host to confirm it does no dialing.

The collector client resolves its watch namespace once at process init from OTELCOL_NAMESPACE and passes the result to Pods(ns).List and Pods(ns).Watch. A value carrying surrounding whitespace is not a valid namespace name, so those calls matched nothing, no collectors were discovered, and allocation stayed empty with no error surfaced. Trim the value so padded input resolves to the intended namespace, matching how the namespace is now resolved on the watcher path.

LoadFromCLI ORs the CLI flag with prometheus_cr.enabled from the config file, so the flag can only turn the PrometheusCR watcher on. Passing =false is silently ineffective when the config file enables it, which the previous help text did not convey. State the OR behavior in the flag's usage string so the asymmetry is discoverable from --help. No behavior change: only the usage string passed to flagSet.Bool changes.

LoadFromCLI OR-es the --enable-prometheus-cr-watcher flag with prometheus_cr.enabled from the config file, so the flag can only enable the watcher: passing =false cannot disable one the config file turns on. Nothing covered that, and replacing the || with a plain assignment would break only the config-file-true rows while leaving every existing test in the package green. Add a table-driven test over the four (config file, CLI) combinations, plus a committed testdata kubeconfig so the test is hermetic. LoadFromCLI builds a client config before it reaches the OR, so with no --kubeconfig-path it falls back to rest.InClusterConfig() and returns "unable to load in-cluster configuration" early. That passes on a developer host that happens to have a kubeconfig and fails in CI, so the flag pointing at testdata/kubeconfig_test.yaml is required rather than decorative. The fixture's server is 127.0.0.1 and is never dialed. pflag.ContinueOnError is a test-level choice so a parse error fails the test instead of exiting the process; the production getFlagSet call keeps pflag.ExitOnError.

movence · 2026-08-13T14:23:18Z

+	// Namespace must be non-empty; the config generator panics otherwise.
+	collectorNamespace := os.Getenv("OTELCOL_NAMESPACE")
+	if collectorNamespace == "" {
+		if ns, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil && len(ns) > 0 {


have a const for this

I extracted the path to a named constant, defaultServiceAccountNamespacePath (watcher/promOperator.go:37). The package-level serviceAccountNamespacePath var at line 40 stays as the seam TestResolveCollectorNamespace uses to redirect the read in tests.

movence · 2026-08-13T14:23:29Z


 	// TODO: We should make these durations configurable
+	// Namespace must be non-empty; the config generator panics otherwise.
+	collectorNamespace := os.Getenv("OTELCOL_NAMESPACE")


where is this env defined?

The operator injects it: internal/manifests/targetallocator/container.go:59-67 appends OTELCOL_NAMESPACE from the downward API (metadata.namespace fieldRef) unless TargetAllocator.Env already sets it. The variable predates this PR: collector/collector.go:30 already reads it.

…eus-cr-watcher-flag

Extract the hardcoded service account namespace path literal into defaultServiceAccountNamespacePath per review feedback. The package-level serviceAccountNamespacePath var remains as the test seam that redirects the read in unit tests. There is no behavior change.

musa-asad mentioned this pull request Jun 16, 2026

fix(collector): roll pods when Prometheus config changes #387

Closed

musa-asad changed the title ~~Register enable-prometheus-cr-watcher flag and fix PrometheusCR watcher startup~~ Fix Target Allocator startup crashes, PrometheusCR watcher, and Prometheus config pod restart Jun 16, 2026

musa-asad self-assigned this Jun 16, 2026

musa-asad force-pushed the ta-register-prometheus-cr-watcher-flag branch 2 times, most recently from b936969 to d198928 Compare June 16, 2026 18:28

musa-asad force-pushed the ta-register-prometheus-cr-watcher-flag branch 2 times, most recently from 36d23fa to 0318a47 Compare June 16, 2026 18:32

musa-asad requested review from okankoAMZ and sky333999 June 16, 2026 18:37

This was referenced Jun 16, 2026

Do not warn on target allocator config load for non-TA Prometheus configs aws/amazon-cloudwatch-agent#2157

Open

Add default Prometheus EMF metric_declaration when target allocator is enabled aws-observability/helm-charts#319

Closed

musa-asad marked this pull request as ready for review June 17, 2026 14:53

musa-asad force-pushed the ta-register-prometheus-cr-watcher-flag branch 4 times, most recently from ac8b92d to b27505e Compare June 17, 2026 21:09

musa-asad force-pushed the ta-register-prometheus-cr-watcher-flag branch from b27505e to d61d693 Compare June 18, 2026 21:41

wenegiemepraise approved these changes Jul 3, 2026

View reviewed changes

This was referenced Jul 6, 2026

fix(target-allocator): tolerate missing ServiceMonitor/PodMonitor CRDs #394

Open

[amazon-cloudwatch-agent-operator] feat(target-allocator): add per-node allocation strategy #398

Open

musa-asad added 7 commits July 20, 2026 14:08

Merge branch 'main' into ta-register-prometheus-cr-watcher-flag

d5479be

Merge remote-tracking branch 'upstream/main' into ta-register-prometh…

8f2d2ef

…eus-cr-watcher-flag

movence reviewed Aug 13, 2026

View reviewed changes

musa-asad added 2 commits August 13, 2026 17:05

Merge remote-tracking branch 'upstream/main' into ta-register-prometh…

d4ee841

…eus-cr-watcher-flag

musa-asad requested a review from movence August 13, 2026 18:50

Navigation Menu

Uh oh!

Fix Target Allocator startup crashes, PrometheusCR watcher, and Prometheus config pod restart - #386

Fix Target Allocator startup crashes, PrometheusCR watcher, and Prometheus config pod restart#386
musa-asad wants to merge 13 commits into
aws:mainfrom
musa-asad:ta-register-prometheus-cr-watcher-flag

musa-asad commented Jun 16, 2026 •

edited

Loading

Uh oh!

wenegiemepraise left a comment

Uh oh!

movence Aug 13, 2026

Uh oh!

musa-asad Aug 13, 2026

Uh oh!

movence Aug 13, 2026

Uh oh!

musa-asad Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

musa-asad commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

TA binary (commit 1):

Pod restart (commit 3):

Regression test (commit 2):

Test Output

A/B Comparison — Upstream OTel vs CWA Helm (fixed) vs CWA Add-On (fixed)

Raw output — Helm path (StatefulSet, 2 replicas, TA + prometheusCR enabled)

Raw output — EKS Add-On path (add-on installed, images patched)

Raw output — Upstream OTel (control)

Unit tests

Related

Commits

Uh oh!

wenegiemepraise left a comment

Choose a reason for hiding this comment

Uh oh!

movence Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

musa-asad Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

movence Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

musa-asad Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

musa-asad commented Jun 16, 2026 •

edited

Loading