diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index a9b6a28f..69e9e10c 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -153,7 +153,7 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request) } // Fetch the current resource - current, hasChanged, err := c.getCurrent(ctx, resource) + current, err := c.getCurrent(ctx, resource) if client.IgnoreNotFound(err) != nil && !isErrMissingNS(err) { return ctrl.Result{}, fmt.Errorf("getting current state: %w", err) } @@ -195,30 +195,19 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request) } } - // Nil current struct means the resource version hasn't changed since it was last observed - // Skip without logging since this is a very hot path - var modified bool - if hasChanged { - resource.ObserveVersion("") // in case reconciliation fails, invalidate the cache first to avoid skipping the next attempt - modified, err = c.reconcileResource(ctx, comp, prev, resource, current) - if err != nil { - return ctrl.Result{}, err - } + modified, err := c.reconcileResource(ctx, comp, prev, resource, current) + if err != nil { + return ctrl.Result{}, err } - - // We requeue to make sure the resource is in sync before updating our cache's resource version - // Otherwise the next sync would just hit the cache without actually diffing the resource. + // If we modified the resource, we should also re-evaluate readiness + // without waiting for the interval. if modified { return ctrl.Result{Requeue: true}, nil } - if current != nil { - if rv := current.GetResourceVersion(); rv != "" { - resource.ObserveVersion(rv) - } - } - // Store the results - deleted := current == nil || current.GetDeletionTimestamp() != nil + deleted := current == nil || + current.GetDeletionTimestamp() != nil || + (resource.Deleted() && comp.Annotations["eno.azure.io/deletion-strategy"] == "orphan") // orphaning should be reflected on the status. c.writeBuffer.PatchStatusAsync(ctx, &resource.ManifestRef, patchResourceState(deleted, ready)) if ready == nil { return ctrl.Result{RequeueAfter: wait.Jitter(c.readinessPollInterval, 0.1)}, nil @@ -354,23 +343,7 @@ func (c *Controller) buildPatch(ctx context.Context, prev, next *reconstitution. return patch, types.StrategicMergePatchType, err } -func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Resource) (*unstructured.Unstructured, bool, error) { - if resource.HasBeenSeen() && !resource.Deleted() { - meta := &metav1.PartialObjectMetadata{} - meta.Name = resource.Ref.Name - meta.Namespace = resource.Ref.Namespace - meta.Kind = resource.GVK.Kind - meta.APIVersion = resource.GVK.GroupVersion().String() - err := c.upstreamClient.Get(ctx, client.ObjectKeyFromObject(meta), meta) - if err != nil { - return nil, false, err - } - if resource.MatchesLastSeen(meta.ResourceVersion) { - return nil, false, nil - } - resourceVersionChanges.Inc() - } - +func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Resource) (*unstructured.Unstructured, error) { current := &unstructured.Unstructured{} current.SetName(resource.Ref.Name) current.SetNamespace(resource.Ref.Namespace) @@ -378,9 +351,9 @@ func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Re current.SetAPIVersion(resource.GVK.GroupVersion().String()) err := c.upstreamClient.Get(ctx, client.ObjectKeyFromObject(current), current) if err != nil { - return nil, true, err + return nil, err } - return current, true, nil + return current, nil } func mungePatch(patch []byte, rv string) ([]byte, error) { diff --git a/internal/controllers/reconciliation/crud_test.go b/internal/controllers/reconciliation/crud_test.go index 669c7acc..78592c34 100644 --- a/internal/controllers/reconciliation/crud_test.go +++ b/internal/controllers/reconciliation/crud_test.go @@ -21,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" apiv1 "github.com/Azure/eno/api/v1" + v1 "github.com/Azure/eno/api/v1" testv1 "github.com/Azure/eno/internal/controllers/reconciliation/fixtures/v1" "github.com/Azure/eno/internal/controllers/synthesis" "github.com/Azure/eno/internal/execution" @@ -755,6 +756,72 @@ func TestOrphanedCompositionDeletion(t *testing.T) { }) } +func TestOrphanedResources(t *testing.T) { + scheme := runtime.NewScheme() + corev1.SchemeBuilder.AddToScheme(scheme) + testv1.SchemeBuilder.AddToScheme(scheme) + + ctx := testutil.NewContext(t) + mgr := testutil.NewManager(t) + upstream := mgr.GetClient() + downstream := mgr.DownstreamClient + + registerControllers(t, mgr) + testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) { + output := &krmv1.ResourceList{} + output.Items = []*unstructured.Unstructured{{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "test-obj", + "namespace": "default", + "annotations": map[string]string{ + "eno.azure.io/reconcile-interval": "100ms", + }, + }, + "data": map[string]string{"foo": "bar"}, + }, + }} + return output, nil + }) + + setupTestSubject(t, mgr) + mgr.Start(t) + _, comp := writeComposition(t, upstream, true) + + // Wait for resource to be created. + obj := &corev1.ConfigMap{} + testutil.Eventually(t, func() bool { + obj.SetName("test-obj") + obj.SetNamespace("default") + err := downstream.Get(ctx, client.ObjectKeyFromObject(obj), obj) + return err == nil + }) + + // Delete the composition. + require.NoError(t, upstream.Delete(ctx, comp)) + t.Logf("deleted composition") + testutil.Eventually(t, func() bool { + return errors.IsNotFound(upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)) + }) + + // Ensure that the slice was deleted before checking the actual resource. + testutil.Eventually(t, func() bool { + rll := &v1.ResourceSliceList{} + err := upstream.List(ctx, rll, client.InNamespace(metav1.NamespaceAll)) + if err != nil { + return false + } + return len(rll.Items) == 0 + }) + + // The resource should be orphaned after the composition is gone. + err := downstream.Get(ctx, client.ObjectKeyFromObject(obj), obj) + require.NoError(t, err) + +} + // TestResourceDefaulting proves that resources which define properties equal to the field's default will eventually converge. func TestResourceDefaulting(t *testing.T) { scheme := runtime.NewScheme() diff --git a/internal/controllers/reconciliation/helpers_test.go b/internal/controllers/reconciliation/helpers_test.go index 14168527..75a43c70 100644 --- a/internal/controllers/reconciliation/helpers_test.go +++ b/internal/controllers/reconciliation/helpers_test.go @@ -35,6 +35,10 @@ func registerControllers(t *testing.T, mgr *testutil.Manager) { } func writeGenericComposition(t *testing.T, client client.Client) (*apiv1.Synthesizer, *apiv1.Composition) { + return writeComposition(t, client, false) +} + +func writeComposition(t *testing.T, client client.Client, orphan bool) (*apiv1.Synthesizer, *apiv1.Composition) { syn := &apiv1.Synthesizer{} syn.Name = "test-syn" syn.Spec.Image = "create" @@ -44,6 +48,9 @@ func writeGenericComposition(t *testing.T, client client.Client) (*apiv1.Synthes comp.Name = "test-comp" comp.Namespace = "default" comp.Spec.Synthesizer.Name = syn.Name + if orphan { + comp.Annotations = map[string]string{"eno.azure.io/deletion-strategy": "orphan"} + } require.NoError(t, client.Create(context.Background(), comp)) return syn, comp diff --git a/internal/resource/resource.go b/internal/resource/resource.go index fad5b850..11b96f45 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -43,7 +43,6 @@ type ManifestRef struct { // Resource is the controller's internal representation of a single resource out of a ResourceSlice. type Resource struct { - lastSeenMeta lastReconciledMeta Ref Ref @@ -257,29 +256,6 @@ type patchMeta struct { Ops jsonpatch.Patch `json:"ops"` } -type lastSeenMeta struct { - lock sync.Mutex - resourceVersion string -} - -func (l *lastSeenMeta) ObserveVersion(rv string) { - l.lock.Lock() - defer l.lock.Unlock() - l.resourceVersion = rv -} - -func (l *lastSeenMeta) HasBeenSeen() bool { - l.lock.Lock() - defer l.lock.Unlock() - return l.resourceVersion != "" -} - -func (l *lastSeenMeta) MatchesLastSeen(rv string) bool { - l.lock.Lock() - defer l.lock.Unlock() - return l.resourceVersion == rv -} - type lastReconciledMeta struct { lock sync.Mutex lastReconciled *time.Time