fix(e2e): give every kind cluster its own kubeconfig - #2301
Conversation
vyncint
changed the title
csatib02
left a comment
csatib02
left a comment
There was a problem hiding this comment.
Chased the flake through the CI history before this lands. The per-cluster --kubeconfig and the stderr capture look right. Two things on the rest.
-parallel 1 and the 16m kind cap work against each other. E2E_TEST_TIMEOUT is unchanged at 20m and it applies per binary, so serialising fluentd-aggregator's four tests spends that one budget four times over. From your own runs: 160.87s and 269.18s parallel in 30546098711, against 629.27s and 490.66s serialised in 30590989952. That is 41-52% of the 1200s cap where it was 13-22%. On the same green runner the single-test packages came in 5-10% faster than baseline (watch-selector 96.6s vs 106.9s), so the jump is serialisation rather than a slow runner. Detail inline on the Makefile.
Most of this PR works around one line. e2e/common/cluster.go:73 uses RequireNoError for the teardown delete, so a cluster that was destroyed cleanly still fails the test when kind's kubeconfig bookkeeping errors. Lines 70 and 71 already use assert.NoError. In 30554200418, TestFluentbitAgentDedicatedNamespace logged Collecting coverage files ... and then failed only at cluster.go:73: the body passed and teardown killed it. TestFluentbitSingleTenantPlusInfra has the same shape, and the two are about 20s apart, so it is not a herd effect. Making 73 match 70-71 retires that failure class whatever the kubeconfig layout is.
Worth splitting in two. Land the per-cluster --kubeconfig and the stderr capture on their own, hold -parallel 1.
One note on the evidence. The stderr capture that would name the delete failure ships in the same commit as the fix, so 30589321074 still shows a bare exit status 1; the lock race is inferred from your local experiment rather than observed in CI. And the green run is one sample. Master was green on 4 of 5 runs after the series with the old flags, so it does not yet separate a real improvement from a quiet runner.
|
Three things in
The sharding wrapped around it is dead and inverted.
go func() {
RequireNoError(t, cluster.Start(ctx))
}()
helpers.go:231 still hardcodes what 96b3131 fixed in the other copy. WatchNamespaces: []string{"tenant"},Latent rather than live: both callers pass |
Review feedback on kube-logging#2301. E2E_TEST_TIMEOUT is 20m per binary, so serialising a package spends that one budget on every test in it. fluentd-aggregator's four took 629.27s and 490.66s serialised (30590989952) against 160.87s and 269.18s parallel (30546098711), which is 41-52% of the 1200s cap where it was 13-22%. It also disarms the kind cap this series added. timeoutFraction derives that cap from the same 20m, so once two tests have run in sequence there is under 16m of package budget left and the readable "timed out after 16m0s" can no longer fire before the binary panics with "test timed out after 20m0s" instead. -p is what bounds peak clusters, and it does it without costing wall time: at most 4 suite binaries at a time, so 8 clusters for the heaviest combination (fluentd-aggregator's 4, volumedrain's 2 and one each from two more) against the 17 an unbounded run creates. Raising the timeout and deriving the kind cap from t.Deadline() would make -parallel 1 workable, but that wants its own measurements rather than riding along here. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The delete that clears a half-built cluster after a create times out did not name the kubeconfig the create used, so it fell back to the shared default. That is the delete most likely to lose the lock, because it runs exactly when several creates are timing out together. The stalled-create case creates with a kubeconfig now, so the delete's arguments are pinned rather than only its presence: without the fix it reports `delete cluster --name stuck` against the expected `delete cluster --kubeconfig /tmp/kind-stuck.kubeconfig --name stuck`. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
Please stop your agent from over-commenting. :) |
Review feedback on kube-logging#2301. Two problems with the per-cluster kubeconfig path. Nothing removed the file. kindCluster.Cleanup only removes the os.CreateTemp copy, and kind rewrites the kubeconfig it manages rather than unlinking it, so a run that was killed handed its file, and any lock left mid-write, to the next one. The path was also predictable in a shared directory, where GetTestCluster already uses os.CreateTemp for the same kind of file. The names now sit inside one os.MkdirTemp directory per run, created 0700, so nothing planted at a guessable path can be written through. DeleteTestCluster removes the kubeconfig and the lock once the cluster is gone, and the directory goes with the last cluster of the binary. common had no tests. These cover the path being per-cluster, stable across calls so create and delete agree, outside the shared temp directory, and the removal taking the lock with it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
…finish Review feedback on kube-logging#2301. Two RequireNoError calls in WithCluster, both of which turn a working test red or drop an error outright. RequireNoError ends in t.FailNow, which is only defined on the test goroutine. Called inside the Start goroutine it did neither of the things it looks like it does: the failure was not recorded, and a cluster that never came up resurfaced as an unrelated Eventually timeout further down. Start's result now travels back over a channel and is asserted in the deferred block, on the test goroutine and after the context is canceled. The bounded wait is there so a runnable that will not stop is named rather than left to exhaust the binary's -timeout. The teardown delete used RequireNoError where lines above it use assert.NoError, so a cluster that came down cleanly still failed its test whenever kind's kubeconfig bookkeeping errored. In 30554200418 TestFluentbitAgentDedicatedNamespace logged its coverage collection and then failed only there. It matches the assertions around it now, and FailNow no longer cuts the rest of the teardown short. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The sharding wrapped around t.Parallel was dead and inverted. sequence is a per-binary counter and ten of the thirteen suites hold exactly one test, so localSeq is always 1 in those binaries. With SHARDS=2 that gives 1 % 2 != 0 for shard 0, which skips, against 1 % 2 == 1 for shard 1, which runs: shard 0 would run nothing and shard 1 everything. Nothing exercised it either way, because the matrix that would set SHARD and SHARDS is commented out in e2e.yaml. E2E_CLUSTERS bounds the concurrency the sharding was reaching for, so this drops it rather than reworking it to shard on t.Name(), and leaves Initialize as the bare t.Parallel the suites already expect. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. LoggingTenant honours nsTenant everywhere except the watch namespace, which was the literal "tenant". WatchNamespaces takes namespaces, not loggingRefs, so it has to name the namespace the Flow and Output above were created in. Latent rather than live: both callers pass nsTenant := "tenant", so the two strings line up today. internal/fixture/tenancy.go already carries this fix, which left the live path and the fixture copy disagreeing on the same logic. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
All three fixed. Checked each against the code first rather than taking them on trust, and all three hold exactly as described. Sharding — aa54d51. Dead and inverted, confirmed.
The same commit makes the teardown delete
|
Review feedback on kube-logging#2301. The matrix that would have driven SHARD and SHARDS was already commented out, and the sharding it fed is gone, so the block goes with it. fail-fast stays: it came in with an earlier k8s-version matrix, not this one. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The comments this series added explained more than the code needed. Cut to one line each where they earn it and dropped where they did not: 27 lines of prose down to 8, plus assertion messages that only restated the expected value. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
Fair, and noted for the replies as much as the code. Trimmed in a44ce1e: 27 lines of comment prose down to 8, and the assertion messages that only restated the expected value are gone. |
|
Re-ran the numbers on the final head. This one looks done: One thing left before the baseline is clean, and it is not this PR's doing.
A 3.1x spread across the green runs, 988.5s is 82% of the 1200s cap, and two of the six hit it. Two things feed it.
The three waits at :490, :495 and :500 each call Cheapest first, if you want to pick this up separately:
Not asking for any of this here. This PR is green and worth landing as it is; raising it so the next one starts from a baseline where a timeout means something is actually wrong. |
|
Hi @csatib02, heads up that c3ee82b landed just after your comment, so it is not in what you measured: no behaviour change, it just fixes the reasoning in 55a3915 (kind recreates the missing parent itself at 0755, so the mode is the real issue) and the test now checks the 0700. Happy to take the |
|
The reasoning is right, the code does not follow it yet.
func TestProbe(t *testing.T) {
first, err := clusterKubeconfigPath("probe-a")
require.NoError(t, err)
dir := filepath.Dir(first)
require.NoError(t, os.RemoveAll(dir))
require.NoError(t, os.MkdirAll(dir, 0o755)) // kind's doing, per this commit
_, err = clusterKubeconfigPath("probe-b")
require.NoError(t, err)
info, err := os.Stat(dir)
require.NoError(t, err)
require.Equal(t, os.FileMode(0o700), info.Mode().Perm())
}Gives
Two lines: Low impact either way. Separately: you checked v0.32.0, Happy to approve with those two lines in. Everything else holds on c3ee82b, 26 checks pass, and |
|
Great! A few things I would rather handle now, before we move forward:
|
|
Hey @vyncint, Please rebase this PR, and resolve the conflicts! |
kind locks the kubeconfig it updates and the lock is non-blocking, so concurrent clusters sharing the default one lose the race and exit 1. That failed tests whose bodies had already passed, during cleanup.Reproduced with 96 concurrent deletes of clusters that do not exist: 7 fail with "failed to lock config file" on a shared kubeconfig, none on their own.Delete also discarded kind's stderr, which is why the failure only ever reported "exit status 1". Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The delete that clears a half-built cluster after a create times out did not name the kubeconfig the create used, so it fell back to the shared default. That is the delete most likely to lose the lock, because it runs exactly when several creates are timing out together.The stalled-create case creates with a kubeconfig now, so the delete's arguments are pinned rather than only its presence: without the fix it reports `delete cluster --name stuck` against the expected `delete cluster --kubeconfig /tmp/kind-stuck.kubeconfig --name stuck`. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. Two problems with the per-cluster kubeconfig path.Nothing removed the file. kindCluster.Cleanup only removes the os.CreateTemp copy, and kind rewrites the kubeconfig it manages rather than unlinking it, so a run that was killed handed its file, and any lock left mid-write, to the next one.The path was also predictable in a shared directory, where GetTestCluster already uses os.CreateTemp for the same kind of file. The names now sit inside one os.MkdirTemp directory per run, created 0700, so nothing planted at a guessable path can be written through. DeleteTestCluster removes the kubeconfig and the lock once the cluster is gone, and the directory goes with the last cluster of the binary.common had no tests. These cover the path being per-cluster, stable across calls so create and delete agree, outside the shared temp directory, and the removal taking the lock with it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
…finish Review feedback on kube-logging#2301. Two RequireNoError calls in WithCluster, both of which turn a working test red or drop an error outright.RequireNoError ends in t.FailNow, which is only defined on the test goroutine. Called inside the Start goroutine it did neither of the things it looks like it does: the failure was not recorded, and a cluster that never came up resurfaced as an unrelated Eventually timeout further down. Start's result now travels back over a channel and is asserted in the deferred block, on the test goroutine and after the context is canceled. The bounded wait is there so a runnable that will not stop is named rather than left to exhaust the binary's -timeout.The teardown delete used RequireNoError where lines above it use assert.NoError, so a cluster that came down cleanly still failed its test whenever kind's kubeconfig bookkeeping errored. In 30554200418 TestFluentbitAgentDedicatedNamespace logged its coverage collection and then failed only there. It matches the assertions around it now, and FailNow no longer cuts the rest of the teardown short. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The sharding wrapped around t.Parallel was dead and inverted.sequence is a per-binary counter and ten of the thirteen suites hold exactly one test, so localSeq is always 1 in those binaries. With SHARDS=2 that gives 1 % 2 != 0 for shard 0, which skips, against 1 % 2 == 1 for shard 1, which runs: shard 0 would run nothing and shard 1 everything. Nothing exercised it either way, because the matrix that would set SHARD and SHARDS is commented out in e2e.yaml.E2E_CLUSTERS bounds the concurrency the sharding was reaching for, so this drops it rather than reworking it to shard on t.Name(), and leaves Initialize as the bare t.Parallel the suites already expect. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. LoggingTenant honours nsTenant everywhere except the watch namespace, which was the literal "tenant". WatchNamespaces takes namespaces, not loggingRefs, so it has to name the namespace the Flow and Output above were created in.Latent rather than live: both callers pass nsTenant := "tenant", so the two strings line up today. internal/fixture/tenancy.go already carries this fix, which left the live path and the fixture copy disagreeing on the same logic. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The matrix that would have driven SHARD and SHARDS was already commented out, and the sharding it fed is gone, so the block goes with it.fail-fast stays: it came in with an earlier k8s-version matrix, not this one. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2301. The comments this series added explained more than the code needed. Cut to one line each where they earn it and dropped where they did not: 27 lines of prose down to 8, plus assertion messages that only restated the expected value. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Self-review of the kubeconfig directory added earlier in this series. Two ways it could strand a cluster, both my own doing.Removing the directory with the binary's last cluster raced any create still in flight. kind writes the kubeconfig only once the control plane is up, so between a create taking its path and kind writing to it there is a window the length of a whole cluster build. Any other test finishing its teardown in that window found the directory empty and removed it, and the create then failed on a missing parent. Two parallel tests in one package are enough. The directory now stays for the run; the per-cluster file and lock still go.The path is also cached for the run by sync.OnceValues, so once anything had removed the directory every later lookup handed back a dead path. It is created on lookup now rather than assumed. The test helper had been papering over this with its own MkdirAll, which is what gave it away.Both helpers are unexported: nothing outside the package used them. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Correcting the reasoning in 55a3915, which named the wrong consequence.kind creates a missing kubeconfig parent itself, so removing the directory never failed a create the way that commit message claims. Checked against kind v0.32.0: `delete cluster --kubeconfig <missing dir>/kc` exits 0 and leaves the directory behind at 0755.0755 is the actual consequence, and a worse one for being silent: the directory exists to be 0700, and letting kind recreate it hands that away without anything failing. Both halves of that commit still stand, for this reason rather than the one given: the directory stays for the run, and the 0700 is reasserted on lookup instead of being left to MkdirTemp.The test now pins the mode rather than only the path, and fails on the missing file without the MkdirAll. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
MkdirAll returns nil without touching the mode when the directory already exists, so c3ee82b never reasserted the 0700 in the case it named. The test passed because os.RemoveAll left the directory absent, exercising only MkdirAll creating a fresh one at the mode it is given.The test now pre-creates at 0755, the state kind leaves behind, and fails without the Chmod.kind writes the parent at 0755 on both v0.31.0 and v0.32.0, so the behaviour holds on the 0.31.0 that Makefile:20 pins for CI. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The stub is a shell script the tests write to disk and drive through FAKE_KIND_* variables, and its stall paths race. TestInvocations/a stalled create deletes the partial cluster failed once in ten local runs. These run in `make test`, so the flake lands on unrelated PRs.What they covered is the kind CLI plumbing, which the e2e suite exercises against the real binary anyway. The timeout arithmetic keeps its five tests: they are pure Go with no stub behind them, and they hold the invariant that a derived cap stays under the enclosing deadline. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
…poll c.expectedResult() was an argument to Should, so Go evaluated it once before Eventually started polling. The "full list" case reads the live namespace set, and envtest never finishes deleting a namespace, so the snapshot went stale as the rest of the package created more and the poll could never match.Seen in CI as `--- FAIL: TestWatchNamespaces (10.01s)`, with test-f5c4d243, a namespace belonging to a different test in the package, sitting in the expected list.Measured: as an argument the expectation is evaluated once against however many polls run; inside the poll it is evaluated once per attempt, so both sides are read together and a set still settling converges. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
c30dc38 to
4c52374
Compare
vyncint
changed the title
|
Hi @csatib02, I rebased this PR, and resolved the conflicts. Please help me review it again! |
|
One thing lost in your rebase. |
-parallel bounds tests inside a binary; -p bounds the binaries and still defaults to GOMAXPROCS, so peak clusters followed the core count. Measured on a harness with the suite's package and test shape: 5 at four cores, 10 at eight, 15 at sixteen. Pinning -p holds it at 5 everywhere. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
common.RequireNoError calls t.FailNow, which the testing package requires to be called from the goroutine running the test. testify's require.Eventually runs its condition on its own goroutine, so ten of these calls were undefined: the error was dropped and the suite failed later on the Eventually deadline, naming the wait rather than the cause. Two sites in fluentd-aggregator, four in fluentd-aggregator-detached and four in syslog-ng-aggregator-detached now log and return false, which also turns a transient API error into a retry instead of a hard stop. The excess-config Create tolerates AlreadyExists, because the List guarding it can be stale by the time a retry runs. Same class as the LogProducer fix in kube-logging#2295 and the cluster.Start fix in kube-logging#2301. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
go test defaults -parallel to GOMAXPROCS, so peak live clusters follows the runner's core count: 8 today, from fluentd-aggregator's 4 tests plus volumedrain's 2 plus one each from two more suites. Every one of them imports the same six images, and all observed 16m deadline hits were kind load, none were kind create. -parallel 2 caps the peak at 6 and stops it growing with the core count. This reverses the reasoning that dropped -parallel 1 from kube-logging#2301, which measured fluentd-aggregator alone. elasticsearch-multiversion has two test functions but only one builds a cluster, so -parallel cannot serialise the suite; it is instead the largest beneficiary of a lower peak, because its runtime is mostly Elasticsearch readiness and that is what starves first. Measured: 856.4s at -parallel 4 (30625716801) against 387.9s at -parallel 1 (30590989952), while fluentd-aggregator went 272.5s to 490.7s. The longest package, which is what the job waits for, therefore fell from 856s to 491s. The kind command cap stays at 16m. Anchoring it to healthy timings was already measured as a regression: at 10m, runs failed that would have passed, because a degraded runner is 2.3-6.2x slower and the loads complete rather than wedge. -parallel 1 and 4 are measured; 2 is interpolated between them. If load timeouts survive this, 1 is the fallback and has two green runs behind it. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
kind deleteexits 1 under concurrency. kind takes a non-blocking lock on the kubeconfig it updates, so clusters sharing the default one lose the race and fail after the test body has passed. 96 concurrent deletes of clusters that do not exist: 7 fail withfailed to lock config fileon a shared kubeconfig, 0 when each names its own. Giving every cluster its own--kubeconfigalso stops the suite rewriting the developer's~/.kube/config. The delete path discarded kind's stderr, which is why this only ever surfaced asexit status 1; it now reports what kind said.Rebased on #2302
The Makefile change came out in the rebase, since #2302 caps concurrency with
-parallel ${E2E_SUITE_PARALLEL}, and is back on your note. The two are not the same bound:-parallellimits tests inside a binary,-p ${E2E_CLUSTERS}limits the binaries, and-pstill defaults toGOMAXPROCS. Measured on a harness with this suite's package and test shape, peak clusters is 5 at four cores, 10 at eight and 15 at sixteen; pinning-pholds it at 5 everywhere. Both knobs are inCLAUDE.mdnow, beside the ones #2302 documented.One correction to what I wrote above earlier: the peak is not
E2E_CLUSTERStimesE2E_SUITE_PARALLEL. That product is 8, but ten of the thirteen suites hold a single test, so the measured ceiling is 5.Two conflicts resolved by hand, both in
KindClusterKubeconfig:createvariable fix(e2e): remove the elasticsearch kill loop and five flake sources #2302 introduced, so the retry reuses them.Also from review
options.Kubeconfig, so it fell back to the shared default — and it runs exactly when several creates are timing out together.kindCluster.Cleanuponly removes theos.CreateTempcopy, and kind rewrites the file it manages rather than unlinking it, so a killed run handed its leftovers and any stale.lockto the next one. The files now live in oneos.MkdirTempdirectory per run at 0700, andDeleteTestClusterremoves the kubeconfig and lock once the cluster is gone.RequireNoErroroff the test goroutine.WithClustercalled it inside thecluster.Startgoroutine, wheret.FailNowis undefined, so a cluster that never started was dropped and resurfaced as an unrelatedEventuallytimeout. Start's result now comes back over a channel and is asserted in the deferred block.RequireNoErroron the teardown delete. A cluster that came down cleanly still failed its test when kind's kubeconfig bookkeeping errored. It matches theassert.NoErrorcalls above it now.sequenceis a per-binary counter and ten of the thirteen suites hold exactly one test, so withSHARDS=2shard 0 would run nothing and shard 1 everything. Gone, along with the commented-out matrix and its env vars;Initializeis the baret.Parallel.helpers.gohardcoded the tenant watch namespace.LoggingTenanthonoursnsTenanteverywhere else. Latent, since both callers pass"tenant", but it left the live path disagreeing with theinternal/fixturecopy.TestWatchNamespacesrecomputes its expectation on every poll instead of capturing it once.Verification
make checkpasses,golangci-lint run --max-same-issues=0 --max-issues-per-linter=0ine2e/reports 0 issues, and all thirteen commits build, vet and test on their own after the rebase. Negative controls, re-run on the rebased head: putting the kubeconfigs back in the shared temp directory failsTestClusterKubeconfigPathonShould not be: "/tmp", and skipping the lock removal failsTestRemoveClusterKubeconfigonfile ".../kind-gamma.kubeconfig.lock" exists.One thing your two requests collided on, worth flagging rather than quietly leaving. The pin you asked for on the partial-cluster delete lived in
TestInvocations/a stalled create deletes the partial cluster, and that went with the stub tests you later asked to drop. The fix itself is still incommands.go, but nothing covers it now, and the same is true of the stderr capture. Both are cheap to re-pin without a shell stub if you want them held; say the word and I will, otherwise they ride on the real binary like the rest of the CLI plumbing.On the evidence, unchanged: the stderr capture that would name a delete failure ships in the same commit as the fix, so the lock race is still inferred from the local experiment rather than observed in CI.