Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Reconcile loggers, prepare for concurrent reconcilers #1836

Merged
merged 12 commits into from
Jan 29, 2025
Merged

Refactor: Reconcile loggers, prepare for concurrent reconcilers #1836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7b09545
Update controller_shared arg ordering
Baarsgaard Jan 25, 2025
796b817
refactor: Update NotificationPolicyTemplate logging
Baarsgaard Jan 23, 2025
4b03742
refactor: Update ContactPoint logging
Baarsgaard Jan 23, 2025
553daa1
refactor: Update AlertRuleGroup logging
Baarsgaard Jan 23, 2025
d80f963
refactor: Update Folder logging
Baarsgaard Jan 23, 2025
0cda3d5
refactor: Update notificationPolicy logging
Baarsgaard Jan 23, 2025
4dbf898
refactor: Update Dashboard logging
Baarsgaard Jan 23, 2025
986fdaf
refactor: Update Datasource logging
Baarsgaard Jan 25, 2025
2f4dce0
refactor: Update Grafana logging
Baarsgaard Jan 23, 2025
7c87658
refactor: Internal Grafana resource reconciler logging
Baarsgaard Jan 23, 2025
519c7fd
refactor: Update MuteTiming logging
Baarsgaard Jan 27, 2025
58fe2b8
fix: Overwrite ctx after logf.IntoContext
Baarsgaard Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions controllers/controller_shared.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"strings"
"time"

"github.com/go-logr/logr"
operatorapi "github.com/grafana/grafana-operator/v5/api"
"github.com/grafana/grafana-operator/v5/api/v1beta1"
grafanav1beta1 "github.com/grafana/grafana-operator/v5/api/v1beta1"
Expand All @@ -23,6 +22,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
)

