diff --git a/status/controller.go b/status/controller.go index 853a0e6..c30198e 100644 --- a/status/controller.go +++ b/status/controller.go @@ -181,7 +181,28 @@ func (c *GenericObjectController[T]) Register(_ context.Context, m manager.Manag } func (c *GenericObjectController[T]) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - return c.reconcile(ctx, req, NewUnstructuredAdapter[T](object.New[T]())) + // Use a typed Get so controller-runtime uses its cache instead of bypassing it. + // UnstructuredAdapter implements runtime.Unstructured (via embedded unstructured.Unstructured), + // which causes controller-runtime to skip the cache and hit the API server directly when used + // with kubeClient.Get. + typedObj := object.New[T]() + if err := c.kubeClient.Get(ctx, req.NamespacedName, typedObj); err != nil { + if errors.IsNotFound(err) { + return reconcile.Result{}, c.cleanupNotFound(req, NewUnstructuredAdapter[T](typedObj)) + } + return reconcile.Result{}, fmt.Errorf("getting object, %w", err) + } + // Convert to proper unstructured so conditions appear as []interface{} (JSON-compatible form) + // rather than the typed []metav1.Condition that ToPartialUnstructured would produce from a + // typed struct. UnstructuredAdapter.GetConditions relies on unstructured.NestedFieldNoCopy + // which requires []interface{}. + unstructuredContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(typedObj) + if err != nil { + return reconcile.Result{}, fmt.Errorf("converting object to unstructured, %w", err) + } + unstructuredObj := &unstructured.Unstructured{Object: unstructuredContent} + unstructuredObj.SetGroupVersionKind(object.GVK(typedObj)) + return c.reconcileFound(ctx, req, &UnstructuredAdapter[T]{Unstructured: *unstructuredObj}) } func (c *Controller[T]) toAdditionalMetricLabels(obj Object) map[string]string { @@ -217,33 +238,40 @@ func toPrometheusLabel(k string) string { func (c *Controller[T]) reconcile(ctx context.Context, req reconcile.Request, o Object) (reconcile.Result, error) { if err := c.kubeClient.Get(ctx, req.NamespacedName, o); err != nil { if errors.IsNotFound(err) { - c.observedConditions.Delete(req) - c.observedGaugeLabels.Delete(req) - c.deletePartialMatchGaugeMetric(c.ConditionCount, ConditionCount, map[string]string{ - MetricLabelNamespace: req.Namespace, - MetricLabelName: req.Name, - }) - c.deletePartialMatchGaugeMetric(c.ConditionCurrentStatusSeconds, ConditionCurrentStatusSeconds, map[string]string{ - MetricLabelNamespace: req.Namespace, - MetricLabelName: req.Name, - }) - c.deletePartialMatchGaugeMetric(c.TerminationCurrentTimeSeconds, TerminationCurrentTimeSeconds, map[string]string{ - MetricLabelNamespace: req.Namespace, - MetricLabelName: req.Name, - }) - if obj, ok := c.terminatingObjects.LoadAndDelete(req); ok { - c.observeHistogram(c.TerminationDuration, TerminationDuration, time.Since(obj.(Object).GetDeletionTimestamp().Time).Seconds(), map[string]string{}, c.toAdditionalMetricLabels(obj.(Object))) - } - if finalizers, ok := c.observedFinalizers.LoadAndDelete(req); ok { - for _, finalizer := range finalizers.([]string) { - c.eventRecorder.Event(o, v1.EventTypeNormal, "Finalized", fmt.Sprintf("Finalized %s", finalizer)) - } - } - return reconcile.Result{}, nil + return reconcile.Result{}, c.cleanupNotFound(req, o) } return reconcile.Result{}, fmt.Errorf("getting object, %w", err) } + return c.reconcileFound(ctx, req, o) +} + +func (c *Controller[T]) cleanupNotFound(req reconcile.Request, o Object) error { + c.observedConditions.Delete(req) + c.observedGaugeLabels.Delete(req) + c.deletePartialMatchGaugeMetric(c.ConditionCount, ConditionCount, map[string]string{ + MetricLabelNamespace: req.Namespace, + MetricLabelName: req.Name, + }) + c.deletePartialMatchGaugeMetric(c.ConditionCurrentStatusSeconds, ConditionCurrentStatusSeconds, map[string]string{ + MetricLabelNamespace: req.Namespace, + MetricLabelName: req.Name, + }) + c.deletePartialMatchGaugeMetric(c.TerminationCurrentTimeSeconds, TerminationCurrentTimeSeconds, map[string]string{ + MetricLabelNamespace: req.Namespace, + MetricLabelName: req.Name, + }) + if obj, ok := c.terminatingObjects.LoadAndDelete(req); ok { + c.observeHistogram(c.TerminationDuration, TerminationDuration, time.Since(obj.(Object).GetDeletionTimestamp().Time).Seconds(), map[string]string{}, c.toAdditionalMetricLabels(obj.(Object))) + } + if finalizers, ok := c.observedFinalizers.LoadAndDelete(req); ok { + for _, finalizer := range finalizers.([]string) { + c.eventRecorder.Event(o, v1.EventTypeNormal, "Finalized", fmt.Sprintf("Finalized %s", finalizer)) + } + } + return nil +} +func (c *Controller[T]) reconcileFound(ctx context.Context, req reconcile.Request, o Object) (reconcile.Result, error) { // Detect and record terminations observedFinalizers, _ := c.observedFinalizers.Swap(req, o.GetFinalizers()) if observedFinalizers != nil { diff --git a/status/controller_test.go b/status/controller_test.go index b7063dd..ea49528 100644 --- a/status/controller_test.go +++ b/status/controller_test.go @@ -16,6 +16,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/samber/lo" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" @@ -30,6 +31,29 @@ var recorder *record.FakeRecorder var kubeClient client.Client var registry = metrics.Registry +// spyClient wraps a client.Client and counts how many Get calls are made with +// a runtime.Unstructured object, which would bypass the controller-runtime cache. +type spyClient struct { + client.Client + mu sync.Mutex + unstructuredGets int +} + +func (spy *spyClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(runtime.Unstructured); ok { + spy.mu.Lock() + spy.unstructuredGets++ + spy.mu.Unlock() + } + return spy.Client.Get(ctx, key, obj, opts...) +} + +func (spy *spyClient) reset() { + spy.mu.Lock() + defer spy.mu.Unlock() + spy.unstructuredGets = 0 +} + var _ = AfterEach(func() { status.ConditionDuration.Reset() status.ConditionCount.Reset() @@ -806,11 +830,13 @@ var _ = Describe("Controller", func() { var _ = Describe("Generic Controller", func() { var genericController *status.GenericObjectController[*TestGenericObject] + var spy *spyClient BeforeEach(func() { recorder = record.NewFakeRecorder(10) - kubeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + spy = &spyClient{Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()} + kubeClient = spy ctx = log.IntoContext(context.Background(), GinkgoLogr) - genericController = status.NewGenericObjectController[*TestGenericObject](kubeClient, recorder, status.EmitDeprecatedMetrics) + genericController = status.NewGenericObjectController[*TestGenericObject](spy, recorder, status.EmitDeprecatedMetrics) }) AfterEach(func() { metrics.Registry = registry // reset the registry to handle cases where the registry is overridden @@ -1292,6 +1318,23 @@ var _ = Describe("Generic Controller", func() { }() } }) + It("should use typed Get to avoid bypassing the controller-runtime cache", func() { + // GenericObjectController must call kubeClient.Get with a typed T object, not an + // UnstructuredAdapter. UnstructuredAdapter implements runtime.Unstructured (via its + // embedded unstructured.Unstructured), which causes controller-runtime's cached client + // to bypass the cache and send direct HTTP GETs to the API server on every requeue. + testObject := test.Object(&TestGenericObject{ + Status: TestGenericStatus{ + Conditions: []metav1.Condition{ + {Type: ConditionTypeFoo, Status: metav1.ConditionTrue, Reason: ConditionTypeFoo}, + }, + }, + }) + ExpectApplied(ctx, kubeClient, testObject) + spy.reset() + ExpectReconciled(ctx, genericController, testObject) + Expect(spy.unstructuredGets).To(Equal(0)) + }) }) func conditionLabelsWithGroupKind(gvk schema.GroupVersionKind, t status.ConditionType, s metav1.ConditionStatus) map[string]string {