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

Jump to bottom

Runtime config: do not validate nil limits #4251

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Runtime config: do not validate nil limits #4251

merged 2 commits into from
Sep 22, 2021

Conversation

dannykopping
Copy link
Contributor

@dannykopping dannykopping commented Sep 1, 2021 •
edited
Loading

Signed-off-by: Danny Kopping danny.kopping@grafana.com

What this PR does / why we need it:
Fixes a panic which occurs when an overrides file has an entry defined for a tenant without any limits:

overrides:
  "54321":
    max_query_series: 10000
  "12345":

12345 in the example above will be unmarshaled to a nil value, and cause a panic.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@dannykopping dannykopping requested a review from a team as a code owner September 1, 2021 14:14
cstyan
cstyan reviewed Sep 15, 2021
View reviewed changes
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

IMO this should result in an error, or at least logging of some info about the nil entry.

Danny Kopping added 2 commits September 16, 2021 09:23
Do not validate nil limits
8d76cd4 
If an overrides file has an entry for a tenant without any limits, this will cause a panic

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Warn when an empty limit is skipped
0873d6b 
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
@dannykopping dannykopping merged commit 0f2f42d into grafana:main Sep 22, 2021
@dannykopping dannykopping deleted the dannykopping/limit-reload branch September 22, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cstyan cstyan cstyan approved these changes

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dannykopping @cstyan