Fix patch annotation idempotence (#37)

Apiserver sets annotations maps to nil when they are empty. Currently Eno doesn't, so it will constantly patch them. This PR fixes the bug and adds patch debug logging. --------- Co-authored-by: Jordan Olshevski <[email protected]>
Azure · Feb 6, 2024 · 287ed9d · 287ed9d
1 parent 35bd0ea
commit 287ed9d
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 3 deletions.
diff --git a/dev/deploy.yaml b/dev/deploy.yaml
@@ -44,3 +44,5 @@ spec:
         env:
           - name: PPROF_ADDR
             value: ":8888"
+          - name: INSECURE_LOG_PATCH
+            value: "true"
diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"os"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -26,6 +27,7 @@ import (
 
 // TODO: Block ResourceSlice deletion until resources have been cleaned up
 // TODO: Clean up unused resource slices older than a duration
+var insecureLogPatch = os.Getenv("INSECURE_LOG_PATCH") == "true"
 
 type Controller struct {
 	client         client.Client
@@ -169,6 +171,9 @@ func (c *Controller) reconcileResource(ctx context.Context, prev, resource *reco
 	if err != nil {
 		return fmt.Errorf("adding resource version: %w", err)
 	}
+	if insecureLogPatch {
+		logger.V(1).Info("INSECURE logging patch", "patch", string(patch))
+	}
 	err = c.upstreamClient.Patch(ctx, current, client.RawPatch(patchType, patch))
 	if err != nil {
 		return fmt.Errorf("applying patch: %w", err)

diff --git a/internal/controllers/synthesis/exec.go b/internal/controllers/synthesis/exec.go
@@ -82,6 +82,7 @@ func (c *execController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		return ctrl.Result{}, nil // old pod - don't bother synthesizing. The lifecycle controller will delete it
 	}
 
+	// TODO: Sometimes this happens concurrently?
 	refs, err := c.synthesize(ctx, syn, comp, pod)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("executing synthesizer: %w", err)
@@ -358,6 +359,10 @@ func consumeReconcileIntervalAnnotation(obj client.Object) *metav1.Duration {
 		return nil
 	}
 	delete(anno, key)
+
+	if len(anno) == 0 {
+		anno = nil // apiserver treats an empty annotation map as nil, we must as well to avoid constant patches
+	}
 	obj.SetAnnotations(anno)
 
 	dur, err := time.ParseDuration(str)

diff --git a/internal/controllers/synthesis/exec_test.go b/internal/controllers/synthesis/exec_test.go
@@ -84,8 +84,8 @@ func TestBuildResourceSlicesReconcileInterval(t *testing.T) {
 	require.Len(t, slices, 1)
 	require.Len(t, slices[0].Spec.Resources, 1)
 	require.NotNil(t, slices[0].Spec.Resources[0].ReconcileInterval)
-	assert.Equal(t, time.Second*10, slices[0].Spec.Resources[0].ReconcileInterval.Duration)                                                                                                             // it's in the manifest
-	assert.Equal(t, "{\"apiVersion\":\"mygroup/v1\",\"kind\":\"Test\",\"metadata\":{\"annotations\":{},\"name\":\"test-resource\",\"namespace\":\"test-ns\"}}\n", slices[0].Spec.Resources[0].Manifest) // it's not in the resource itself
+	assert.Equal(t, time.Second*10, slices[0].Spec.Resources[0].ReconcileInterval.Duration)                                                                                          // it's in the manifest
+	assert.Equal(t, "{\"apiVersion\":\"mygroup/v1\",\"kind\":\"Test\",\"metadata\":{\"name\":\"test-resource\",\"namespace\":\"test-ns\"}}\n", slices[0].Spec.Resources[0].Manifest) // it's not in the resource itself
 }
 
 func TestBuildResourceSlicesTombstonesVersionSemantics(t *testing.T) {

diff --git a/internal/controllers/synthesis/lifecycle.go b/internal/controllers/synthesis/lifecycle.go
@@ -115,6 +115,7 @@ func (c *podLifecycleController) Reconcile(ctx context.Context, req ctrl.Request
 			return ctrl.Result{}, fmt.Errorf("listing resource slices: %w", err)
 		}
 		if len(sliceList.Items) > 0 || len(pods.Items) > 0 {
+			logger.V(1).Info(fmt.Sprintf("refusing to remove composition finalizer because %d associated resource slices and %d pods still exist", len(sliceList.Items), len(pods.Items)))
 			return ctrl.Result{}, nil // some resources still exist
 		}
 
@@ -124,7 +125,7 @@ func (c *podLifecycleController) Reconcile(ctx context.Context, req ctrl.Request
 				return ctrl.Result{}, fmt.Errorf("removing finalizer: %w", err)
 			}
 
-			logger.Info("removed finalizer from composition because none of its resource slices remain")
+			logger.Info("removed finalizer from composition because none of its resource slices or synthesizer pods remain")
 		}
 
 		return ctrl.Result{}, nil

diff --git a/internal/controllers/synthesis/lifecycle_test.go b/internal/controllers/synthesis/lifecycle_test.go
@@ -49,6 +49,12 @@ func TestCompositionDeletion(t *testing.T) {
 		return comp.Status.CurrentState != nil && len(comp.Status.CurrentState.ResourceSlices) > 0
 	})
 
+	// Wait for the resource slice to be created
+	testutil.Eventually(t, func() bool {
+		require.NoError(t, client.IgnoreNotFound(cli.Get(ctx, client.ObjectKeyFromObject(comp), comp)))
+		return comp.Status.CurrentState != nil && comp.Status.CurrentState.ResourceSlices != nil
+	})
+
 	// Delete the composition
 	require.NoError(t, cli.Delete(ctx, comp))
 	deleteGen := comp.Generation
@@ -59,6 +65,11 @@ func TestCompositionDeletion(t *testing.T) {
 		return comp.Status.CurrentState != nil && comp.Status.CurrentState.ObservedCompositionGeneration >= deleteGen
 	})
 
+	// The composition should still exist after a bit
+	// Yeahyeahyeah a fake clock would be better but this is more obvious and not meaningfully slower
+	time.Sleep(time.Second)
+	require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(comp), comp))
+
 	// Delete the resource slice(s)
 	slices := &apiv1.ResourceSliceList{}
 	require.NoError(t, cli.List(ctx, slices))