Skip to content

Commit

Permalink
Misc. slice cleanup bugfix (#213)
Browse files Browse the repository at this point in the history 
There are two cases where the slice cleanup logic doesn't behave
correctly:

- Slices are deleted too early when the current synthesis has made more
retry attempts than slices referenced by the previous one
- The composition's generation has been incremented but a new synthesis
has not yet been written

This PR fixes both cases and carefully refactors the code.

---------

Co-authored-by: Jordan Olshevski <[email protected]>
  • Loading branch information
@jveski
jveski and Jordan Olshevski authored Oct 8, 2024
1 parent ea1aa76 commit 2db70d9
Show file tree
Hide file tree
Showing 2 changed files with 194 additions and 6 deletions.
19 changes: 14 additions & 5 deletions internal/controllers/synthesis/slicecleanup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,20 @@ func shouldDeleteSlice(comp *apiv1.Composition, slice *apiv1.ResourceSlice) bool
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)
isSynthesized := comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil
compIsDeleted := comp.DeletionTimestamp != nil
return isOutdated || (isSynthesized && compIsDeleted || (!isReferencedByComp && slice.Spec.CompositionGeneration < comp.Generation))

var (
hasBeenRetried = slice.Spec.Attempt != 0 && comp.Status.CurrentSynthesis.Attempts > slice.Spec.Attempt && slice.Spec.SynthesisUUID == comp.Status.CurrentSynthesis.UUID
isReferencedByComp = synthesisReferencesSlice(comp.Status.CurrentSynthesis, slice) || synthesisReferencesSlice(comp.Status.PreviousSynthesis, slice)
isSynthesized = comp.Status.CurrentSynthesis.Synthesized != nil
compIsDeleted = comp.DeletionTimestamp != nil
fromOldComposition = slice.Spec.CompositionGeneration < comp.Status.CurrentSynthesis.ObservedCompositionGeneration
)

// We can only safely delete resource slices when either:
// - Another retry of the same synthesis has already started
// - Synthesis is complete and the composition is being deleted
// - The slice was derived from an older composition
return hasBeenRetried || (isSynthesized && compIsDeleted) || (!isReferencedByComp && fromOldComposition)
}

func shouldReleaseSliceFinalizer(comp *apiv1.Composition, slice *apiv1.ResourceSlice) bool {
Expand Down
181 changes: 180 additions & 1 deletion internal/controllers/synthesis/slicecleanup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,30 @@ func TestShouldDeleteSlice(t *testing.T) {
expected: false,
},
{
name: "slice is outdated",
name: "another attempt started for a different synthesis, old one still references the slice",
comp: &apiv1.Composition{
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Attempts: 5,
UUID: "the-next-one",
},
PreviousSynthesis: &apiv1.Synthesis{
ResourceSlices: []*apiv1.ResourceSliceRef{{Name: "test-slice"}},
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{
Name: "test-slice",
},
Spec: apiv1.ResourceSliceSpec{
Attempt: 3,
},
},
expected: false,
},
{
name: "another attempt started for the same synthesis",
comp: &apiv1.Composition{
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Expand Down Expand Up @@ -139,6 +162,162 @@ func TestShouldDeleteSlice(t *testing.T) {
slice: &apiv1.ResourceSlice{},
expected: true,
},
{
name: "synthesis terminated and slice referenced",
comp: &apiv1.Composition{
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
ResourceSlices: []*apiv1.ResourceSliceRef{{Name: "test-slice"}},
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
},
expected: false,
},
{
name: "synthesis terminated, different composition generation, same synthesis",
comp: &apiv1.Composition{
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
CompositionGeneration: 2,
},
},
expected: false,
},
{
name: "synthesis terminated, same composition generation, different synthesis",
comp: &apiv1.Composition{
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
SynthesisUUID: "foo",
},
},
expected: false,
},
{
name: "synthesis terminated, newer composition generation, different synthesis",
comp: &apiv1.Composition{
ObjectMeta: metav1.ObjectMeta{
Generation: 3,
},
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
ObservedCompositionGeneration: 2,
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
SynthesisUUID: "foo",
CompositionGeneration: 1,
},
},
expected: true,
},
{
name: "synthesis in-progress, newer composition generation, different synthesis",
comp: &apiv1.Composition{
ObjectMeta: metav1.ObjectMeta{
Generation: 3,
},
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
ObservedCompositionGeneration: 2,
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
SynthesisUUID: "foo",
CompositionGeneration: 1,
},
},
expected: true,
},
{
name: "synthesis in-progress, newer composition generation, same synthesis",
comp: &apiv1.Composition{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
},
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
ObservedCompositionGeneration: 1,
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
CompositionGeneration: 1,
},
},
expected: false,
},
{
name: "synthesis terminated, newer composition and synthesis generation, different synthesis",
comp: &apiv1.Composition{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
},
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
ObservedCompositionGeneration: 2,
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
SynthesisUUID: "foo",
CompositionGeneration: 1,
},
},
expected: true,
},
{
name: "synthesis terminated, older composition generation, different synthesis",
comp: &apiv1.Composition{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
},
Status: apiv1.CompositionStatus{
CurrentSynthesis: &apiv1.Synthesis{
Synthesized: &metav1.Time{Time: time.Now()},
ObservedCompositionGeneration: 1,
},
},
},
slice: &apiv1.ResourceSlice{
ObjectMeta: metav1.ObjectMeta{Name: "test-slice"},
Spec: apiv1.ResourceSliceSpec{
SynthesisUUID: "foo",
CompositionGeneration: 2,
},
},
expected: false,
},
{
name: "composition is deleted but synthesis not terminated",
comp: &apiv1.Composition{
Expand Down

0 comments on commit 2db70d9

Please sign in to comment.