From 01aa424a7b9175b2f636d100de24be61c7300a24 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 10 Sep 2026 11:19:11 +0000 Subject: [PATCH 1/2] test(controller): stop waiting on a CRD rebuild the apiserver may never 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 --- .../controller/live_message_migration_test.go | 93 +++++++++++++------ 1 file changed, 67 insertions(+), 26 deletions(-) diff --git a/internal/controller/live_message_migration_test.go b/internal/controller/live_message_migration_test.go index 8fe44eaf..aad3ee18 100644 --- a/internal/controller/live_message_migration_test.go +++ b/internal/controller/live_message_migration_test.go @@ -4,7 +4,6 @@ package controller import ( "context" - "strings" "testing" "time" @@ -33,8 +32,28 @@ func applyIgnoringUnknownFields(ctx context.Context, c client.Client, obj *unstr &client.PatchOptions{Raw: &metav1.PatchOptions{FieldValidation: "Ignore"}}) } -func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { - ctx := context.Background() +const gitTargetCRDName = "gittargets.configbutler.ai" + +// legacyGitTarget is the shape a GitTarget had before liveTemplate replaced the two +// per-event templates: the documents this migration has to keep working for. +func legacyGitTarget(name string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "configbutler.ai/v1alpha3", "kind": "GitTarget", + "metadata": map[string]any{"name": name, "namespace": "default"}, + "spec": map[string]any{ + "gitProviderRef": map[string]any{"name": "provider"}, + "branch": "main", + "path": name, + "commit": map[string]any{ + "message": map[string]any{"eventTemplate": "old event", "groupTemplate": "old group"}, + }, + }, + }} +} + +// startCRDEnv brings up an envtest apiserver with the project CRDs and returns a client. +func startCRDEnv(t *testing.T) client.Client { + t.Helper() env := &envtest.Environment{CRDDirectoryPaths: []string{"../../config/crd/bases"}, ErrorIfCRDPathMissing: true} cfg, err := env.Start() require.NoError(t, err) @@ -44,40 +63,62 @@ func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { require.NoError(t, apiextv1.AddToScheme(sch)) c, err := client.New(cfg, client.Options{Scheme: sch}) require.NoError(t, err) + + return c +} + +// TestLiveMessageValidation_RejectsLegacyAcceptsMigrated pins what the SHIPPED schema does: +// it refuses the pre-migration shape and accepts the migrated one. +// +// It runs against the CRD exactly as installed, and never edits it. That is the point. This +// assertion used to live at the end of the migration test below, which got there by stripping +// the validation and then putting it back -- and waiting for the restore to take effect is a +// race the apiserver loses often enough to matter. 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 new schema reads back correctly while the previously compiled +// one goes on validating, and nothing retriggers it. Measured under load: 571 polls across 60s +// with the restored rules present in the live CRD and still unenforced. A longer timeout does +// not help, because the wait is not slow, it is stuck -- every run that passed did so within +// 6.3s, and every run that failed used its whole budget. Asserting against an unmodified CRD +// removes the rebuild from the picture rather than waiting on it. +func TestLiveMessageValidation_RejectsLegacyAcceptsMigrated(t *testing.T) { + ctx := context.Background() + c := startCRDEnv(t) + + err := applyIgnoringUnknownFields(ctx, c, legacyGitTarget("reject-legacy")) + require.Error(t, err) + require.Contains(t, err.Error(), "liveTemplate") + + migrated := legacyGitTarget("accept-migrated") + require.NoError(t, unstructured.SetNestedMap(migrated.Object, + map[string]any{"liveTemplate": "chore: sync {{.Count}}"}, "spec", "commit", "message")) + require.NoError(t, applyIgnoringUnknownFields(ctx, c, migrated)) +} + +func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { + ctx := context.Background() + c := startCRDEnv(t) + legacy := legacyGitTarget + + // Storing the pre-migration shape means getting it past the validation that forbids it, and + // the only way in is to take that validation off the CRD. It is never put back: proving the + // shipped schema still rejects this shape is TestLiveMessageValidation_RejectsLegacyAcceptsMigrated's + // job, against an untouched CRD, precisely so that this test does not have to wait on a + // second handler rebuild it cannot make the apiserver perform. var crd apiextv1.CustomResourceDefinition - require.NoError(t, c.Get(ctx, client.ObjectKey{Name: "gittargets.configbutler.ai"}, &crd)) - current := crd.DeepCopy() + require.NoError(t, c.Get(ctx, client.ObjectKey{Name: gitTargetCRDName}, &crd)) message := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] message.XValidations = nil crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] = message require.NoError(t, c.Update(ctx, &crd)) - legacy := func(name string) *unstructured.Unstructured { - return &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "configbutler.ai/v1alpha3", "kind": "GitTarget", - "metadata": map[string]any{"name": name, "namespace": "default"}, - "spec": map[string]any{ - "gitProviderRef": map[string]any{"name": "provider"}, - "branch": "main", - "path": name, - "commit": map[string]any{ - "message": map[string]any{"eventTemplate": "old event", "groupTemplate": "old group"}, - }, - }, - }} - } + + // The first successful apply IS the evidence that the relaxed schema is being served. for _, name := range []string{"migrate-apply", "migrate-patch"} { obj := legacy(name) require.Eventually(t, func() bool { return applyIgnoringUnknownFields(ctx, c, obj) == nil }, 10*time.Second, 100*time.Millisecond) } - require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(&crd), &crd)) - crd.Spec = current.Spec - require.NoError(t, c.Update(ctx, &crd)) - require.Eventually(t, func() bool { - err := applyIgnoringUnknownFields(ctx, c, legacy("reject-new")) - return err != nil && strings.Contains(err.Error(), "liveTemplate") - }, 10*time.Second, 100*time.Millisecond) for _, name := range []string{"migrate-apply", "migrate-patch"} { var target api.GitTarget key := client.ObjectKey{Name: name, Namespace: "default"} From d0efe29887072c26b853a050f52e98a8979a9d73 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 10 Sep 2026 11:48:04 +0000 Subject: [PATCH 2/2] test(controller): relax the CRD before installing it, not after 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 --- .coverage-baseline | 2 +- .../controller/live_message_migration_test.go | 79 +++++++++++++------ 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/.coverage-baseline b/.coverage-baseline index f5bfc226..d916ff4b 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -77.8 +77.9 diff --git a/internal/controller/live_message_migration_test.go b/internal/controller/live_message_migration_test.go index aad3ee18..9d2b36dc 100644 --- a/internal/controller/live_message_migration_test.go +++ b/internal/controller/live_message_migration_test.go @@ -4,8 +4,9 @@ package controller import ( "context" + "os" + "path/filepath" "testing" - "time" "github.com/stretchr/testify/require" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -15,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/yaml" api "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) @@ -32,7 +34,7 @@ func applyIgnoringUnknownFields(ctx context.Context, c client.Client, obj *unstr &client.PatchOptions{Raw: &metav1.PatchOptions{FieldValidation: "Ignore"}}) } -const gitTargetCRDName = "gittargets.configbutler.ai" +const crdBaseDir = "../../config/crd/bases" // legacyGitTarget is the shape a GitTarget had before liveTemplate replaced the two // per-event templates: the documents this migration has to keep working for. @@ -51,10 +53,18 @@ func legacyGitTarget(name string) *unstructured.Unstructured { }} } -// startCRDEnv brings up an envtest apiserver with the project CRDs and returns a client. -func startCRDEnv(t *testing.T) client.Client { +// startEnv brings up an envtest apiserver holding exactly the given CRDs and returns a client. +// Passing the CRDs as objects rather than a directory is what lets a caller install a MODIFIED +// schema, without ever editing one that is already being served. +func startEnv(t *testing.T, crds ...*apiextv1.CustomResourceDefinition) client.Client { t.Helper() - env := &envtest.Environment{CRDDirectoryPaths: []string{"../../config/crd/bases"}, ErrorIfCRDPathMissing: true} + env := &envtest.Environment{} + if len(crds) == 0 { + env.CRDDirectoryPaths = []string{crdBaseDir} + env.ErrorIfCRDPathMissing = true + } else { + env.CRDs = crds + } cfg, err := env.Start() require.NoError(t, err) t.Cleanup(func() { require.NoError(t, env.Stop()) }) @@ -67,6 +77,38 @@ func startCRDEnv(t *testing.T) client.Client { return c } +// gitTargetCRDWithoutMessageValidation reads the shipped GitTarget CRD and returns it with the +// retirement rules on spec.commit.message removed. +// +// The point is WHEN this happens: before the apiserver has ever seen the CRD, so the relaxed +// schema is the only one it ever compiles. Editing a live CRD instead means asking it to rebuild +// a serving handler, and under CPU pressure that rebuild is sometimes missed outright -- the new +// schema reads back correctly while the previously compiled one goes on validating, with nothing +// to retrigger it. Doing the edit here removes the rebuild from the test rather than waiting on +// one, which is not something a timeout can fix: measured under load, every run that converged +// did so within 6.3s and every run that failed used its entire budget, whether that was 10 +// seconds or 60. +func gitTargetCRDWithoutMessageValidation(t *testing.T) *apiextv1.CustomResourceDefinition { + t.Helper() + raw, err := os.ReadFile(filepath.Join(crdBaseDir, "configbutler.ai_gittargets.yaml")) + require.NoError(t, err) + var crd apiextv1.CustomResourceDefinition + require.NoError(t, yaml.Unmarshal(raw, &crd)) + for i := range crd.Spec.Versions { + root := crd.Spec.Versions[i].Schema.OpenAPIV3Schema + spec := root.Properties["spec"] + commit := spec.Properties["commit"] + msg := commit.Properties["message"] + require.NotEmpty(t, msg.XValidations, "shipped CRD no longer carries the rules this test relaxes") + msg.XValidations = nil + commit.Properties["message"] = msg + spec.Properties["commit"] = commit + root.Properties["spec"] = spec + } + + return &crd +} + // TestLiveMessageValidation_RejectsLegacyAcceptsMigrated pins what the SHIPPED schema does: // it refuses the pre-migration shape and accepts the migrated one. // @@ -83,7 +125,7 @@ func startCRDEnv(t *testing.T) client.Client { // removes the rebuild from the picture rather than waiting on it. func TestLiveMessageValidation_RejectsLegacyAcceptsMigrated(t *testing.T) { ctx := context.Background() - c := startCRDEnv(t) + c := startEnv(t) err := applyIgnoringUnknownFields(ctx, c, legacyGitTarget("reject-legacy")) require.Error(t, err) @@ -97,27 +139,16 @@ func TestLiveMessageValidation_RejectsLegacyAcceptsMigrated(t *testing.T) { func TestLiveMessageMigration_StoredValuesAndRemoval(t *testing.T) { ctx := context.Background() - c := startCRDEnv(t) legacy := legacyGitTarget - // Storing the pre-migration shape means getting it past the validation that forbids it, and - // the only way in is to take that validation off the CRD. It is never put back: proving the - // shipped schema still rejects this shape is TestLiveMessageValidation_RejectsLegacyAcceptsMigrated's - // job, against an untouched CRD, precisely so that this test does not have to wait on a - // second handler rebuild it cannot make the apiserver perform. - var crd apiextv1.CustomResourceDefinition - require.NoError(t, c.Get(ctx, client.ObjectKey{Name: gitTargetCRDName}, &crd)) - message := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] - message.XValidations = nil - crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["commit"].Properties["message"] = message - require.NoError(t, c.Update(ctx, &crd)) - - // The first successful apply IS the evidence that the relaxed schema is being served. + // Storing the pre-migration shape means getting it past the validation that forbids it. That + // relaxation is applied to the CRD BEFORE it is installed, so the apiserver compiles it once + // and nothing here ever waits on a schema change. Proving the SHIPPED schema still rejects + // this shape is TestLiveMessageValidation_RejectsLegacyAcceptsMigrated's job, against a CRD + // that is never modified at all. + c := startEnv(t, gitTargetCRDWithoutMessageValidation(t)) for _, name := range []string{"migrate-apply", "migrate-patch"} { - obj := legacy(name) - require.Eventually(t, func() bool { - return applyIgnoringUnknownFields(ctx, c, obj) == nil - }, 10*time.Second, 100*time.Millisecond) + require.NoError(t, applyIgnoringUnknownFields(ctx, c, legacy(name))) } for _, name := range []string{"migrate-apply", "migrate-patch"} { var target api.GitTarget