From 40cec4f75b88a798e3e9d4b7f2424e7e4920ae72 Mon Sep 17 00:00:00 2001 From: Chih-Sheng Huang Date: Mon, 25 Nov 2024 15:16:32 -0800 Subject: [PATCH] Log the reason why swap comp synthesis (#244) Resolve #225 Log the reason why swap comp synthesis during reconciliation process --- internal/controllers/synthesis/lifecycle.go | 47 ++++++++++++++++--- .../controllers/synthesis/lifecycle_test.go | 18 ++++++- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/internal/controllers/synthesis/lifecycle.go b/internal/controllers/synthesis/lifecycle.go index 1a724bf8..5e35cd39 100644 --- a/internal/controllers/synthesis/lifecycle.go +++ b/internal/controllers/synthesis/lifecycle.go @@ -139,7 +139,9 @@ func (c *podLifecycleController) Reconcile(ctx context.Context, req ctrl.Request } // Swap the state to prepare for resynthesis if needed - if shouldSwapStates(syn, comp) { + reason, shouldSwap := shouldSwapStates(syn, comp) + if shouldSwap { + logger = logger.WithValues("reason", reason) SwapStates(comp) if err := c.client.Status().Update(ctx, comp); err != nil { return ctrl.Result{}, fmt.Errorf("swapping compisition state: %w", err) @@ -388,7 +390,7 @@ func SwapStates(comp *apiv1.Composition) { } } -func shouldSwapStates(synth *apiv1.Synthesizer, comp *apiv1.Composition) bool { +func shouldSwapStates(synth *apiv1.Synthesizer, comp *apiv1.Composition) (string, bool) { // synthesize when (either): // - synthesis has never occurred // - the spec has changed @@ -399,10 +401,43 @@ func shouldSwapStates(synth *apiv1.Synthesizer, comp *apiv1.Composition) bool { // - synthesis is not already pending // - all bound input resources exist and are in lockstep (or composition is being deleted) syn := comp.Status.CurrentSynthesis - return (syn == nil || - syn.ObservedCompositionGeneration != comp.Generation || - (!inputRevisionsEqual(synth, comp.Status.InputRevisions, syn.InputRevisions) && syn.Synthesized != nil && !comp.ShouldIgnoreSideEffects())) && - (comp.DeletionTimestamp != nil || (comp.InputsExist(synth) && !comp.InputsOutOfLockstep(synth))) + + isSynNil := syn == nil + isCompGenDiff := syn != nil && syn.ObservedCompositionGeneration != comp.Generation + isInputRevisionsEqual := syn != nil && inputRevisionsEqual(synth, comp.Status.InputRevisions, syn.InputRevisions) + isSynthesizedNotNil := syn != nil && syn.Synthesized != nil + shouldIgnoreSideEffects := comp.ShouldIgnoreSideEffects() + isCompDeleted := comp.DeletionTimestamp != nil + isCompInputsExist := comp.InputsExist(synth) + isCompInputsOutOfLockstep := comp.InputsOutOfLockstep(synth) + + reason := "" + // First condition + if isSynNil { + reason += "CurrentSynthesisEmpty" + } else { + if isCompGenDiff { + reason += "CompositionChanged" + } else if !isInputRevisionsEqual && isSynthesizedNotNil && !shouldIgnoreSideEffects { + reason += "InputsChanged" + } + } + if !(isSynNil || isCompGenDiff || (!isInputRevisionsEqual && isSynthesizedNotNil && !shouldIgnoreSideEffects)) { + return "", false + } + + // Second condition + if isCompDeleted { + reason += " && CompositionDeleted" + } else if isCompInputsExist && !isCompInputsOutOfLockstep { + reason += " && InputsInLockstep" + } + + if !(isCompDeleted || (isCompInputsExist && !isCompInputsOutOfLockstep)) { + return "", false + } + + return reason, true } func shouldBackOffPodCreation(comp *apiv1.Composition) bool { diff --git a/internal/controllers/synthesis/lifecycle_test.go b/internal/controllers/synthesis/lifecycle_test.go index e659e0f8..5b325080 100644 --- a/internal/controllers/synthesis/lifecycle_test.go +++ b/internal/controllers/synthesis/lifecycle_test.go @@ -675,10 +675,12 @@ func TestShouldSwapStates(t *testing.T) { Name string Expectation bool Composition apiv1.Composition + Reason string }{ { Name: "zero value", Expectation: true, + Reason: "CurrentSynthesisEmpty && InputsInLockstep", }, { Name: "missing input", @@ -688,6 +690,7 @@ func TestShouldSwapStates(t *testing.T) { Bindings: []apiv1.Binding{{Key: "foo"}}, }, }, + Reason: "", }, { Name: "matching input synthesis in progress", @@ -707,6 +710,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "", }, { Name: "non-matching composition generation", @@ -721,6 +725,7 @@ func TestShouldSwapStates(t *testing.T) { }, }, }, + Reason: "CompositionChanged && InputsInLockstep", }, { Name: "matching input synthesis terminal", @@ -741,6 +746,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "", }, { Name: "non-matching input synthesis terminal", @@ -762,6 +768,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "InputsChanged && InputsInLockstep", }, { Name: "non-matching input synthesis terminal ignore side effects", @@ -788,6 +795,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "", }, { Name: "non-matching input synthesis non-terminal", @@ -809,6 +817,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "", }, { Name: "non-matching input synthesis deleting", @@ -833,6 +842,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "CompositionChanged && CompositionDeleted", }, { Name: "missing input synthesis deleting", @@ -847,6 +857,7 @@ func TestShouldSwapStates(t *testing.T) { }, Status: apiv1.CompositionStatus{}, }, + Reason: "CurrentSynthesisEmpty && CompositionDeleted", }, { Name: "revision mismatch", @@ -871,6 +882,7 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "", }, { Name: "revision match", @@ -895,13 +907,17 @@ func TestShouldSwapStates(t *testing.T) { }}, }, }, + Reason: "CompositionChanged && InputsInLockstep", }, } + for _, tc := range tests { t.Run(tc.Name, func(t *testing.T) { syn := &apiv1.Synthesizer{} syn.Spec.Refs = []apiv1.Ref{{Key: "foo"}} - assert.Equal(t, tc.Expectation, shouldSwapStates(syn, &tc.Composition)) + reason, shouldSwap := shouldSwapStates(syn, &tc.Composition) + assert.Equal(t, tc.Expectation, shouldSwap) + assert.Equal(t, tc.Reason, reason) }) } }