-
Notifications
You must be signed in to change notification settings - Fork 823
change to check tenant limit validation only for some targets #6880
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
change to check tenant limit validation only for some targets #6880
Conversation
pkg/cortex/runtime_config.go
Outdated
if err := ul.Validate(l.cfg.Distributor.ShardByAllLabels, l.cfg.Ingester.ActiveSeriesMetricsEnabled); err != nil { | ||
return nil, err | ||
targetStr := l.cfg.Target.String() | ||
if strings.Contains(targetStr, All) || strings.Contains(targetStr, Distributor) || strings.Contains(targetStr, Querier) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SungJin1212 Is querier here because it also use the shardByLabes to get user shard and query? Should we add ruler as well as it does the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielblando
This is due to docs. The user only enables -distributor.shard-by-all-labels
to the distributor and the querier. Then the other components would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required in querier for
cortex/pkg/distributor/query.go
Line 103 in 960b372
if !d.cfg.ShardByAllLabels && len(matchers) > 0 { |
As @danielblando mentioned we need to also include Ruler as Ruler is basically distributor + querier. We should also update the doc to mention that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielblando @yeya24
I updated both PR and Docs. Thanks for the comments!
9e127cb
to
03f8e2a
Compare
Signed-off-by: SungJin1212 <[email protected]>
03f8e2a
to
f7dcba4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR changes to check tenant limit validation when loading runtime config only for
all
,distributor
,querier
, andruler
targets. Without this, the other components could fail when the-distributor.shard-by-all-labels
is false.Which issue(s) this PR fixes:
Fixes #6741
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]