[amazon-cloudwatch-agent-test] Add scraper routing integration tests - #724
Open
wenegiemepraise wants to merge 18 commits into
Open
[amazon-cloudwatch-agent-test] Add scraper routing integration tests#724wenegiemepraise wants to merge 18 commits into
wenegiemepraise wants to merge 18 commits into
Conversation
Add an integration suite (test/otel/pernode) and Terraform harness (terraform/eks/daemon/otel-pernode) that validate the Target Allocator per-node allocation strategy end to end, plus the zero-step ServiceMonitor/PodMonitor CRD bundling (G1) and TA resilience to missing CRDs (G2). Suite: - per_node_test.go asserts every scraped series is collected by the agent on the scraped pod's own node (target_node == @resource.k8s.node.name) and that the workload spans >= 2 nodes. - crd_bundling_test.go asserts the SM/PM CRDs are served via discovery after a plain chart install (no prometheus-operator prerequisite). - ta_resilience_test.go asserts the TA Deployment is Available with zero container restarts and that it discovers the monitors once the CRDs exist. - resources/workload.yaml: sm-app/pm-app behind a ServiceMonitor and PodMonitor with a target_node relabel, plus a load generator. Harness installs a helm-charts checkout that bundles the CRDs (no separate CRD install step, by design), deploys custom operator and Target Allocator images carrying the per-node + CRD-watch code, forces the per-node strategy on the CR, applies the workload, and runs the suite.
Validated the pernode suite against a live per-node cluster: - Select Target Allocator pods by app.kubernetes.io/name=cloudwatch-agent- target-allocator (the operator labels component as amazon-cloudwatch-agent-target-allocator, so the previous selector matched nothing). - Assert the TA is currently Ready/Running and not in CrashLoopBackOff instead of requiring a lifetime restart count of 0. The readiness/crashloop check still catches a TA that died on a missing CRD, but is portable across reruns on long-lived clusters; the lifetime restart count is now logged informationally (expected 0 only on a freshly provisioned harness cluster).
- Add local_chart_path var to install the chart from a local checkout (skipping the git clone) so unpushed working-tree changes can be exercised. - Raise the helm_release timeout to 900s for the multi-deployment fresh install (operator, agent DaemonSet, Target Allocator, webhook).
Add scraper_routing_test.go to the pernode E2E suite (integration build tag): - TestScraperRoleWiring: the cluster-scraper AmazonCloudWatchAgent CR carries spec.targetAllocator.prometheusCR.scraperRole=cluster-scraper and the per-node agent carries none (default role) -- the annotation-routing partition wiring. - TestClusterScraperTargetAllocatorHealthy: the operator built the cluster-scraper Target Allocator Deployment and it is Available and not crashlooping.
musa-asad
reviewed
Jul 21, 2026
| // carries no scraperRole (default role: claims only unannotated monitors). The two roles are | ||
| // complementary, giving exactly-one ownership. | ||
| func TestScraperRoleWiring(t *testing.T) { | ||
| assert.Equal(t, clusterScraperRoleValue, scraperRoleOf(t, clusterScraperAgentName), |
Contributor
There was a problem hiding this comment.
TestScraperRoleWiring only reads the rendered scraperRole and the health test only checks the Target Allocator is up, so neither actually proves routing. Could we annotate one monitor and assert via each /jobs that the annotated job lands only on cluster-scraper and the bare one only on per-node?
musa-asad
reviewed
Jul 21, 2026
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| dep, err := clientset.AppsV1().Deployments(agentNamespace).Get(ctx, clusterScraperTADeploymentName, metav1.GetOptions{}) |
Contributor
There was a problem hiding this comment.
The harness only provisions cloudwatch-agent, so the cluster-scraper CR and its Target Allocator never render and these tests fail NotFound, and patch_cr only patches the custom image onto cloudwatch-agent. Could we render both agents and patch that image onto the cluster-scraper CR too?
musa-asad
reviewed
Jul 21, 2026
| // monitor is owned by exactly one agent (no double-scrape, no gap). | ||
| // | ||
| // These assertions are deterministic and checkable out-of-cluster (CR spec + TA Deployment health); | ||
| // the per-monitor claim is exercised by the operator unit test TestAnnotationRoleMatches. |
Contributor
There was a problem hiding this comment.
This deferral comment points at TestAnnotationRoleMatches, which is just a predicate unit test, rather than TestLoadConfigScraperRouting where an annotated monitor actually gets filtered into the scrape jobs. Could we repoint it so readers chasing routing land on the real test?
musa-asad
reviewed
Jul 21, 2026
| return role | ||
| } | ||
|
|
||
| func unstructuredNestedString(obj map[string]interface{}, fields ...string) (string, bool, error) { |
Contributor
There was a problem hiding this comment.
unstructuredNestedString reimplements unstructured.NestedString from a package that's already in the graph and can never actually return an error, so the error it returns and the require.NoError at :64 are both dead. Could we just use unstructured.NestedString?
musa-asad
reviewed
Jul 21, 2026
| // scraperRoleOf reads spec.targetAllocator.prometheusCR.scraperRole from an AmazonCloudWatchAgent CR. | ||
| func scraperRoleOf(t *testing.T, agentName string) string { | ||
| t.Helper() | ||
| restConfig, err := clientcmd.BuildConfigFromFlags("", kubeconfigPath()) |
Contributor
There was a problem hiding this comment.
scraperRoleOf rebuilds its own restConfig and dynamic client on every call, the health test duplicates deploymentAvailable from ta_resilience_test.go, and one 30s context covers both calls. Could we centralize the client and health helpers in k8s_helpers_test.go and give each call its own context?
TestTargetAllocatorHealthy claimed to verify G2 CRD-install-ordering resilience, but the harness bundles the CRDs at install so the missing-CRD window is never opened, and a rollout restart would zero the lifetime restart signal. Rename to TestTargetAllocatorHealthyOnBundledInstall and reword the assertions/comments to describe what it actually checks (TA healthy on a bundled install). The missing- CRD tolerance itself is covered by the operator's Target Allocator unit tests.
crdServed only checked the CRDs were served, so a rerun or a pre-existing prometheus-operator passed even when the chart bundled nothing. Fetch the CRD objects via a dynamic client and assert app.kubernetes.io/managed-by=Helm and meta.helm.sh/release-namespace == the release namespace, so only CRDs this chart release installed count. Treat CRD absence as a precondition failure. Drop the now-unused discovery-based crdServed helper.
sm-app/pm-app used topologySpreadConstraints with whenUnsatisfiable: ScheduleAnyway (soft), so both replicas could land on one node and flake TestPerNodeCoverageAcrossNodes, which needs two distinct target_nodes. Switch both to DoNotSchedule (hard spread on kubernetes.io/hostname) so the two replicas are guaranteed on separate nodes.
queryWorkloadMetric broke on the first metric and sm-app/pm-app share metric names, so the ServiceMonitor path passing greened the test while a broken PodMonitor path went unnoticed; it also used context.Background() and could hang. Stamp a deterministic app label per monitor (sm-app/pm-app) via relabel, filter queries by app, validate each path independently (subtests), and thread a context.Context with a bounded deadline through queryWorkloadMetric.
Pin k8s_version to a GA EKS version (1.35 -> 1.31) so a default apply does not fail in regions that do not yet offer it; scrub the dev registry path from the operator_image_repo example; and correct the helm chart-source comment (it does not default to a fork) to warn that upstream main lacks the feature until the stack merges and to pin a fixed ref / use local_chart_path.
The helm chart was fetched in a data source that ran rm -rf ./helm-charts then git clone on every plan -- a side effect in a data source that could wipe a checkout in the caller's cwd. Replace with a null_resource that clones into path.module behind a git-dir guard, triggered only when the repo/branch changes (fetch+checkout on change, clone once otherwise). Point chart_path and the helm_release dependency at it.
patch_cr slept 30s hoping the CR existed and the workload apply raced CRD creation. Replace with kubectl wait --for=condition=Established on the AmazonCloudWatchAgent CRD (then poll for the CR) before patching, and on the bundled ServiceMonitor/PodMonitor CRDs before applying the workload. Drop the validator's fixed 3-minute propagation sleep -- the tests poll CloudWatch with a bounded retry. terraform validate passes.
buildGroundTruth listed all pods into a pods map that nothing reads, and nodeNames() was never called. Remove the pods field, the pod List, and nodeNames(); ground truth now only holds nodes (all that the per-node checks use). (crdServed and its redundant not-found branch were already removed when the CRD-bundling test moved to a Helm-ownership check.)
Propagate the aws#720 review fixes (bundled-install smoke test, CRD Helm-ownership assertion, workload DoNotSchedule spread, both-path metric validation + context, terraform footguns/clone/readiness, dead-code removal) so the routing suite builds on the updated harness.
Use unstructured.NestedString instead of a hand-rolled reimplementation (whose error path was dead); read the CR via the shared dynamicClient helper instead of rebuilding restConfig + client on every call; give the Deployment Get and Pods List their own contexts; and repoint the deferral comment at TestLoadConfigScraperRouting (where an annotated monitor is actually filtered into the scrape jobs) rather than the predicate unit test.
The chart renders both the per-node (cloudwatch-agent) and cluster-scraper (cloudwatch-agent-cluster-scraper) agents, but patch_cr only swapped the custom images onto the per-node CR, so the cluster-scraper agent + its Target Allocator ran default images that reject scraper_role and crashloop (failing the routing tests with NotFound/unready). Patch the custom agent + TA images onto the cluster-scraper CR (keeping its consistent-hashing strategy + scraperRole) and restart it alongside the per-node agent.
TestScraperRoleWiring only read the rendered scraperRole and the health test only checked the TA was up -- neither proved routing. Add a routed (annotated) and a default (bare) PodMonitor fixture, apply it from the harness, and add TestAnnotationRoutingPartition: an in-cluster probe curls each Target Allocator's /jobs (HTTPS, no client cert -- mirrors the manual runbook) and asserts the routed monitor's job is owned only by the cluster-scraper TA and the bare monitor's only by the per-node TA (no double-scrape, no gap).
musa-asad
approved these changes
Jul 24, 2026
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
Add annotation-based scraper-routing integration tests to the
test/otel/pernodesuite (integrationbuild tag).What
TestScraperRoleWiring: the cluster-scraperAmazonCloudWatchAgentCR carriesspec.targetAllocator.prometheusCR.scraperRole=cluster-scraperand the per-node agent carries none (default role) — the annotation-routing partition wiring.TestClusterScraperTargetAllocatorHealthy: the operator built the cluster-scraper Target Allocator Deployment and it is Available and not crashlooping.Testing
go vet -tags integration ./test/otel/pernode/clean. Run against a cluster with the SR operator + helm PRs deployed:KUBECONFIG=... CLUSTER_NAME=<c> AWS_REGION=<r> go test -tags integration ./test/otel/pernode/ -run 'TestScraperRoleWiring|TestClusterScraperTargetAllocatorHealthy' -vDependencies
Exercises the SR operator (aws/amazon-cloudwatch-agent-operator#399) + helm PRs. Stacks on the E2E suite (#720,
pernode-e2e).