Skip to content

[test/otel/pernode] Add per-node + zero-step CRD E2E suite and EKS harness - #720

Open
wenegiemepraise wants to merge 1 commit into
aws:mainfrom
wenegiemepraise:pernode-e2e
Open

wenegiemepraise wants to merge 1 commit into
aws:mainfrom
wenegiemepraise:pernode-e2e

Conversation

@wenegiemepraise

Copy link
Copy Markdown

Summary

Add the test/otel/pernode integration suite plus a Terraform EKS harness that validates
the ServiceMonitor/PodMonitor scraping path end to end on a live cluster: per-node target
locality, zero-step CRD bundling, and Target Allocator resilience to missing CRDs.

What's included

Suite (test/otel/pernode/, build tag integration):

  • per_node_test.go — every SM/PM series is node-local: target_node == @resource.k8s.node.name; targets span ≥2 nodes.
  • crd_bundling_test.go — SM + PM CRDs are served/established after a plain chart install (harness never installs them separately).
  • ta_resilience_test.go — TA Deployment Available, no CrashLoopBackOff, 0 restarts on a fresh cluster; and it actually discovers monitors.
  • setup_test.go, k8s_helpers_test.go, metrics_test.go — shared clientset/ground-truth, CRD discovery, otelmetrics client bootstrap.
  • resources/workload.yaml — sm-app/pm-app workloads + SM/PM with the target_node relabel + load generator.

Harness (terraform/eks/daemon/otel-pernode/): provisions EKS, installs the chart with the
per-node-capable operator/TA images, applies the workloads, and runs the suite. Because it
exercises unreleased code it takes operator_image_*, ta_image, helm_chart_repo/branch vars.

Verification method (per-node)

target_node             = scraped POD's node   (SM/PM relabel from __meta_kubernetes_pod_node_name)
@resource.k8s.node.name = scraping AGENT's node (from ${env:K8S_NODE_NAME})
per-node holds  ⇔  for every series, target_node == @resource.k8s.node.name

Dependencies / notes

  • Validates the feature PRs (operator per-node + CRD resilience, helm bundling) — most useful
    once those and their custom images are available; tests are skip-guarded so they don't break
    the broader suite where fixtures are absent.
  • go vet -tags integration ./test/otel/pernode/... is clean against current main.


// Informational: on a freshly provisioned cluster (the otel-pernode harness)
// this should be 0, evidencing the TA never restarted to pick up the CRDs.
t.Logf("Target Allocator lifetime container restarts: %d (expected 0 on a fresh harness cluster)", totalRestarts(pods))

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The harness installs the chart with CRDs already bundled and then rollout restarts the target-allocator, so this never opens the missing CRD window it's meant to test, and the restart zeroes the one signal (lifetime restart count). Could we relabel it a smoke test on a bundled install rather than CRD resilience?

Comment thread test/otel/pernode/crd_bundling_test.go Outdated
// ServiceMonitor and PodMonitor CRDs are present and established (served by the
// API server) with no manual prerequisite. The otel-pernode harness installs the
// chart onto a cluster that does NOT pre-install these CRDs, so their presence
// here is attributable to the chart's bundling.

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crdServed only checks the CRDs are served, not that this chart installed them, so a rerun or a preexisting prometheus-operator passes even when the chart bundles nothing. Could we assert the CRD carries this release's app.kubernetes.io/managed-by=Helm and treat CRD absence as a precondition?

topologySpreadConstraints:
- maxSkew: 1
topologyKey: kubernetes.io/hostname
whenUnsatisfiable: ScheduleAnyway

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This workload only soft prefers spreading (ScheduleAnyway), but TestPerNodeCoverageAcrossNodes needs two target_nodes, so both replicas can land on one node and flake it. Could we force the spread with DoNotSchedule or a kubernetes.io/hostname podAntiAffinity? Same for pm-app at line 94.


var results []otelmetrics.MetricResult
var usedMetric string
for _, m := range perNodeMetrics {

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callers break on the first perNodeMetrics, and sm-app/pm-app share metric names, so one path passing greens the test while the other is broken, and queryWorkloadMetric never checks the test context so it can hang. Could we filter on app/job for one series per path and thread a context.Context through?


variable "k8s_version" {
type = string
default = "1.35"

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few defaults are footguns: k8s_version 1.35 may be ahead of what EKS offers in a region (so a default apply fails), and the helm_chart_* defaults point at upstream main. Could we pin a GA version and a real chart ref, and drop the dev registry path in operator_image_repo, before this goes public?

data "external" "clone_helm_chart" {
count = var.local_chart_path == "" ? 1 : 0
program = ["bash", "-c", <<-EOT
rm -rf ./helm-charts

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data.external.clone_helm_chart runs rm -rf then git clone on every plan, which is a side effect in a data source and can wipe a local checkout in the cwd. Could we move it to a null_resource with a triggers guard that clones into path.module behind a [ -d ] check instead?

provisioner "local-exec" {
command = <<-EOT
set -e
sleep 30

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null_resource.patch_cr waits on fixed sleeps instead of readiness (same with validator and the workload apply racing CRD creation), so on a slow cluster kubectl patch fails not found before the CR exists. Could we switch to kubectl wait on the CR and Established CRDs plus kubectl rollout status?

return groundTruth
}

func buildGroundTruth() (*k8sGroundTruth, error) {

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The groundTruth and TestMain helpers here duplicate the standard suite, so any fallback fix has to land twice, and buildGroundTruth's pod List and nodeNames() are dead code. Could we pull the shared helpers into a common package, drop the dead code, and collapse crdServed's redundant not found branch?

wenegiemepraise added a commit to wenegiemepraise/amazon-cloudwatch-agent-test that referenced this pull request Jul 24, 2026
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.
@Aakash-Dantre

Aakash-Dantre commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Checklist posted for the PR author to move into the PR description under a ## PR Checklist heading.

PR Checklist

  • Commits are squashed into a logical, reviewable set — single commit, authored by the PR author
  • Commits and PR description comply with Amazon internal guidelines
  • make passes locally — make simple-lint PASS (impi + checklicense), go vet -tags integration ./test/otel/pernode/... PASS
  • All GitHub Actions checks on the PR are passing
  • Integration test evidence: link to a passing run of this suite
  • New or updated integration test coverage: N/A — this PR is the test coverage
  • New functionality has unit tests; bug fixes have a reproducing test
  • Config translation changes include updated golden files — N/A, test-only change
  • Breaking or customer-visible changes are called out in the PR description — none, test-only

Note for the description: helm_chart_branch defaults to main, which the file's own comment says
to avoid ("Pin to a fixed ref (not a moving branch) for CI"). Until the chart work merges, main
installs a chart without the feature under test.

… harness

Adds the test/otel/pernode integration suite (build tag: integration) plus a
Terraform EKS harness validating the ServiceMonitor/PodMonitor scraping path
end to end: per-node target locality (target_node equals the scraping agent's
k8s.node.name), bundled SM/PM CRDs served after a plain chart install, and
Target Allocator health on a bundled install.

Verified: make simple-lint passes; go vet -tags integration
./test/otel/pernode/... is clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants