Repository navigation
test(controller): stop waiting on a CRD rebuild the apiserver may never do - #357
Merged
Merged
Conversation
…er do TestLiveMessageMigration_StoredValuesAndRemoval failed roughly a third of the time on a loaded machine, on main as much as on any branch, and cost reruns on pull requests that had nothing to do with it. The cause is not slowness. To store a pre-migration GitTarget the test has to get it past the validation that forbids that shape, so it stripped the rules off the CRD, wrote the objects, put the rules back, and waited for the apiserver to enforce them again. That last wait is the one that fails, and no timeout fixes it: the apiextensions apiserver rebuilds a CRD's serving handler asynchronously after a spec write, and under CPU pressure that rebuild is sometimes missed outright. The restored rules are present and readable in the live CRD while the previously compiled schema goes on validating, and nothing ever retriggers it. Measured under load: 571 polls across 60 seconds with the rules stored and unenforced, then enforcement immediately after one further, metadata-only write. Widening the budget is therefore useless, and was measured to be: every run that passed did so inside 6.3s -- comfortably under the original 10s -- and every run that failed burned its whole budget, whether that was 10 seconds or 60. Nothing ever landed in between. Forcing the rebuild instead is worse: rewriting the CRD on each poll invalidates the rebuild already in flight so it never converges (4 failures in 12), and writing once per settle window is no better than leaving it alone (3 in 12). So this removes the restore rather than tuning it. Proving the SHIPPED schema rejects the legacy shape does not need a CRD edited at runtime -- it is a stronger assertion against the CRD exactly as installed -- and that now lives in its own test, which touches no CRD and so has no rebuild to race. What remains here is the migration itself: strip the validation once, store the legacy objects, and never put it back. Coverage is not reduced. The restore's assertion is now made directly on the real schema instead of one reassembled mid-test. Verified against the stressor that reproduces the original: 40 consecutive passes, where the old shape failed 5 times in 10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe tests add shared helpers for CRD-based validation and legacy GitTarget construction. They verify that the unmodified CRD rejects the legacy shape, accepts the migrated ChangesLive message validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This test-only change improves CRD migration coverage and removes an intermittent wait without changing production behavior. It is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The previous commit stopped the test putting the validation back, which removed the wait that was failing. It still took the rules off a CRD the apiserver was already serving, so one handler rebuild remained -- the same kind that is sometimes missed, just in the direction that had not been observed to fail yet. There is no need to edit a live CRD at all. The shipped YAML is read, the retirement rules on spec.commit.message are dropped in memory, and the result is handed to envtest as the CRD to install. The apiserver compiles that schema once, on startup, and is never asked to rebuild anything. So the migration test now performs no CRD writes and contains no Eventually at all: it is immune to the rebuild race by construction rather than by measurement. Storing the legacy objects is a plain require.NoError. A guard keeps the pair honest. Relaxing rules that are no longer there would leave a test that proves nothing, so the helper asserts the shipped CRD still carries them, and TestLiveMessageValidation_RejectsLegacyAcceptsMigrated independently proves they bite -- it applies the very same legacy object against a CRD that is never modified, and requires the rejection. Coverage rises to 77.9%; baseline bumped as cover-check asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
What
TestLiveMessageMigration_StoredValuesAndRemovalfailed roughly a third of the time on a loaded machine — onmainas much as on any branch — and has been costing reruns on pull requests that have nothing to do with it. It took one out on #354.This removes the race rather than tuning it.
It is not a slow wait, it is a stuck one
To store a pre-migration GitTarget the test has to get a shape past the validation that forbids it. So it stripped the rules off the CRD, wrote the objects, put the rules back, and waited for the apiserver to enforce them again. That last wait is what fails.
The apiextensions apiserver rebuilds a CRD's serving handler asynchronously after a spec write, and under CPU pressure that rebuild is sometimes missed outright. Instrumenting the failing wait:
571 applies across 60 seconds, every one accepted — while the restored rules sit in the live CRD, readable. The previously compiled schema just goes on validating, and nothing retriggers it. Confirmed by probe: one further metadata-only write to the CRD made enforcement immediate.
So a longer timeout cannot work, and does not
Measured directly, because it was the obvious first thing to try:
Nothing ever landed in between. The outcome is bimodal: converge in about two seconds, or never. Widening 10s → 60s rescued exactly zero runs and added 50 seconds to each failure.
What was tried, under one stressor
Rewriting each poll is actively harmful: every write invalidates the rebuild already in flight, so it never converges.
The change
Two tests, and neither writes to a CRD at all:
TestLiveMessageValidation_RejectsLegacyAcceptsMigrated(new) asserts the shipped schema rejects the legacy shape and accepts the migrated one, against the CRD exactly as installed, never edited. No rebuild exists to race.TestLiveMessageMigration_StoredValuesAndRemovalkeeps the migration mechanics. Getting the pre-migration shape stored still means relaxing those rules, but the relaxation now happens to the CRD before it is installed — the shipped YAML is read, the rules are dropped in memory, and the result is handed toenvtest.Environment{CRDs: ...}. The apiserver compiles that schema once at startup and is never asked to rebuild.The migration test therefore performs no CRD writes and contains no
Eventuallyat all. It is immune to the rebuild race by construction, not by measurement.A guard keeps the pair honest: relaxing rules that no longer exist would leave a test proving nothing, so the helper asserts the shipped CRD still carries them (
shipped CRD no longer carries the rules this test relaxes), and Test A independently proves they bite — it applies the very same legacy object against an unmodified CRD and requires the rejection.Why not kcp
Worth recording, since it came up. kcp workspaces in this repo are the watched side — cheap logical clusters that get mirrored through a ClusterProvider kubeconfig — while the GitTarget/ClusterProvider config plane lives in the management cluster. In the operator, kcp appears only as a sanitize concern (
kcp.io/cluster). There is no coupling between a kcp workspace and the config plane, and building one to serve this test would trade a 3-second unit test for part of an opt-in e2e leg that is skipped by default. The isolation argument was also already satisfied: this runs in envtest, a throwaway apiserver. What needed removing was the in-place schema edit, not the shared environment.Coverage is not reduced. What the restore used to prove is now proved more directly, on the real schema rather than one reassembled mid-test.
Verification
task lint,task test(77.8%, baseline held),task test-e2e(85 passed, 0 failed).🤖 Generated with Claude Code