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.

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 Grafana reconcile loop #1880

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Mar 1, 2025

  • Feat: Added namespace label to instance metrics, instances sharing names across namespaces are now have separate counters.
  • Feat: AllowApplyFailed condition tests to still work by adding a grafana/skip-ready-check annotation to instances.
  • Refactor: Grafana status is now updated once in a deferred function and not needlessly copied.
  • Refactor: all Grafana instance reconciler signatures no longer take a GrafanaStatus struct
    Locally unused args are prefixed with _ for clarity
  • Refactor: Moved r.getVersion from GrafanaReconciler to CompleteReconciler
    External and internal instances now use the same reconciliation logic.
    Failing to fetch the version now marks the instance unready status.stageStatus=failed with a status.lastMessage
    This allows sorting out External instances as unready by GetScopedMatchingInstances.
  • Test: new condition option stage=complete stageStatus=failed on instances.

New namespace metric label:

# kubectl port-forward -n grafana-operator-system deployments/grafana-operator-controller-manager-v5 9090 &
# curl http://localhost:9090/metrics | grep namespace=

grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="GET",namespace="default",status="200"} 2362
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="GET",namespace="default",status="404"} 5
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="POST",namespace="default",status="200"} 3
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="POST",namespace="default",status="201"} 2
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="POST",namespace="default",status="202"} 1
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="PUT",namespace="default",status="200"} 786
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="PUT",namespace="default",status="202"} 1052
grafana_operator_grafana_api_requests{instance_name="grafana-testdata",method="PUT",namespace="default",status="500"} 1
grafana_operator_reconciler_failed_reconciles{instance_name="grafana",namespace="default",stage="complete"} 5
grafana_operator_reconciler_failed_reconciles{instance_name="grafana",namespace="default",stage="ingress"} 13
grafana_operator_reconciler_failed_reconciles{instance_name="grafana-testdata",namespace="default",stage="complete"} 12
grafana_operator_reconciler_reconciles{instance_name="grafana",namespace="default"} 18
grafana_operator_reconciler_reconciles{instance_name="grafana-testdata",namespace="default"} 13

@github-actions github-actions bot added the refactor this PR refactors code without introducing functionality label Mar 1, 2025
@Baarsgaard Baarsgaard marked this pull request as ready for review March 1, 2025 16:37
@theSuess theSuess added this to the v5.18.0 milestone Mar 3, 2025
Use status struct directly

remove updateStatus funct

defer status updates

Removed duplicate version checks
AdminUrl is now handled as part of the completeReconciler

External grafanas are now marked as failed when version cannot be fetched
chore: Clean up getVersion error messages
@Baarsgaard Baarsgaard force-pushed the refactor_grafana_reconcile_loop branch 4 times, most recently from 68c8576 to f096403 Compare March 6, 2025 21:36
@Baarsgaard Baarsgaard force-pushed the refactor_grafana_reconcile_loop branch from f096403 to 640ac23 Compare March 6, 2025 22:22
@Baarsgaard Baarsgaard requested a review from weisdd March 7, 2025 08:25
@@ -87,9 +87,11 @@ func GetScopedMatchingInstances(ctx context.Context, k8sClient client.Client, cr
if !selected {
continue
}

doReadyCheck := instance.Annotations["grafana/skip-ready-check"] == ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the comment explaining usage of readiness checks here.
Also, let's rename it from readyCheck to readinessCheck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor this PR refactors code without introducing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants