From 5aa0ed2bd37bd51db62e524d9a9558bbdc2e23db Mon Sep 17 00:00:00 2001 From: Steffen Baarsgaard Date: Mon, 3 Nov 2025 21:34:42 +0100 Subject: [PATCH] feat(ContactPoint): Track previously matched instances for cleanup --- controllers/alertrulegroup_controller.go | 4 +- controllers/contactpoint_controller.go | 88 ++++++++++++++----- controllers/contactpoint_controller_test.go | 2 +- controllers/controller_shared.go | 50 +++++++++-- controllers/controller_shared_test.go | 6 +- controllers/dashboard_controller.go | 4 +- controllers/datasource_controller.go | 6 +- controllers/folder_controller.go | 4 +- controllers/librarypanel_controller.go | 4 +- controllers/mutetiming_controller.go | 4 +- controllers/notificationpolicy_controller.go | 4 +- .../notificationtemplate_controller.go | 4 +- 12 files changed, 134 insertions(+), 46 deletions(-) diff --git a/controllers/alertrulegroup_controller.go b/controllers/alertrulegroup_controller.go index ad0b838c1..fc1f82436 100644 --- a/controllers/alertrulegroup_controller.go +++ b/controllers/alertrulegroup_controller.go @@ -91,7 +91,7 @@ func (r *GrafanaAlertRuleGroupReconciler) Reconcile(ctx context.Context, req ctr removeSuspended(&group.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, group) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, group) if err != nil { setNoMatchingInstancesCondition(&group.Status.Conditions, group.Generation, err) meta.RemoveStatusCondition(&group.Status.Conditions, conditionAlertGroupSynchronized) @@ -299,7 +299,7 @@ func (r *GrafanaAlertRuleGroupReconciler) finalize(ctx context.Context, group *g isCleanupInGrafanaRequired = false } - instances, err := GetScopedMatchingInstances(ctx, r.Client, group) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, group) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/contactpoint_controller.go b/controllers/contactpoint_controller.go index 504cc4a21..fd6d4db6e 100644 --- a/controllers/contactpoint_controller.go +++ b/controllers/contactpoint_controller.go @@ -99,7 +99,7 @@ func (r *GrafanaContactPointReconciler) Reconcile(ctx context.Context, req ctrl. removeSuspended(&cr.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, instDelta, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { setNoMatchingInstancesCondition(&cr.Status.Conditions, cr.Generation, err) meta.RemoveStatusCondition(&cr.Status.Conditions, conditionContactPointSynchronized) @@ -154,11 +154,50 @@ func (r *GrafanaContactPointReconciler) Reconcile(ctx context.Context, req ctrl. } } - condition := buildSynchronizedCondition("Contact point", conditionContactPointSynchronized, cr.Generation, applyErrors, len(instances)) + cleanupErrors := make(map[string]string) + dirtyInstances := make([]grafanav1beta1.Grafana, 0, len(instDelta)) + + for _, namespacedName := range instDelta { + g := grafanav1beta1.Grafana{} + + err := r.Get(ctx, namespacedName, &g) + if err != nil { + if kuberr.IsNotFound(err) { + continue + } + + dirtyInstances = append(dirtyInstances, g) + cleanupErrors[namespacedName.String()] = err.Error() + } + + err = r.deleteFromInstance(ctx, &g, cr) + if err != nil { + dirtyInstances = append(dirtyInstances, g) + cleanupErrors[namespacedName.String()] = err.Error() + + continue + } + } + + av := make([]string, 0, len(instances)+len(dirtyInstances)) + for _, g := range append(instances, dirtyInstances...) { + av = append(av, fmt.Sprintf("%s/%s", g.Namespace, g.Name)) + } + + slices.Sort(av) + v := strings.Join(av, ",") + + err = addAnnotation(ctx, r.Client, cr, annotationMatchedInstances, v) + if err != nil { + log.Error(err, "annotating contact point with matched instances", "annotation", annotationMatchedInstances) + } + + allErrors := mergeReconcileErrors(applyErrors, cleanupErrors) + condition := buildSynchronizedCondition("Contact point", conditionContactPointSynchronized, cr.Generation, allErrors, len(instances)) meta.SetStatusCondition(&cr.Status.Conditions, condition) - if len(applyErrors) > 0 { - return ctrl.Result{}, fmt.Errorf("failed to apply to all instances: %v", applyErrors) + if len(allErrors) > 0 { + return ctrl.Result{}, fmt.Errorf("syncing all instances: %v", allErrors) } return ctrl.Result{RequeueAfter: r.Cfg.requeueAfter(cr.Spec.ResyncPeriod)}, nil @@ -321,36 +360,45 @@ func (r *GrafanaContactPointReconciler) finalize(ctx context.Context, cr *grafan log := logf.FromContext(ctx) log.Info("Finalizing GrafanaContactPoint") - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { return fmt.Errorf("fetching instances: %w", err) } for _, instance := range instances { - cl, err := client2.NewGeneratedGrafanaClient(ctx, r.Client, &instance) - if err != nil { - return fmt.Errorf("building grafana client: %w", err) - } - - remoteReceivers, err := r.getReceiversFromName(cl, cr) + err := r.deleteFromInstance(ctx, &instance, cr) if err != nil { return err } + } - for _, rec := range remoteReceivers { - _, err = cl.Provisioning.DeleteContactpoints(rec.UID) //nolint:errcheck - if err != nil { - return fmt.Errorf("deleting contact point: %w", err) - } - } + return nil +} - // Update grafana instance Status - err = instance.RemoveNamespacedResource(ctx, r.Client, cr) +func (r *GrafanaContactPointReconciler) deleteFromInstance(ctx context.Context, g *grafanav1beta1.Grafana, cr *grafanav1beta1.GrafanaContactPoint) error { + cl, err := client2.NewGeneratedGrafanaClient(ctx, r.Client, g) + if err != nil { + return fmt.Errorf("building grafana client: %w", err) + } + + remoteReceivers, err := r.getReceiversFromName(cl, cr) + if err != nil { + return err + } + + for _, rec := range remoteReceivers { + _, err = cl.Provisioning.DeleteContactpoints(rec.UID) //nolint:errcheck if err != nil { - return fmt.Errorf("removing contact point from Grafana cr: %w", err) + return fmt.Errorf("deleting contact point: %w", err) } } + // Update grafana instance Status + err = g.RemoveNamespacedResource(ctx, r.Client, cr) + if err != nil { + return fmt.Errorf("removing contact point from Grafana cr: %w", err) + } + return nil } diff --git a/controllers/contactpoint_controller_test.go b/controllers/contactpoint_controller_test.go index c0f20373a..bb1f3c4c1 100644 --- a/controllers/contactpoint_controller_test.go +++ b/controllers/contactpoint_controller_test.go @@ -59,7 +59,7 @@ var _ = Describe("ContactPoint Reconciler: Provoke Conditions", func() { Type: conditionContactPointSynchronized, Reason: conditionReasonApplyFailed, }, - wantErr: "failed to apply to all instances", + wantErr: "syncing all instances:", }, { name: "Referenced secret does not exist", diff --git a/controllers/controller_shared.go b/controllers/controller_shared.go index 728fb053d..5fe292ef4 100644 --- a/controllers/controller_shared.go +++ b/controllers/controller_shared.go @@ -53,6 +53,9 @@ const ( // Finalizer grafanaFinalizer = "operator.grafana.com/finalizer" + + // Annotations + annotationMatchedInstances = "operator.grafana.com/matched-instances" ) var ( @@ -92,13 +95,13 @@ func defaultRateLimiter() workqueue.TypedRateLimiter[reconcile.Request] { // Only matching instances in the scope of the resource are returned // Resources with allowCrossNamespaceImport expands the scope to the entire cluster // Intended to be used in reconciler functions -func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr v1beta1.CommonResource) ([]v1beta1.Grafana, error) { +func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr v1beta1.CommonResource) ([]v1beta1.Grafana, []types.NamespacedName, error) { log := logf.FromContext(ctx) instanceSelector := cr.MatchLabels() // Should never happen, sanity check if instanceSelector == nil { - return []v1beta1.Grafana{}, nil + return []v1beta1.Grafana{}, []types.NamespacedName{}, nil } opts := []client.ListOption{ @@ -115,11 +118,33 @@ func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr err := k8sClient.List(ctx, &list, opts...) if err != nil { - return []v1beta1.Grafana{}, err + return []v1beta1.Grafana{}, []types.NamespacedName{}, err + } + + matchedInstancesDelta := make([]types.NamespacedName, 0, len(list.Items)) + + // Find all previously matched instances + rawInst := cr.Metadata().Annotations[annotationMatchedInstances] + if rawInst != "" { + for inst := range strings.SplitSeq(rawInst, ",") { + meta := strings.Split(inst, "/") + + // Remove duplicates + found := slices.ContainsFunc(matchedInstancesDelta, func(n types.NamespacedName) bool { + return n.Namespace == meta[0] && n.Name == meta[1] + }) + if !found { + matchedInstancesDelta = append(matchedInstancesDelta, types.NamespacedName{ + Namespace: meta[0], + Name: meta[1], + }) + } + } } + // NOTE Triggers cleanup against all previously matched instances if any if len(list.Items) == 0 { - return []v1beta1.Grafana{}, nil + return []v1beta1.Grafana{}, matchedInstancesDelta, nil } selectedList := make([]v1beta1.Grafana, 0, len(list.Items)) @@ -133,6 +158,10 @@ func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr continue } + // Remove instances that were matched again + // This way a delta is built for later cleanup + matchedInstancesDelta = removeNamespacedNameEntry(matchedInstancesDelta, instance.Namespace, instance.Name) + // admin url is required to interact with Grafana // the instance or route might not yet be ready if instance.Status.Stage != v1beta1.OperatorStageComplete || instance.Status.StageStatus != v1beta1.OperatorStageResultSuccess { @@ -147,11 +176,22 @@ func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr log.Info("Grafana instances not ready, excluded from matching", "instances", unreadyInstances) } + // TODO Decide whether to log recently excluded instances here or in each controller + // if len(matchedInstancesDelta) > 0 { + // log.Info("Grafana instances excluded by instanceSelector, to be removed from instances", "instances", matchedInstancesDelta) + // } + if len(selectedList) == 0 { log.Info("None of the available Grafana instances matched the selector, skipping reconciliation", "AllowCrossNamespaceImport", cr.AllowCrossNamespace()) } - return selectedList, nil + return selectedList, matchedInstancesDelta, nil +} + +func removeNamespacedNameEntry(s []types.NamespacedName, namespace, name string) []types.NamespacedName { + return slices.DeleteFunc(s, func(n types.NamespacedName) bool { + return n.Namespace == namespace && n.Name == name + }) } // getFolderUID returns the folderUID from an existing GrafanaFolder CR within the same namespace diff --git a/controllers/controller_shared_test.go b/controllers/controller_shared_test.go index ddce1e96a..12902ea25 100644 --- a/controllers/controller_shared_test.go +++ b/controllers/controller_shared_test.go @@ -533,18 +533,18 @@ var _ = Describe("GetMatchingInstances functions", Ordered, func() { Context("Ensure AllowCrossNamespaceImport is upheld by GetScopedMatchingInstances", func() { It("Finds all ready instances when instanceSelector is empty", func() { - instances, err := GetScopedMatchingInstances(testCtx, k8sClient, matchAllFolder) + instances, _, err := GetScopedMatchingInstances(testCtx, k8sClient, matchAllFolder) Expect(err).ToNot(HaveOccurred()) Expect(instances).To(HaveLen(2 + 2)) // +2 To account for instances created in controllers/suite_test.go to provoke conditions }) It("Finds all ready and Matching instances", func() { - instances, err := GetScopedMatchingInstances(testCtx, k8sClient, &allowFolder) + instances, _, err := GetScopedMatchingInstances(testCtx, k8sClient, &allowFolder) Expect(err).ToNot(HaveOccurred()) Expect(instances).ToNot(BeEmpty()) Expect(instances).To(HaveLen(2)) }) It("Finds matching and ready and matching instance in namespace", func() { - instances, err := GetScopedMatchingInstances(testCtx, k8sClient, denyFolder) + instances, _, err := GetScopedMatchingInstances(testCtx, k8sClient, denyFolder) Expect(err).ToNot(HaveOccurred()) Expect(instances).ToNot(BeEmpty()) Expect(instances).To(HaveLen(1)) diff --git a/controllers/dashboard_controller.go b/controllers/dashboard_controller.go index 029846f59..d3291f715 100644 --- a/controllers/dashboard_controller.go +++ b/controllers/dashboard_controller.go @@ -115,7 +115,7 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req removeInvalidSpec(&cr.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { setNoMatchingInstancesCondition(&cr.Status.Conditions, cr.Generation, err) meta.RemoveStatusCondition(&cr.Status.Conditions, conditionDashboardSynchronized) @@ -221,7 +221,7 @@ func (r *GrafanaDashboardReconciler) finalize(ctx context.Context, cr *v1beta1.G uid := content.CustomUIDOrUID(cr, cr.Status.UID) - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/datasource_controller.go b/controllers/datasource_controller.go index dc6e83275..ab825abca 100644 --- a/controllers/datasource_controller.go +++ b/controllers/datasource_controller.go @@ -97,7 +97,7 @@ func (r *GrafanaDatasourceReconciler) Reconcile(ctx context.Context, req ctrl.Re removeSuspended(&cr.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { setNoMatchingInstancesCondition(&cr.Status.Conditions, cr.Generation, err) meta.RemoveStatusCondition(&cr.Status.Conditions, conditionDatasourceSynchronized) @@ -190,7 +190,7 @@ func (r *GrafanaDatasourceReconciler) Reconcile(ctx context.Context, req ctrl.Re } func (r *GrafanaDatasourceReconciler) deleteOldDatasource(ctx context.Context, cr *v1beta1.GrafanaDatasource) error { - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { return fmt.Errorf("fetching instances: %w", err) } @@ -223,7 +223,7 @@ func (r *GrafanaDatasourceReconciler) finalize(ctx context.Context, cr *v1beta1. log := logf.FromContext(ctx) log.Info("Finalizing GrafanaDatasource") - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/folder_controller.go b/controllers/folder_controller.go index 2b3091d3b..d552ed0bf 100644 --- a/controllers/folder_controller.go +++ b/controllers/folder_controller.go @@ -100,7 +100,7 @@ func (r *GrafanaFolderReconciler) Reconcile(ctx context.Context, req ctrl.Reques removeInvalidSpec(&folder.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, folder) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, folder) if err != nil { setNoMatchingInstancesCondition(&folder.Status.Conditions, folder.Generation, err) meta.RemoveStatusCondition(&folder.Status.Conditions, conditionFolderSynchronized) @@ -154,7 +154,7 @@ func (r *GrafanaFolderReconciler) finalize(ctx context.Context, folder *grafanav uid := folder.CustomUIDOrUID() - instances, err := GetScopedMatchingInstances(ctx, r.Client, folder) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, folder) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/librarypanel_controller.go b/controllers/librarypanel_controller.go index 0e7159ea1..385481f34 100644 --- a/controllers/librarypanel_controller.go +++ b/controllers/librarypanel_controller.go @@ -130,7 +130,7 @@ func (r *GrafanaLibraryPanelReconciler) Reconcile(ctx context.Context, req ctrl. // begin instance selection and reconciliation - instances, err := GetScopedMatchingInstances(ctx, r.Client, libraryPanel) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, libraryPanel) if err != nil { setNoMatchingInstancesCondition(&libraryPanel.Status.Conditions, libraryPanel.Generation, err) meta.RemoveStatusCondition(&libraryPanel.Status.Conditions, conditionLibraryPanelSynchronized) @@ -238,7 +238,7 @@ func (r *GrafanaLibraryPanelReconciler) finalize(ctx context.Context, cr *v1beta uid := content.CustomUIDOrUID(cr, cr.Status.UID) - instances, err := GetScopedMatchingInstances(ctx, r.Client, cr) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, cr) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/mutetiming_controller.go b/controllers/mutetiming_controller.go index 935ccefaf..03659bbfa 100644 --- a/controllers/mutetiming_controller.go +++ b/controllers/mutetiming_controller.go @@ -85,7 +85,7 @@ func (r *GrafanaMuteTimingReconciler) Reconcile(ctx context.Context, req ctrl.Re removeSuspended(&muteTiming.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, muteTiming) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, muteTiming) if err != nil { setNoMatchingInstancesCondition(&muteTiming.Status.Conditions, muteTiming.Generation, err) meta.RemoveStatusCondition(&muteTiming.Status.Conditions, conditionMuteTimingSynchronized) @@ -207,7 +207,7 @@ func (r *GrafanaMuteTimingReconciler) finalize(ctx context.Context, muteTiming * log := logf.FromContext(ctx) log.Info("Finalizing GrafanaMuteTiming") - instances, err := GetScopedMatchingInstances(ctx, r.Client, muteTiming) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, muteTiming) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go index 80e255066..1410d7d1d 100644 --- a/controllers/notificationpolicy_controller.go +++ b/controllers/notificationpolicy_controller.go @@ -134,7 +134,7 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req meta.RemoveStatusCondition(¬ificationPolicy.Status.Conditions, conditionNotificationPolicyLoopDetected) - instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy) if err != nil { setNoMatchingInstancesCondition(¬ificationPolicy.Status.Conditions, notificationPolicy.Generation, err) meta.RemoveStatusCondition(¬ificationPolicy.Status.Conditions, conditionNotificationPolicySynchronized) @@ -304,7 +304,7 @@ func (r *GrafanaNotificationPolicyReconciler) finalize(ctx context.Context, noti log := logf.FromContext(ctx) log.Info("Finalizing GrafanaNotificationPolicy") - instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy) if err != nil { return fmt.Errorf("fetching instances: %w", err) } diff --git a/controllers/notificationtemplate_controller.go b/controllers/notificationtemplate_controller.go index c57b7b900..3dbded56b 100644 --- a/controllers/notificationtemplate_controller.go +++ b/controllers/notificationtemplate_controller.go @@ -84,7 +84,7 @@ func (r *GrafanaNotificationTemplateReconciler) Reconcile(ctx context.Context, r removeSuspended(¬ificationTemplate.Status.Conditions) - instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationTemplate) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, notificationTemplate) if err != nil { setNoMatchingInstancesCondition(¬ificationTemplate.Status.Conditions, notificationTemplate.Generation, err) meta.RemoveStatusCondition(¬ificationTemplate.Status.Conditions, conditionNotificationTemplateSynchronized) @@ -156,7 +156,7 @@ func (r *GrafanaNotificationTemplateReconciler) finalize(ctx context.Context, no log := logf.FromContext(ctx) log.Info("Finalizing GrafanaNotificationTemplate") - instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationTemplate) + instances, _, err := GetScopedMatchingInstances(ctx, r.Client, notificationTemplate) if err != nil { return fmt.Errorf("fetching instances: %w", err) }