Fix resource slice race condition (#212)

It's possible that the reconciler process "sees" a resource slice before the corresponding composition. Currently we handle this correctly after the initial synthesis by comparing generations. But the logic is inverted in the case that the synthesis is nil - __resource slices are deleted if there is a newer composition generation__. For example: - Composition is created - Composition hits the reconciler informers - Synthesis starts and completes - Resource slice hits the reconciler informers - The slice is deleted by `shouldDeleteSlice` - either because the composition has been updated (newer generation than resource slice), or because composition is deleted) - Synthesis status hits the reconciler informers This doesn't break tests because the integration tests use the same informers for some controllers that normally run in separate processes, so events are received in the same order by both sets of controllers. --------- Co-authored-by: Jordan Olshevski <[email protected]>
Azure · Oct 7, 2024 · a72c6bc · a72c6bc
1 parent 09a5dfe
commit a72c6bc
Show file tree

Hide file tree

Showing 2 changed files with 136 additions and 6 deletions.
diff --git a/internal/controllers/synthesis/slicecleanup.go b/internal/controllers/synthesis/slicecleanup.go
@@ -93,23 +93,23 @@ func (c *sliceCleanupController) Reconcile(ctx context.Context, req ctrl.Request
 }
 
 func shouldDeleteSlice(comp *apiv1.Composition, slice *apiv1.ResourceSlice) bool {
-	if comp.Status.CurrentSynthesis != nil && slice.Spec.CompositionGeneration > comp.Status.CurrentSynthesis.ObservedCompositionGeneration {
+	if comp.Status.CurrentSynthesis == nil || slice.Spec.CompositionGeneration > comp.Status.CurrentSynthesis.ObservedCompositionGeneration {
 		return false // stale informer
 	}
 	isOutdated := slice.Spec.Attempt != 0 && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Attempts > slice.Spec.Attempt
 	isReferencedByComp := synthesisReferencesSlice(comp.Status.CurrentSynthesis, slice) || synthesisReferencesSlice(comp.Status.PreviousSynthesis, slice)
-	isPendingSynthesis := comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil
+	isSynthesized := comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil
 	compIsDeleted := comp.DeletionTimestamp != nil
-	return isOutdated || (isPendingSynthesis && compIsDeleted || (!isReferencedByComp && slice.Spec.CompositionGeneration < comp.Generation))
+	return isOutdated || (isSynthesized && compIsDeleted || (!isReferencedByComp && slice.Spec.CompositionGeneration < comp.Generation))
 }
 
 func shouldReleaseSliceFinalizer(comp *apiv1.Composition, slice *apiv1.ResourceSlice) bool {
-	if comp.Status.CurrentSynthesis != nil && slice.Spec.CompositionGeneration > comp.Status.CurrentSynthesis.ObservedCompositionGeneration {
+	if comp.Status.CurrentSynthesis == nil || slice.Spec.CompositionGeneration > comp.Status.CurrentSynthesis.ObservedCompositionGeneration {
 		return false // stale informer
 	}
 	isOutdated := slice.Spec.Attempt != 0 && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Attempts > slice.Spec.Attempt
-	isPendingSynthesis := comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil
-	return isOutdated || (isPendingSynthesis && (!resourcesRemain(comp, slice) || (!synthesisReferencesSlice(comp.Status.CurrentSynthesis, slice) && !synthesisReferencesSlice(comp.Status.PreviousSynthesis, slice))))
+	isSynthesized := comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil
+	return isOutdated || (isSynthesized && (!resourcesRemain(comp, slice) || (!synthesisReferencesSlice(comp.Status.CurrentSynthesis, slice) && !synthesisReferencesSlice(comp.Status.PreviousSynthesis, slice))))
 }
 
 func synthesisReferencesSlice(syn *apiv1.Synthesis, slice *apiv1.ResourceSlice) bool {

diff --git a/internal/controllers/synthesis/slicecleanup_test.go b/internal/controllers/synthesis/slicecleanup_test.go
@@ -2,7 +2,9 @@ package synthesis
 
 import (
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -56,3 +58,131 @@ func TestSliceCleanupControllerOrphanedSlice(t *testing.T) {
 		return errors.IsNotFound(mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(slice), slice))
 	})
 }
+
+func TestShouldDeleteSlice(t *testing.T) {
+	tests := []struct {
+		name     string
+		comp     *apiv1.Composition
+		slice    *apiv1.ResourceSlice
+		expected bool
+	}{
+		{
+			name: "stale informer (CurrentSynthesis is nil)",
+			comp: &apiv1.Composition{
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: nil,
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					CompositionGeneration: 2,
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "stale informer (synthesis is stale)",
+			comp: &apiv1.Composition{
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: &apiv1.Synthesis{ObservedCompositionGeneration: 1},
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					CompositionGeneration: 2,
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "slice is outdated",
+			comp: &apiv1.Composition{
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: &apiv1.Synthesis{
+						Attempts: 5,
+					},
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					Attempt: 3,
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "slice is referenced by composition",
+			comp: &apiv1.Composition{
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: &apiv1.Synthesis{},
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					CompositionGeneration: 1,
+				},
+			},
+			expected: false, // assumes synthesisReferencesSlice returns true
+		},
+		{
+			name: "synthesis terminated and composition is deleted",
+			comp: &apiv1.Composition{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+				},
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: &apiv1.Synthesis{
+						Synthesized: &metav1.Time{Time: time.Now()},
+					},
+				},
+			},
+			slice:    &apiv1.ResourceSlice{},
+			expected: true,
+		},
+		{
+			name: "composition is deleted but synthesis not terminated",
+			comp: &apiv1.Composition{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+				},
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: nil,
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					CompositionGeneration: 1,
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "slice is outdated, not referenced, and comp.Generation is higher",
+			comp: &apiv1.Composition{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Generation:        5,
+				},
+				Status: apiv1.CompositionStatus{
+					CurrentSynthesis: &apiv1.Synthesis{
+						Attempts: 3,
+					},
+				},
+			},
+			slice: &apiv1.ResourceSlice{
+				Spec: apiv1.ResourceSliceSpec{
+					Attempt:               1,
+					CompositionGeneration: 2,
+				},
+			},
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := shouldDeleteSlice(tt.comp, tt.slice)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}