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

[Improvement] Fix partially the throtteling bug. #607

Merged
merged 9 commits into from
Nov 23, 2021

Conversation

Cellebyte
Copy link
Contributor

@Cellebyte Cellebyte commented Nov 10, 2021

Description

Added some minor updates to the CRDs to support the missing security flag.
I also reduced the api requests.

Relevant issues/tickets

Type of change

I removed the throtteling a little bit by not asking for all api-resources in the cluster which are stable.
Now I reduced it only to the apiGroupVersion which reduces the request amount by huge margin.
This should mostly remove the throtteling messages.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Verified independently on a cluster by reviewer
  • Verified against an OpenShift Cluster.

@NissesSenap
Copy link
Collaborator

Hi @Cellebyte as mentioned in #603 there is no point in enabling DisableInitialAdminCreation since we use the admin user to create dashboards.
Try this change locally and add a dashboard and I'm rather sure it shouldn't work.

The partial solution if minimizing the API calls Is more interesting though.
So if you update the PR to only include that we could take a look at that part

@pb82 have started to look at number of API requests as well so I will let him look at that part.

@Cellebyte
Copy link
Contributor Author

Hi @Cellebyte as mentioned in #603 there is no point in enabling DisableInitialAdminCreation since we use the admin user to create dashboards. Try this change locally and add a dashboard and I'm rather sure it shouldn't work.

The partial solution if minimizing the API calls Is more interesting though. So if you update the PR to only include that we could take a look at that part

@pb82 have started to look at number of API requests as well so I will let him look at that part.

I reverted the changes I did to the CRD.
I only left the fix for the API Requests.

@Cellebyte
Copy link
Contributor Author

@NissesSenap is this now ready to merge?

@Cellebyte Cellebyte changed the title [Improvements] Fix partially the throtteling bug and added the required field to the operator crd. [Improvements] Fix partially the throtteling bug. Nov 19, 2021
@Cellebyte Cellebyte changed the title [Improvements] Fix partially the throtteling bug. [Improvement] Fix partially the throtteling bug. Nov 19, 2021
@pb82
Copy link
Collaborator

pb82 commented Nov 19, 2021

@Cellebyte i'll give it a try and merge early next week, thanks a lot for the contribution!

Copy link
Collaborator

@pb82 pb82 left a comment

Choose a reason for hiding this comment

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

LGTM, that makes sense.

@NissesSenap NissesSenap merged commit 6bcff3f into grafana:master Nov 23, 2021
@Cellebyte Cellebyte deleted the add/bitnami branch November 24, 2021 18:00
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.

[Bug] Excessive amount of queries to apiserver
4 participants