Expand Down Expand Up @@ -75,7 +75,8 @@ func GetMatchingInstances(ctx context.Context, k8sClient client.Client, labelSel
// 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(log logr.Logger, 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, error) {
log := logf.FromContext(ctx)
instanceSelector := cr.MatchLabels()

// Should never happen, sanity check
Expand Down
12 changes: 7 additions & 5 deletions controllers/controller_shared_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/log"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

func TestLabelsSatisfyMatchExpressions(t *testing.T) {
Expand Down Expand Up @@ -296,6 +296,9 @@ var _ = Describe("GetMatchingInstances functions", Ordered, func() {
unreadyGrafana.Name = "unready-instance"

ctx := context.Background()
testLog := logf.FromContext(ctx).WithSink(logf.NullLogSink{})
ctx = logf.IntoContext(ctx, testLog)

// Pre-create all resources
BeforeAll(func() { // Necessary to use assertions
Expect(k8sClient.Create(ctx, &namespace)).NotTo(HaveOccurred())
Expand All @@ -316,20 +319,19 @@ var _ = Describe("GetMatchingInstances functions", Ordered, func() {
})

Context("Ensure AllowCrossNamespaceImport is upheld by GetScopedMatchingInstances", func() {
testLog := log.FromContext(ctx).WithSink(log.NullLogSink{})
It("Finds all ready instances when instanceSelector is empty", func() {
instances, err := GetScopedMatchingInstances(testLog, ctx, k8sClient, matchAllFolder)
instances, err := GetScopedMatchingInstances(ctx, k8sClient, matchAllFolder)
Expect(err).ToNot(HaveOccurred())
Expect(instances).To(HaveLen(3))
})
It("Finds all ready and Matching instances", func() {
instances, err := GetScopedMatchingInstances(testLog, ctx, k8sClient, &allowFolder)
instances, err := GetScopedMatchingInstances(ctx, 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(testLog, ctx, k8sClient, denyFolder)
instances, err := GetScopedMatchingInstances(ctx, k8sClient, denyFolder)
Expect(err).ToNot(HaveOccurred())
Expect(instances).ToNot(BeEmpty())
Expect(instances).To(HaveLen(1))
Expand Down
57 changes: 30 additions & 27 deletions controllers/dashboard_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (

"github.com/grafana/grafana-operator/v5/embeds"

"github.com/go-logr/logr"
genapi "github.com/grafana/grafana-openapi-client-go/client"
"github.com/grafana/grafana-openapi-client-go/client/dashboards"
"github.com/grafana/grafana-openapi-client-go/client/folders"
Expand All @@ -51,7 +50,7 @@ import (
"k8s.io/client-go/discovery"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"
logf "sigs.k8s.io/controller-runtime/pkg/log"

v1 "k8s.io/api/core/v1"
)
Expand All @@ -63,7 +62,6 @@ const (
// GrafanaDashboardReconciler reconciles a GrafanaDashboard object
type GrafanaDashboardReconciler struct {
Client client.Client
Log logr.Logger
Scheme *runtime.Scheme
Discovery discovery.DiscoveryInterface
}
Expand All @@ -73,7 +71,7 @@ type GrafanaDashboardReconciler struct {
//+kubebuilder:rbac:groups=grafana.integreatly.org,resources=grafanadashboards/finalizers,verbs=update

func (r *GrafanaDashboardReconciler) syncDashboards(ctx context.Context) (ctrl.Result, error) {
syncLog := log.FromContext(ctx).WithName("GrafanaDashboardReconciler")
log := logf.FromContext(ctx)
dashboardsSynced := 0

// get all grafana instances
Expand Down Expand Up @@ -122,7 +120,7 @@ func (r *GrafanaDashboardReconciler) syncDashboards(ctx context.Context) (ctrl.R
if err != nil {
var notFound *dashboards.DeleteDashboardByUIDNotFound
if errors.As(err, &notFound) {
syncLog.Info("dashboard no longer exists", "namespace", namespace, "name", name)
log.Info("dashboard no longer exists", "namespace", namespace, "name", name)
} else {
return ctrl.Result{Requeue: false}, err
}
Expand All @@ -141,7 +139,7 @@ func (r *GrafanaDashboardReconciler) syncDashboards(ctx context.Context) (ctrl.R
}

if dashboardsSynced > 0 {
syncLog.Info("successfully synced dashboards", "dashboards", dashboardsSynced)
log.Info("successfully synced dashboards", "dashboards", dashboardsSynced)
}
return ctrl.Result{Requeue: false}, nil
}
Expand All @@ -161,8 +159,8 @@ func getDashboardsToDelete(allDashboards *v1beta1.GrafanaDashboardList, grafanas
}

func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
controllerLog := log.FromContext(ctx).WithName("GrafanaDashboardReconciler")
r.Log = controllerLog
log := logf.FromContext(ctx).WithName("GrafanaDashboardReconciler")
ctx = logf.IntoContext(ctx, log)

// periodic sync reconcile
if req.Namespace == "" && req.Name == "" {
Expand All @@ -186,37 +184,37 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req
}
return ctrl.Result{}, nil
}
controllerLog.Error(err, "error getting grafana dashboard cr")
log.Error(err, "error getting grafana dashboard cr")
return ctrl.Result{RequeueAfter: RequeueDelay}, err
}

instances, err := r.GetMatchingDashboardInstances(ctx, cr, r.Client)
if err != nil {
controllerLog.Error(err, "could not find matching instances", "name", cr.Name, "namespace", cr.Namespace)
log.Error(err, "could not find matching instances", "name", cr.Name, "namespace", cr.Namespace)
return ctrl.Result{RequeueAfter: RequeueDelay}, err
}

removeNoMatchingInstance(&cr.Status.Conditions)
controllerLog.Info("found matching Grafana instances for dashboard", "count", len(instances.Items))
log.Info("found matching Grafana instances for dashboard", "count", len(instances.Items))

dashboardJson, err := r.fetchDashboardJson(ctx, cr)
if err != nil {
controllerLog.Error(err, "error fetching dashboard", "dashboard", cr.Name)
log.Error(err, "error fetching dashboard", "dashboard", cr.Name)
return ctrl.Result{RequeueAfter: RequeueDelay}, nil
}

// Retrieving the model before the loop ensures to exit early in case of failure and not fail once per matching instance
dashboardModel, hash, err := r.getDashboardModel(cr, dashboardJson)
if err != nil {
controllerLog.Error(err, "failed to prepare dashboard model", "dashboard", cr.Name)
log.Error(err, "failed to prepare dashboard model", "dashboard", cr.Name)
return ctrl.Result{Requeue: false}, nil
}

uid := fmt.Sprintf("%s", dashboardModel["uid"])

// Garbage collection for a case where dashboard uid get changed, dashboard creation is expected to happen in a separate reconcilication cycle
if cr.IsUpdatedUID(uid) {
controllerLog.Info("dashboard uid got updated, deleting dashboards with the old uid")
log.Info("dashboard uid got updated, deleting dashboards with the old uid")
err = r.onDashboardDeleted(ctx, req.Namespace, req.Name)
if err != nil {
return ctrl.Result{RequeueAfter: RequeueDelay}, err
Expand Down Expand Up @@ -245,7 +243,7 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req
// an admin url is required to interact with grafana
// the instance or route might not yet be ready
if grafana.Status.Stage != v1beta1.OperatorStageComplete || grafana.Status.StageStatus != v1beta1.OperatorStageResultSuccess {
controllerLog.Info("grafana instance not ready", "grafana", grafana.Name)
log.Info("grafana instance not ready", "grafana", grafana.Name)
success = false
continue
}
Expand All @@ -256,15 +254,15 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req
// grafana reconciler will pick them up
err = ReconcilePlugins(ctx, r.Client, r.Scheme, &grafana, cr.Spec.Plugins, fmt.Sprintf("%v-dashboard", cr.Name))
if err != nil {
controllerLog.Error(err, "error reconciling plugins", "dashboard", cr.Name, "grafana", grafana.Name)
log.Error(err, "error reconciling plugins", "dashboard", cr.Name, "grafana", grafana.Name)
success = false
}
}

// then import the dashboard into the matching grafana instances
err = r.onDashboardCreated(ctx, &grafana, cr, dashboardModel, hash)
if err != nil {
controllerLog.Error(err, "error reconciling dashboard", "dashboard", cr.Name, "grafana", grafana.Name)
log.Error(err, "error reconciling dashboard", "dashboard", cr.Name, "grafana", grafana.Name)
applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error()
success = false
}
Expand Down Expand Up @@ -294,6 +292,7 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req
}

func (r *GrafanaDashboardReconciler) onDashboardDeleted(ctx context.Context, namespace string, name string) error {
log := logf.FromContext(ctx)
list := v1beta1.GrafanaList{}
var opts []client.ListOption
err := r.Client.List(ctx, &list, opts...)
Expand Down Expand Up @@ -341,10 +340,10 @@ func (r *GrafanaDashboardReconciler) onDashboardDeleted(ctx context.Context, nam
return err
}
if resp.StatusCode == 200 {
r.Log.Info("unused folder successfully removed")
log.Info("unused folder successfully removed")
}
if resp.StatusCode == 432 {
r.Log.Info("folder still in use by other dashboards")
log.Info("folder still in use by other dashboards")
}
}
}
Expand All @@ -368,6 +367,7 @@ func (r *GrafanaDashboardReconciler) onDashboardDeleted(ctx context.Context, nam
}

func (r *GrafanaDashboardReconciler) onDashboardCreated(ctx context.Context, grafana *v1beta1.Grafana, cr *v1beta1.GrafanaDashboard, dashboardModel map[string]interface{}, hash string) error {
log := logf.FromContext(ctx)
if grafana.IsExternal() && cr.Spec.Plugins != nil {
return fmt.Errorf("external grafana instances don't support plugins, please remove spec.plugins from your dashboard cr")
}
Expand Down Expand Up @@ -399,7 +399,7 @@ func (r *GrafanaDashboardReconciler) onDashboardCreated(ctx context.Context, gra

if exists && remoteUID != uid {
// If there's already a dashboard with the same title in the same folder, grafana preserves that dashboard's uid, so we should remove it first
r.Log.Info("found dashboard with the same title (in the same folder) but different uid, removing the dashboard before recreating it with a new uid")
log.Info("found dashboard with the same title (in the same folder) but different uid, removing the dashboard before recreating it with a new uid")
_, err = grafanaClient.Dashboards.DeleteDashboardByUID(remoteUID) //nolint:errcheck
if err != nil {
var notFound *dashboards.DeleteDashboardByUIDNotFound
Expand Down Expand Up @@ -755,21 +755,22 @@ func (r *GrafanaDashboardReconciler) SetupWithManager(mgr ctrl.Manager, ctx cont

if err == nil {
go func() {
log := logf.FromContext(ctx).WithName("GrafanaDashboardReconciler")
for {
select {
case <-ctx.Done():
return
case <-time.After(initialSyncDelay):
result, err := r.Reconcile(ctx, ctrl.Request{})
if err != nil {
r.Log.Error(err, "error synchronizing dashboards")
log.Error(err, "error synchronizing dashboards")
continue
}
if result.Requeue {
r.Log.Info("more dashboards left to synchronize")
log.Info("more dashboards left to synchronize")
continue
}
r.Log.Info("dashboard sync complete")
log.Info("dashboard sync complete")
return
}
}
Expand All @@ -780,23 +781,25 @@ func (r *GrafanaDashboardReconciler) SetupWithManager(mgr ctrl.Manager, ctx cont
}

func (r *GrafanaDashboardReconciler) GetMatchingDashboardInstances(ctx context.Context, dashboard *v1beta1.GrafanaDashboard, k8sClient client.Client) (v1beta1.GrafanaList, error) {
log := logf.FromContext(ctx)
instances, err := GetMatchingInstances(ctx, k8sClient, dashboard.Spec.InstanceSelector)
if err != nil || len(instances.Items) == 0 {
dashboard.Status.NoMatchingInstances = true
if err := r.Client.Status().Update(ctx, dashboard); err != nil {
r.Log.Info("unable to update the status of %v, in %v", dashboard.Name, dashboard.Namespace)
log.Info("unable to update the status of %v, in %v", dashboard.Name, dashboard.Namespace)
}
return v1beta1.GrafanaList{}, err
}
dashboard.Status.NoMatchingInstances = false
if err := r.Client.Status().Update(ctx, dashboard); err != nil {
r.Log.Info("unable to update the status of %v, in %v", dashboard.Name, dashboard.Namespace)
log.Info("unable to update the status of %v, in %v", dashboard.Name, dashboard.Namespace)
}

return instances, err
}

func (r *GrafanaDashboardReconciler) UpdateHomeDashboard(ctx context.Context, grafana v1beta1.Grafana, uid string, dashboard *v1beta1.GrafanaDashboard) error {
log := logf.FromContext(ctx)
grafanaClient, err := client2.NewGeneratedGrafanaClient(ctx, r.Client, &grafana)
if err != nil {
return err
Expand All @@ -806,10 +809,10 @@ func (r *GrafanaDashboardReconciler) UpdateHomeDashboard(ctx context.Context, gr
HomeDashboardUID: uid,
})
if err != nil {
r.Log.Error(err, "unable to update the home dashboard", "namespace", dashboard.Namespace, "name", dashboard.Name)
log.Error(err, "unable to update the home dashboard", "namespace", dashboard.Namespace, "name", dashboard.Name)
return err
}

r.Log.Info("home dashboard configured", "namespace", dashboard.Namespace, "name", dashboard.Name)
log.Info("home dashboard configured", "namespace", dashboard.Namespace, "name", dashboard.Name)
return nil
}
3 changes: 0 additions & 3 deletions controllers/dashboard_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
"syscall"
"testing"

ctrl "sigs.k8s.io/controller-runtime"

"github.com/grafana/grafana-operator/v5/api/v1beta1"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -150,7 +148,6 @@ func TestGetDashboardEnvs(t *testing.T) {

reconciler := &GrafanaDashboardReconciler{
Client: k8sClient,
Log: ctrl.Log.WithName("TestDashboardReconciler"),
}

envs, err := reconciler.getDashboardEnvs(ctx, &dashboard)
Expand Down
Loading