diff --git a/e2e/input_removal_test.go b/e2e/input_removal_test.go new file mode 100644 index 00000000..63c37c79 --- /dev/null +++ b/e2e/input_removal_test.go @@ -0,0 +1,160 @@ +package e2e + +import ( + "context" + "fmt" + "reflect" + "testing" + "time" + + flow "github.com/Azure/go-workflow" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" + + apiv1 "github.com/Azure/eno/api/v1" + fw "github.com/Azure/eno/e2e/framework" +) + +// TestSynthesizerInputRemoval verifies that removing a ref from a synthesizer doesn't block resynthesis. +// +// The composition keeps its binding for the removed ref (as when bindings are inherited from a Symphony), +// and the removed input carries a synthesizer-generation annotation. Unless its input revision is pruned, +// that stale revision holds the composition out of lockstep and the new synthesis never happens. +func TestSynthesizerInputRemoval(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + cli := fw.NewClient(t) + + synthName := fw.UniqueName("input-removal-synth") + compName := fw.UniqueName("input-removal-comp") + outputName := fw.UniqueName("input-removal-output") + compKey := types.NamespacedName{Name: compName, Namespace: "default"} + + configA := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: fw.UniqueName("input-removal-configa"), Namespace: "default"}, + Data: map[string]string{"valueA": "alpha"}, + } + configB := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: fw.UniqueName("input-removal-configb"), + Namespace: "default", + // Ties configb's revision to the synthesizer's first generation, so it goes stale once the ref is removed + Annotations: map[string]string{"eno.azure.io/synthesizer-generation": "1"}, + }, + Data: map[string]string{"valueB": "bravo"}, + } + + // Copies every "valueA"/"valueB" entry found in the inputs into the output ConfigMap + script := fmt.Sprintf(`data=$(grep -o '"value[AB]":"[^"]*"' | sort -u | paste -sd, - || true) +printf '{"apiVersion":"config.kubernetes.io/v1","kind":"ResourceList","items":[{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"%s","namespace":"default"},"data":{%%s}}]}' "$data"`, outputName) + + configMapRef := apiv1.ResourceRef{Version: "v1", Kind: "ConfigMap"} + synth := fw.NewMinimalSynthesizer(synthName, fw.WithCommand([]string{"/bin/bash", "-c", script})) + synth.Spec.Refs = []apiv1.Ref{ + {Key: "configa", Resource: configMapRef}, + {Key: "configb", Resource: configMapRef}, + } + + comp := fw.NewComposition(compName, "default", fw.WithSynthesizerRefs(apiv1.SynthesizerRef{Name: synthName})) + comp.Spec.Bindings = []apiv1.Binding{ + {Key: "configa", Resource: apiv1.ResourceBinding{Name: configA.Name, Namespace: configA.Namespace}}, + {Key: "configb", Resource: apiv1.ResourceBinding{Name: configB.Name, Namespace: configB.Namespace}}, + } + + var initialSynthGen int64 + + // -- Define workflow steps -- + + createConfigA := fw.CreateStep(t, "createConfigA", cli, configA) + createConfigB := fw.CreateStep(t, "createConfigB", cli, configB) + createSynthesizer := fw.CreateStep(t, "createSynthesizer", cli, synth) + createComposition := fw.CreateStep(t, "createComposition", cli, comp) + + waitReady := flow.Func("waitReady", func(ctx context.Context) error { + fw.WaitForCompositionReady(t, ctx, cli, compKey, 3*time.Minute) + current := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, compKey, current)) + require.NotNil(t, current.Status.CurrentSynthesis) + initialSynthGen = current.Status.CurrentSynthesis.ObservedSynthesizerGeneration + t.Logf("initial ObservedSynthesizerGeneration: %d", initialSynthGen) + return nil + }) + + verifyInitialOutput := flow.Func("verifyInitialOutput", func(ctx context.Context) error { + waitForOutputData(t, ctx, cli, outputName, map[string]string{"valueA": "alpha", "valueB": "bravo"}) + t.Log("verified output contains both inputs") + return nil + }) + + removeConfigBRef := flow.Func("removeConfigBRef", func(ctx context.Context) error { + require.NoError(t, cli.Get(ctx, types.NamespacedName{Name: synthName}, synth)) + synth.Spec.Refs = []apiv1.Ref{{Key: "configa", Resource: configMapRef}} + t.Log("removing configb ref from synthesizer (composition binding is left in place)") + return cli.Update(ctx, synth) + }) + + waitResynthesized := flow.Func("waitResynthesized", func(ctx context.Context) error { + fw.WaitForCompositionResynthesized(t, ctx, cli, compKey, initialSynthGen, 3*time.Minute) + t.Log("composition re-synthesized and ready") + return nil + }) + + verifyUpdatedOutput := flow.Func("verifyUpdatedOutput", func(ctx context.Context) error { + waitForOutputData(t, ctx, cli, outputName, map[string]string{"valueA": "alpha"}) + + current := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, compKey, current)) + for _, rev := range current.Status.InputRevisions { + require.NotEqual(t, "configb", rev.Key, "input revision for removed ref should be pruned") + } + t.Log("verified output contains only the remaining input") + return nil + }) + + deleteComposition := fw.DeleteStep(t, "deleteComposition", cli, comp) + + verifyOutputDeleted := flow.Func("verifyOutputDeleted", func(ctx context.Context) error { + output := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: outputName, Namespace: "default"}} + fw.WaitForResourceDeleted(t, ctx, cli, output, 60*time.Second) + return nil + }) + + cleanup := fw.CleanupStep(t, "cleanup", cli, synth, configA, configB) + + // -- Wire the DAG -- + + w := new(flow.Workflow) + w.Add( + flow.Step(createComposition).DependsOn(createConfigA, createConfigB, createSynthesizer), + flow.Step(waitReady).DependsOn(createComposition), + flow.Step(verifyInitialOutput).DependsOn(waitReady), + flow.Step(removeConfigBRef).DependsOn(verifyInitialOutput), + flow.Step(waitResynthesized).DependsOn(removeConfigBRef), + flow.Step(verifyUpdatedOutput).DependsOn(waitResynthesized), + flow.Step(deleteComposition).DependsOn(verifyUpdatedOutput), + flow.Step(verifyOutputDeleted).DependsOn(deleteComposition), + flow.Step(cleanup).DependsOn(verifyOutputDeleted), + ) + + require.NoError(t, w.Do(ctx)) +} + +// waitForOutputData polls until the output ConfigMap's data exactly matches expected. +func waitForOutputData(t *testing.T, ctx context.Context, cli client.Client, name string, expected map[string]string) { + t.Helper() + err := wait.PollUntilContextTimeout(ctx, 2*time.Second, time.Minute, true, func(ctx context.Context) (bool, error) { + output := &corev1.ConfigMap{} + if err := cli.Get(ctx, types.NamespacedName{Name: name, Namespace: "default"}, output); err != nil { + return false, nil + } + t.Logf("output configmap %s data: %v", name, output.Data) + return reflect.DeepEqual(expected, output.Data), nil + }) + require.NoError(t, err, "timed out waiting for output configmap %s to have data %v", name, expected) +} diff --git a/internal/controllers/watch/integration_test.go b/internal/controllers/watch/integration_test.go index 22b3e07e..b14fd17d 100644 --- a/internal/controllers/watch/integration_test.go +++ b/internal/controllers/watch/integration_test.go @@ -519,6 +519,77 @@ func TestRemoveInput(t *testing.T) { }) } +// TestRemoveSynthesizerRefWithLingeringBinding covers a synthesizer dropping a ref while the +// composition still carries a binding for it (e.g. inherited from a Symphony). +// The stale revision must be pruned, otherwise it holds the composition out of lockstep forever. +func TestRemoveSynthesizerRefWithLingeringBinding(t *testing.T) { + mgr := testutil.NewManager(t) + require.NoError(t, NewController(mgr.Manager)) + mgr.Start(t) + + ctx := testutil.NewContext(t) + cli := mgr.GetClient() + + for _, name := range []string{"input-a", "input-b"} { + cm := &corev1.ConfigMap{} + cm.Name = name + cm.Namespace = "default" + cm.Annotations = map[string]string{"eno.azure.io/synthesizer-generation": "1"} + require.NoError(t, cli.Create(ctx, cm)) + } + + refB := apiv1.Ref{Key: "b", Resource: apiv1.ResourceRef{Version: "v1", Kind: "ConfigMap"}} + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + synth.Spec.Refs = []apiv1.Ref{ + {Key: "a", Resource: apiv1.ResourceRef{Version: "v1", Kind: "ConfigMap"}}, + refB, + } + require.NoError(t, cli.Create(ctx, synth)) + + comp := &apiv1.Composition{} + comp.Name = "test-comp" + comp.Namespace = "default" + comp.Spec.Synthesizer.Name = synth.Name + comp.Spec.Bindings = []apiv1.Binding{ + {Key: "a", Resource: apiv1.ResourceBinding{Name: "input-a", Namespace: "default"}}, + {Key: "b", Resource: apiv1.ResourceBinding{Name: "input-b", Namespace: "default"}}, + } + require.NoError(t, cli.Create(ctx, comp)) + + testutil.Eventually(t, func() bool { + cli.Get(ctx, client.ObjectKeyFromObject(comp), comp) + return len(comp.Status.InputRevisions) == 2 + }) + + // Remove ref "b" from the synthesizer but leave the binding on the composition + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + cli.Get(ctx, client.ObjectKeyFromObject(synth), synth) + synth.Spec.Refs = synth.Spec.Refs[:1] + return cli.Update(ctx, synth) + }) + require.NoError(t, err) + + testutil.Eventually(t, func() bool { + cli.Get(ctx, client.ObjectKeyFromObject(comp), comp) + return len(comp.Status.InputRevisions) == 1 && comp.Status.InputRevisions[0].Key == "a" + }) + assert.Len(t, comp.Spec.Bindings, 2, "binding should be left untouched") + + // Adding the ref back restores the revision + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + cli.Get(ctx, client.ObjectKeyFromObject(synth), synth) + synth.Spec.Refs = append(synth.Spec.Refs, refB) + return cli.Update(ctx, synth) + }) + require.NoError(t, err) + + testutil.Eventually(t, func() bool { + cli.Get(ctx, client.ObjectKeyFromObject(comp), comp) + return len(comp.Status.InputRevisions) == 2 + }) +} + // TestConcurrentStatusUpdates verifies that the retry mechanism handles // concurrent updates to composition status without causing a retry storm. // This tests the fix for the issue where conflict errors would cause immediate diff --git a/internal/controllers/watch/pruning.go b/internal/controllers/watch/pruning.go index e5341dc4..f4e3d18d 100644 --- a/internal/controllers/watch/pruning.go +++ b/internal/controllers/watch/pruning.go @@ -4,10 +4,16 @@ import ( "context" apiv1 "github.com/Azure/eno/api/v1" + "github.com/Azure/eno/internal/manager" "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) type pruningController struct { @@ -33,40 +39,106 @@ func (c *pruningController) Reconcile(ctx context.Context, req ctrl.Request) (ct synth := &apiv1.Synthesizer{} synth.Name = comp.Spec.Synthesizer.Name err = c.client.Get(ctx, client.ObjectKeyFromObject(synth), synth) - if client.IgnoreNotFound(err) != nil { + if errors.IsNotFound(err) { + // Without the synthesizer we can't tell which refs are still declared, so don't prune anything + return ctrl.Result{}, nil + } + if err != nil { logger.Error(err, "failed to get synthesizer") return ctrl.Result{}, err } - for i, ir := range comp.Status.InputRevisions { - if hasBindingKey(comp, synth, ir.Key) { + kept := make([]apiv1.InputRevisions, 0, len(comp.Status.InputRevisions)) + var pruned []string + for _, ir := range comp.Status.InputRevisions { + if shouldKeepInputRevision(comp, synth, ir.Key) { + kept = append(kept, ir) continue } - logger.Info("pruning input revision - no longer has binding", "key", ir.Key, "revision", ir.Revision, "index", i) - comp.Status.InputRevisions = append(comp.Status.InputRevisions[:i], comp.Status.InputRevisions[i+1:]...) - err = c.client.Status().Update(ctx, comp) - if err != nil { - logger.Error(err, "failed to update composition status") - return ctrl.Result{}, err - } - - logger.Info("pruned old input revision from composition status", "compositionName", comp.Name, "compositionNamespace", comp.Namespace, "ref", ir.Key) + pruned = append(pruned, ir.Key) + } + if len(pruned) == 0 { return ctrl.Result{}, nil } + logger.Info("pruning input revisions - no longer declared by synthesizer or bound by composition", "keys", pruned, "synthesizerGeneration", synth.Generation) + comp.Status.InputRevisions = kept + err = c.client.Status().Update(ctx, comp) + if err != nil { + logger.Error(err, "failed to update composition status") + return ctrl.Result{}, err + } + + logger.Info("pruned old input revisions from composition status", "keys", pruned) return ctrl.Result{}, nil } -func hasBindingKey(comp *apiv1.Composition, synth *apiv1.Synthesizer, key string) bool { - for _, b := range comp.Spec.Bindings { - if b.Key == key { - return true - } - } +// shouldKeepInputRevision reports whether the input revision for key should be kept. Both must be true: +// - The synthesizer still has a ref with this key. +// - The ref points to a resource, either by naming one itself or through a matching composition binding. +// +// A composition binding by itself doesn't count. It can stay behind after the ref is removed +// (e.g. when the binding comes from a Symphony). +func shouldKeepInputRevision(comp *apiv1.Composition, synth *apiv1.Synthesizer, key string) bool { for _, ref := range synth.Spec.Refs { - if ref.Key == key && ref.Resource.Name != "" { + if ref.Key != key { + continue + } + if ref.Resource.Name != "" { return true // implicit binding } + for _, b := range comp.Spec.Bindings { + if b.Key == key { + return true + } + } + return false } return false } + +// newSynthEventHandler enqueues every composition that uses the synthesizer, so revisions for refs +// removed from the synthesizer are pruned without waiting for an unrelated composition event. +func (c *pruningController) newSynthEventHandler() handler.TypedEventHandler[*apiv1.Synthesizer, reconcile.Request] { + fn := func(ctx context.Context, synth *apiv1.Synthesizer) []reconcile.Request { + logger := logr.FromContextOrDiscard(ctx) + + list := &apiv1.CompositionList{} + err := c.client.List(ctx, list, client.MatchingFields{ + manager.IdxCompositionsBySynthesizer: synth.Name, + }) + if err != nil { + logger.Error(err, "failed to list compositions for synthesizer", "synthesizerName", synth.Name) + return nil + } + + reqs := make([]reconcile.Request, 0, len(list.Items)) + for i := range list.Items { + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) + } + return reqs + } + return handler.TypedEnqueueRequestsFromMapFunc(fn) +} + +// synthRefsChangedPredicate limits synthesizer events to the ones that can leave stale input revisions: +// - Create: a synthesizer created after its compositions; earlier prune passes skipped them. +// - Update: only when spec.refs changed. Other changes (e.g. image) can't make a revision stale. +// +// Deletes are dropped because the pruner doesn't prune without a synthesizer. +func synthRefsChangedPredicate() predicate.TypedPredicate[*apiv1.Synthesizer] { + return predicate.TypedFuncs[*apiv1.Synthesizer]{ + CreateFunc: func(e event.TypedCreateEvent[*apiv1.Synthesizer]) bool { + return true + }, + UpdateFunc: func(e event.TypedUpdateEvent[*apiv1.Synthesizer]) bool { + return !equality.Semantic.DeepEqual(e.ObjectOld.Spec.Refs, e.ObjectNew.Spec.Refs) + }, + DeleteFunc: func(e event.TypedDeleteEvent[*apiv1.Synthesizer]) bool { + return false + }, + GenericFunc: func(e event.TypedGenericEvent[*apiv1.Synthesizer]) bool { + return false + }, + } +} diff --git a/internal/controllers/watch/pruning_test.go b/internal/controllers/watch/pruning_test.go new file mode 100644 index 00000000..e56be166 --- /dev/null +++ b/internal/controllers/watch/pruning_test.go @@ -0,0 +1,158 @@ +package watch + +import ( + "testing" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/Azure/eno/internal/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestShouldKeepInputRevision(t *testing.T) { + tests := []struct { + name string + refs []apiv1.Ref + bindings []apiv1.Binding + key string + expected bool + }{ + { + name: "ref with explicit binding", + refs: []apiv1.Ref{{Key: "foo"}}, + bindings: []apiv1.Binding{{Key: "foo"}}, + key: "foo", + expected: true, + }, + { + name: "ref with implicit binding", + refs: []apiv1.Ref{{Key: "foo", Resource: apiv1.ResourceRef{Name: "input"}}}, + key: "foo", + expected: true, + }, + { + name: "ref without any binding", + refs: []apiv1.Ref{{Key: "foo"}}, + key: "foo", + expected: false, + }, + { + name: "binding outlives removed ref", + refs: []apiv1.Ref{{Key: "bar"}}, + bindings: []apiv1.Binding{{Key: "foo"}, {Key: "bar"}}, + key: "foo", + expected: false, + }, + { + name: "synthesizer without refs", + bindings: []apiv1.Binding{{Key: "foo"}}, + key: "foo", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Spec.Bindings = tt.bindings + synth := &apiv1.Synthesizer{} + synth.Spec.Refs = tt.refs + + assert.Equal(t, tt.expected, shouldKeepInputRevision(comp, synth, tt.key)) + }) + } +} + +func TestPruningControllerReconcile(t *testing.T) { + newSynth := func() *apiv1.Synthesizer { + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + synth.Spec.Refs = []apiv1.Ref{{Key: "keep"}} + return synth + } + newComp := func() *apiv1.Composition { + comp := &apiv1.Composition{} + comp.Name = "test-comp" + comp.Namespace = "default" + comp.Spec.Synthesizer.Name = "test-synth" + comp.Spec.Bindings = []apiv1.Binding{{Key: "keep"}, {Key: "removed-1"}, {Key: "removed-2"}} + comp.Status.InputRevisions = []apiv1.InputRevisions{ + {Key: "removed-1", ResourceVersion: "1"}, + {Key: "keep", ResourceVersion: "2"}, + {Key: "removed-2", ResourceVersion: "3"}, + } + return comp + } + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-comp", Namespace: "default"}} + + t.Run("prunes all undeclared refs in one pass", func(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, newSynth(), newComp()) + c := &pruningController{client: cli} + + _, err := c.Reconcile(ctx, req) + require.NoError(t, err) + + comp := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, req.NamespacedName, comp)) + assert.Equal(t, []apiv1.InputRevisions{{Key: "keep", ResourceVersion: "2"}}, comp.Status.InputRevisions) + }) + + t.Run("synthesizer missing", func(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, newComp()) + c := &pruningController{client: cli} + + _, err := c.Reconcile(ctx, req) + require.NoError(t, err) + + comp := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, req.NamespacedName, comp)) + assert.Len(t, comp.Status.InputRevisions, 3) + }) + + t.Run("nothing to prune", func(t *testing.T) { + ctx := testutil.NewContext(t) + comp := newComp() + comp.Status.InputRevisions = comp.Status.InputRevisions[1:2] + cli := testutil.NewClient(t, newSynth(), comp) + c := &pruningController{client: cli} + + before := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(comp), before)) + + _, err := c.Reconcile(ctx, req) + require.NoError(t, err) + + after := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(comp), after)) + assert.Equal(t, before.ResourceVersion, after.ResourceVersion) + }) +} + +func TestSynthRefsChangedPredicate(t *testing.T) { + p := synthRefsChangedPredicate() + newSynth := func(image string, refs ...apiv1.Ref) *apiv1.Synthesizer { + synth := &apiv1.Synthesizer{} + synth.Spec.Image = image + synth.Spec.Refs = refs + return synth + } + refA := apiv1.Ref{Key: "a"} + refB := apiv1.Ref{Key: "b"} + + assert.True(t, p.Create(event.TypedCreateEvent[*apiv1.Synthesizer]{Object: newSynth("v1", refA)}), "create") + assert.False(t, p.Delete(event.TypedDeleteEvent[*apiv1.Synthesizer]{Object: newSynth("v1", refA)}), "delete") + assert.True(t, p.Update(event.TypedUpdateEvent[*apiv1.Synthesizer]{ + ObjectOld: newSynth("v1", refA, refB), + ObjectNew: newSynth("v1", refA), + }), "ref removed") + assert.False(t, p.Update(event.TypedUpdateEvent[*apiv1.Synthesizer]{ + ObjectOld: newSynth("v1", refA), + ObjectNew: newSynth("v2", refA), + }), "only image changed") +} diff --git a/internal/controllers/watch/watch.go b/internal/controllers/watch/watch.go index 37b89b42..dfc78367 100644 --- a/internal/controllers/watch/watch.go +++ b/internal/controllers/watch/watch.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/source" apiv1 "github.com/Azure/eno/api/v1" "github.com/Azure/eno/internal/flowcontrol" @@ -44,8 +45,12 @@ func NewControllerWithInputBufferWorkers(mgr ctrl.Manager, inputBufferWorkers in return err } + pruner := &pruningController{ + client: mgr.GetClient(), + } return ctrl.NewControllerManagedBy(mgr). For(&apiv1.Composition{}). + WatchesRawSource(source.Kind(mgr.GetCache(), &apiv1.Synthesizer{}, pruner.newSynthEventHandler(), synthRefsChangedPredicate())). WithLogConstructor(manager.NewLogConstructor(mgr, "watchPruningController")). Complete(&pruningController{ client: mgr.GetClient(),