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

Track dashboards per namespace in ControllerConfig #690

Conversation

addreas
Copy link
Contributor

@addreas addreas commented Feb 16, 2022

Description

I was having issues with dashboards getting re-fetched and the grafana instance restarting constantly, and eventually I tracked it down to this being the root cause.

Relevant issues/tickets

Without digging too deep I see that #686 seems similar to the issues I was facing.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

Has been running in my home cluster for a while, and the issues have vanished, so it seems I did something right. Another pair of eyes never hurt though.

@HubertStefanski
Copy link
Member

HubertStefanski commented Feb 21, 2022

hey @addreas Can you verify that manifests/crds generate correctly for this type? AFAIR we had a few issues with generation because of the custom type in the map. However, if that works now, we're more than happy to revert to the mapped approach

@addreas addreas force-pushed the multi-namespace-dashboard-tracking branch from 4ffc730 to 0694317 Compare Feb 22, 2022
@addreas
Copy link
Contributor Author

addreas commented Feb 22, 2022

Hmm, not sure I follow. Isn't this type internal to the controller? I can't see this type used in anything generated, make generate manifests api-docs seem to work fine either way, though.

Pushed a fix for the formatting to make go-lint happy.

@HubertStefanski
Copy link
Member

HubertStefanski commented Mar 1, 2022

Hmm, not sure I follow. Isn't this type internal to the controller? I can't see this type used in anything generated, make generate manifests api-docs seem to work fine either way, though.

Pushed a fix for the formatting to make go-lint happy.

Thanks @addreas I'll take a look at this again today and verify it locally, then we can merge this!

@HubertStefanski
Copy link
Member

HubertStefanski commented Mar 1, 2022

@addreas I've had a chance to look through, Code looks good, however, the Grafana CR status isn't being updated with the references, as I think you intended for it to do. Currently this looks like it's stored in memory (which is fine as long as the operator pod is up and running)
@pb82 @NissesSenap @robshelly , What is your take on this? Are we Ok with keeping the dashboard refs in memory as opposed to in the status field? are there any edge cases you guys can think of where the operator might not correctly act on CRUD operations for a dashboard when it loses references?
I think we probably won't lose any data if the operator needs to resync itself with those dashboards (worst case it will force update the grafana pod)

@addreas
Copy link
Contributor Author

addreas commented Mar 2, 2022

Good catch, @HubertStefanski, didn't think of checking if that still worked before. Looking at my changes now I see how I might have stopped at making the compiler happy without actually thinking about what the code was intended to do. Taking a look at fixing it.

Copy link
Member

@HubertStefanski HubertStefanski left a comment

@addreas sorry for being soo slow with this, I verified it on my openshift cluster, works beautifully 👍

@HubertStefanski HubertStefanski merged commit 0f0e7d1 into grafana-operator:master Mar 24, 2022
8 checks passed
addreas added a commit to addreas/grafana-operator that referenced this pull request May 5, 2022
@addreas addreas mentioned this pull request May 5, 2022
7 tasks
NissesSenap pushed a commit that referenced this pull request May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants