E2E: the harness shape is generating the bugs — restructure e2e/ around a lifecycle-owning harness

[vyncint]

Is your feature request related to a problem? Please describe.

The e2e module has grown to the point where the per-test boilerplate is larger than the tests, and that boilerplate is where the bugs are. Everything below was measured on master @ a252292c.

The module is 7004 lines of Go across 13 suite directories, with 2096 of those in e2e/common/. There are 17 test functions, and every one of them provisions its own KIND cluster (11 suites × 1, volumedrain × 2, fluentd-aggregator × 4).

The entry point every suite goes through is:

func WithCluster(name string, t *testing.T, fn func(*testing.T, Cluster), beforeCleanup func(*testing.T, Cluster) error, opts ...cluster.Option)

— common/cluster.go:52. Five positional parameters, two of them differently-shaped anonymous closures. Because setup and teardown are both callback bodies rather than anything reusable, each of the 17 call sites is a self-contained nested block: median 206 lines, longest 699 (TestElasticsearch_MultiVersion), shortest 104.

What that shape has produced, counted across the module:

	count
var TestTempDir + init() reading PROJECT_DIR, byte-identical	13 (one renamed TestTempDirUnnamed)
scheme-registration AddToScheme lines	103
beforeCleanup PrintLogs/coverage closures	17, in 3 divergent variants
references to the common.<Image>Repo / Tag constants	186
5*time.Minute, 3*time.Second literal pairs	17
context.Background()	26
t.Context(), though the module is go 1.26.0	0
kubectl shell-outs	22
common/panicobject.go, zero references outside itself	133 lines

Two structural causes, both of which block incremental cleanup:

1. Inverted package containment. common/cond and common/setup are subpackages of common yet import it — cond/conditions.go:22, setup/loggingoperator.go:29, setup/logproducer.go:30. Anything the three layers share therefore has to live in common, which is why common is a grab-bag and why it cannot be split in place.

2. The module is not linted. make lint covers the repo root and pkg/sdk; e2e is a separate module and is skipped. Under the pinned golangci-lint 2.12.2 it has 45 findings:

   linter	count
   gofumpt	18
   whitespace	16
   gci	4
   staticcheck	3
   goimports	2
   errcheck	1
   ineffassign	1

   A caveat that cost me some time, and will cost the next person the same: golangci-lint run prints only 12 of those 45 by default. max-same-issues defaults to 3 and caps per unique message text — gci, gofumpt and goimports all emit the identical string "File is not properly formatted", so 24 findings collapse into 3 printed lines, and whitespace's 16 collapse into 4. Fixing the 12 you can see just reveals the next batch. The real total only appears with --max-same-issues=0 --max-issues-per-linter=0.

   For the record, I put "8 findings" in E2E: unbounded kind CLI calls turn a stalled cluster build into a 20-minute panic that takes the whole package down #2287. That was wrong, and so is the 12 that the default output suggests — the true count at the commit that issue cites is 46. I'm correcting it there.

The four real bugs this shape is hiding

These are correctness problems, not style, and I verified each one:

1. A failed helm init is silently swallowed. common/setup/loggingoperator.go:96 assigns the error from actionConfig.Init(...) and never checks it; line 111 overwrites err with the result of LocateChart. Every install therefore proceeds on a possibly half-initialized action config. This is the single ineffassign finding in the table above — the one case where the unlinted module is actively costing something.

2. t.FailNow() is called from a non-test goroutine, 15 times. go setup.LogProducer(t, ...) appears at 15 call sites. LogProducer ends with two common.RequireNoError calls (common/helpers.go:53-58), which call t.FailNow(). The testing docs are explicit that FailNow must be called from the goroutine running the test. In practice the create error is dropped and the test instead dies several minutes later at a require.Eventually with a message that points at the wrong thing. Nothing about LogProducer needs a goroutine — it issues two Create calls and returns. common/cluster.go:66 has the same pattern around cluster.Start.

3. Three waits that can never elapse. elasticsearch-multiversion/elasticsearch_multiversion_test.go:445,450,455 each wait 30*time.Minute for an Elasticsearch deployment. The package runs under -timeout 20m (Makefile:55, applied at Makefile:243), so the binary panics before any of those ceilings is reached. The intended "give up and report" path is unreachable.

4. The coverage-collection policy contradicts itself, and under-reports. Of the 17 clusters, only 12 collect coverage at all — fluentd-aggregator collects from 1 of its 4, volumedrain from 1 of its 2, and fluentd-aggregator-namespacelabel from none — so the reported e2e coverage figure is measuring less than it appears to. Of the 12 that do collect, 9 log the error and continue, while 3 log it and then return err from beforeCleanup (elasticsearch-multiversion:749, watch-selector:193, logging_metrics_monitoring:305), failing a test that otherwise passed, for a reason unrelated to what it was testing.

Describe the solution you'd like

Replace the callback entry point with a harness that owns the lifecycle, and move the shared pieces into real packages under e2e/internal/ — built alongside common/ rather than refactoring it in place, since the inverted imports make in-place impossible. Suites keep one directory each; that is load-bearing, as it gives them independent -timeout and lets CI select a subset.

The shape I have in mind, for a suite that is currently around 200 lines:

func TestMultiWorker(t *testing.T) {
	env := harness.Start(t, harness.Config{Namespace: "testing"})
	// cluster named from t.Name(); scheme built once; operator installed;
	// t.Cleanup registers dump-logs + collect-coverage + delete-cluster; ctx = t.Context()

	out := fixture.HTTPOutput(env.NS, "test-output", env.Receiver.URL("multiworker"))
	env.Create(
		fixture.Logging(env.NS, "multiworker",
			fixture.WithFluentbit(),
			fixture.WithFluentd(fixture.Workers(2), fixture.Drain()),
		),
		out,
		fixture.Flow(env.NS, "test-flow", fixture.MatchProducer(), out),
	)
	env.StartLogProducer()

	env.WaitFor(wait.OperatorReady, wait.ProducerReady, wait.FluentdReady)
	env.Receiver.MustReceive("multiworker")
}

Each element retires a specific count from the table above: t.Cleanup retires the 17 teardown closures and the three divergent coverage policies; a scheme built once retires the 103 AddToScheme lines; t.Context() retires the 26 context.Background() calls and lets cond's ctx *context.Context parameters become plain values; fixtures defaulting to the local images retire the 186 constant references; named wait budgets retire the 17 magic 5m/3s pairs; the harness owning the temp dir retires the 13 init() copies. A synchronous StartLogProducer returning an error retires bug 2.

What I'd like judged, before I write any of it

Not the whole change — one suite. fluentbit-multitenant is the smallest at 174 lines. I would migrate that one alone, as a PR that shows the harness API against a real suite, and stop there until the API has been reviewed. The main risk in this work is not the mechanics; it is committing to an API and then replicating it 13 times. One migrated suite is enough to tell whether the API is right, and cheap to throw away if it isn't.

If that lands, the rest goes one suite per PR.

Describe alternatives you've considered

• Refactor common/ in place. Blocked by the inverted imports — common cannot be split while its own subpackages import it. Building alongside and deleting common last is what makes each step independently reviewable.
• Fix only the bugs and leave the structure. This is the fallback, and the bug fixes are worth having on their own (see below). But all four exist because the boilerplate is copy-pasted, so leaving the shape means the next four arrive the same way.
• One big PR. ~5000 lines across 13 suites will not get a useful review.

Explicitly out of scope

Not proposing any of this here, and I won't fold it into restructure commits:

• Elasticsearch suite performance. It is the longest package in every run, by a wide margin. Three recent green runs on master:

  run	elasticsearch-multiversion	whole e2e job
  30358685184	597.2 s	987 s
  30356357409	1016.1 s	1271 s
  30353824896	1023.6 s	1263 s

  Next-longest package in those runs is 267–297 s, so ES is 2.2–3.5× anything else and the run-to-run spread on ES alone is over 7 minutes. Most of it is waiting for three Elasticsearch versions to become ready, and I suspect a liveness-probe kill-loop — but that is a suspicion, not a finding: I have not captured restart counts. Separate effort either way.

• The E2E: unbounded kind CLI calls turn a stalled cluster build into a 20-minute panic that takes the whole package down #2287 part-2 concurrency cap. That belongs to E2E: unbounded kind CLI calls turn a stalled cluster build into a 20-minute panic that takes the whole package down #2287. Worth repeating one measurement I already published there: loading the same 522 MB image into 4 fresh clusters took 23.3 s serially against 10.8 s concurrently on an idle 12-CPU host — concurrency is ~2.16× faster, so naive serialisation is the wrong fix.

• Batching kind load, reducing the 17-cluster fan-out, and replacing the 22 kubectl shell-outs. The harness should eventually own that seam, but the existing shell-outs keep working behind it.

Additional context

Two things I'd rather say up front than have found in review:

The bug fixes land separately regardless of what happens to this proposal. They need no design agreement, and if the restructure stalls or is declined, they should still be in. I'm opening the first one now: adding e2e to make lint and fixing all 45 findings, with the swallowed helm-init error (bug 1) as its own commit. That one has to be atomic — make lint runs in CI at .github/workflows/ci.yaml:53, so enabling the module without fixing it turns CI red. Bugs 2–4 follow as small independent PRs.

Two smaller things I'll fold into those fixes rather than the restructure:

• The local image name for the syslog-ng reloader disagrees with itself: common/helpers.go:43 says syslog-ng-reloader, common/setup/loggingoperator.go:46 says syslogng-reload. This is masked today because the Makefile always passes the env var; a bare go test would try to load an image that does not exist.
• common/panicobject.go is 133 lines with no references outside itself, and still implements GetZZZ_DeprecatedClusterName / SetZZZ_DeprecatedClusterName, which metav1.Object no longer declares (absent from apimachinery v0.36.1 entirely).

