Repository navigation
Validate tenant limits in user-overrides endpoint - #7897
SungJin1212 wants to merge 1 commit into
Conversation
381de3b to
c39c181
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
c39c181 to
7e3e7fd
Compare
sandy2008
left a comment
There was a problem hiding this comment.
Validating before upload makes sense and the loader refactor is behaviour-preserving; the unit and runtime-config tests pass for me. One concern about split deployments inline.
| func (t *Cortex) initOverrides() (services.Service, error) { | ||
|
|
||
| overridesAPI, err := overrides.New(t.Cfg.RuntimeConfig, util_log.Logger, prometheus.DefaultRegisterer) | ||
| overridesAPI, err := overrides.New(t.Cfg.RuntimeConfig, runtimeConfigLoader{cfg: t.Cfg}.validateTenantLimits, util_log.Logger, prometheus.DefaultRegisterer) |
There was a problem hiding this comment.
This validator runs with the overrides process's own t.Cfg, while runtimeConfigLoader.load only validates tenant limits for the all/distributor/querier/ruler targets (the #6741 gating in runtime_config.go). So a standalone -target=overrides that doesn't also get -distributor.shard-by-all-labels=true, the setup from #6741, will now reject max_global_series_per_user with a 400 even though the components that load the file accept it. I checked with a quick probe: with Target: overrides and default flags, load() accepts that override but validateTenantLimits returns "the ingester.max-global-series-per-user limit is unsupported if distributor.shard-by-all-labels is disabled". The same applies to the active-series, HA tracker timeout, close-idle-TSDB and name-validation settings. Could we at least document that the overrides component needs the same values as the rest of the cluster (suggestion on the guide)?
|
|
||
| ### Limits Validation | ||
|
|
||
| Before writing to the runtime configuration file, the merged limits of the tenant are validated with the same checks that Cortex components run when they load the runtime configuration. For example, setting `max_global_series_per_user` is rejected when `-distributor.shard-by-all-labels` is disabled. |
There was a problem hiding this comment.
| Before writing to the runtime configuration file, the merged limits of the tenant are validated with the same checks that Cortex components run when they load the runtime configuration. For example, setting `max_global_series_per_user` is rejected when `-distributor.shard-by-all-labels` is disabled. | |
| Before writing to the runtime configuration file, the merged limits of the tenant are validated with the same checks that Cortex components run when they load the runtime configuration, using the configuration of the process that runs the overrides module. For example, setting `max_global_series_per_user` is rejected when `-distributor.shard-by-all-labels` is disabled there, so when the overrides module runs as its own target it needs the same `-distributor.shard-by-all-labels`, `-ingester.active-series-metrics-enabled`, `-distributor.ha-tracker.update-timeout`, `-distributor.ha-tracker.update-timeout-jitter-max`, `-blocks-storage.tsdb.close-idle-tsdb-timeout` and `-name-validation-scheme` values as the rest of the cluster. |
What this PR does:
POST /api/v1/user-overridescurrently writes the merged tenant limits to the runtime config without running the validation that components apply when loading it. While the invalid entry stays in the file, no further runtime config changes take effect on components, and any of them restarted in the meantime fail to start withfailed to load runtime config.This PR validates the merged limits of the tenant before uploading them.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags