From 287ed9ddffefb417bba8a6d647fb28bbf62d4ec3 Mon Sep 17 00:00:00 2001 From: Jordan Olshevski Date: Tue, 6 Feb 2024 16:42:31 -0600 Subject: [PATCH] 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 --- dev/deploy.yaml | 2 ++ internal/controllers/reconciliation/controller.go | 5 +++++ internal/controllers/synthesis/exec.go | 5 +++++ internal/controllers/synthesis/exec_test.go | 4 ++-- internal/controllers/synthesis/lifecycle.go | 3 ++- internal/controllers/synthesis/lifecycle_test.go | 11 +++++++++++ 6 files changed, 27 insertions(+), 3 deletions(-) diff --git a/dev/deploy.yaml b/dev/deploy.yaml index 4e24e4e1..d89f3f7b 100644 --- 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 index 9739cf59..d3ddd7fa 100644 --- 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 index 69037b7e..cba388e4 100644 --- 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 index 8fe29615..f4aade4d 100644 --- 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 index 43d85e19..ed2034a4 100644 --- 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 index 37e550cc..6189e864 100644 --- 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))