One thing I want maintainer input on rather than deciding myself: common.Initialize (common/helpers.go:60-70) implements sharding via a package-level sequence counter. Because the counter is per-test-binary and 11 of the 13 suites contain exactly one test, localSeq is almost always 1 — so SHARDS=2 puts nearly everything in shard 1 and shard 0 runs almost nothing. The CI matrix that would drive it is commented out (.github/workflows/e2e.yaml:81,137). I'd remove it and keep the t.Parallel() call, which is load-bearing, but deleting a facility someone may intend to revive is your call, not mine.

Environment: measurements taken on master @ a252292c, golangci-lint 2.12.2 as pinned at Makefile:11, Go 1.26.5.

/kind feature

[csatib02]

Status against the items here, ahead of #2301 and #2302 merging.

§7's kill-loop is confirmed and fixed. The out-of-scope note records the Elasticsearch liveness-probe kill-loop as "diagnosed but unconfirmed (never captured a restart count)". The counts are now captured: duration tracks restarts at roughly 230s each, 1 restart 387.9s, 2 restarts 613.6-632.5s, 3 restarts 856.4-1056.8s, across eleven runs. The probe allowed 60s plus three 10s periods and killed at about 90s, against a 44.6-67.4s warm JVM boot. #2302 drops it and raises limits.memory to 1536Mi, because 1Gi against a 512m heap made OOMKill an indistinguishable second cause. Once that merges the package is 376.1s with zero restarts, down from 856.4s.

Bug 2 is retired once #2301 and #2302 merge. The t.FailNow()-off-the-test-goroutine class: #2295 took the 15 go setup.LogProducer sites, #2298 the wait helpers, #2301 cluster.Start, and #2302 the ten common.RequireNoError calls sitting inside require.Eventually condition closures, which testify runs on its own goroutine and which the enumeration here did not cover.

Bug 3 is not done. #2293 replaced the unreachable 30m waits with a budget derived from t.Deadline(), but the derivation has its own defect: the three waits each call esReadyBudget(t), and each call re-derives time.Until(deadline) - esReadyMargin whole, so the first can spend the entire budget and budgetWithin then floors the other two at time.Second. The failure then names elasticsearch8 or elasticsearch9 when elasticsearch7 was the slow one. Lower priority now that the package has roughly 3x headroom, but it misreports the culprit. Getting its own PR.

Leaving this open, because §5.4-5.10 have not started: there is no internal/harness, WithCluster(name, t, fn, beforeCleanup, opts...) is still the entry point in e2e/common/cluster.go, the 13 suites are unmigrated, and common/ is still present. §5.5's checkpoint — migrate fluentbit-multitenant alone and get the harness API reviewed before the other twelve — is the thing this issue asked to be judged, and that has not happened yet.

[csatib02]

§4's fixture sketch is superseded — the harness half landed, the fixture half was rejected by measurement.

Recording it here so batch 3 is not written against it. §4 sketched:

env.Create(
    fixture.Logging(env.NS, "multiworker",
        fixture.WithFluentbit(),
        fixture.WithFluentd(fixture.Workers(2), fixture.Drain()),
    ),
)

Those defaulted builders are gone as of #2308, and the reason is data rather than taste. #2306 was the first evidence: two of three migrating suites declined WithFluentbit() and the syslog-ng spec builder, because the defaults inject fields those suites do not have. #2308 then counted it — ControlNamespace is in 17 of 17 Logging literals, but EnableRecreateWorkloadOnImmutableFieldChange is only in 12, so the builder was already injecting a divergence into five of its would-be callers. Of the ten suites with a FluentdSpec, six share the identical Resources block, three set BufferVolumeMetrics{}, three set Workers: 2. No field is universal enough to default safely.

The rule that replaces it, which predicts every outcome the three PRs reached the slow way: extract a value when it is identical across all callers and no test asserts it. Otherwise it stays in the suite. Image names pass on both counts, 99 of 99. Workers: 2 fails on the second, since it is what the multiworker suite exists to test. Resources fails on the first at 6 of 10 — and extracting for six while four override is precisely the defaulted-builder trap.

What §3's layout becomes:

• internal/harness — lifecycle, waits, receiver, and images. Images are environment config rather than fixtures; setup.defaultImages is the list actually loaded into the cluster, so the harness owning it collapses the three current declarations (common/helpers.go, common/setup/loggingoperator.go, internal/fixture/images.go) into one.
• internal/wait — conditions as values, landed.
• Whole-scenario constructors only in what is left of internal/fixture — tenancy.go today — and worth renaming, since fixture presently means image constants plus a URL formatter plus a tenancy scenario, which is common re-forming under a newer name. A scenario is admitted when two or more suites need the same situation, not the same fields.
• No CR builders at all. The Logging and Flow literals are each suite's specification.

§4 was right about one thing this issue should keep: env.Receiver.URL(tag). ReceiverURL exists in fixture today with no caller outside tenancy.go, while ten suites still write http://%s-test-receiver:8080/%s inline. That is the clearest extraction left in the module and it belongs on the harness next to MustReceive.

Everything else here still stands: §5.6-5.10 are unstarted, common/ is still present, and nine suites are unmigrated.