From a99ed2449a466c26df250246a76831c16c3ccbb2 Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Fri, 10 May 2024 08:40:56 -0700 Subject: [PATCH 1/5] Select correct tenant during query federation We are creating a job which has the tenant name for each tenant to query, but we use the incorrect index in the `ids` slice that contains the list of tenants. This causes the querier to select results for the first tenant in the list of tenants to match with instead of the actual tenant the querier was intended for. I updated the existing tests to check the labels of the results as well instead of only relying on the number of series found. Fixes https://github.com/cortexproject/cortex/issues/5941 Signed-off-by: Charlie Le --- .../tenantfederation/merge_queryable.go | 2 +- .../tenantfederation/merge_queryable_test.go | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/querier/tenantfederation/merge_queryable.go b/pkg/querier/tenantfederation/merge_queryable.go index c765f9f2d3..c172261d1e 100644 --- a/pkg/querier/tenantfederation/merge_queryable.go +++ b/pkg/querier/tenantfederation/merge_queryable.go @@ -339,7 +339,7 @@ func (m *mergeQuerier) Select(ctx context.Context, sortSeries bool, hints *stora return fmt.Errorf("unexpected type %T", jobIntf) } // Based on parent ctx here as we are using lazy querier. - newCtx := user.InjectOrgID(parentCtx, ids[job.pos]) + newCtx := user.InjectOrgID(parentCtx, job.id) seriesSets[job.pos] = &addLabelsSeriesSet{ upstream: job.querier.Select(newCtx, sortSeries, hints, filteredMatchers...), labels: labels.Labels{ diff --git a/pkg/querier/tenantfederation/merge_queryable_test.go b/pkg/querier/tenantfederation/merge_queryable_test.go index e8aa04ea28..f0b985a0a7 100644 --- a/pkg/querier/tenantfederation/merge_queryable_test.go +++ b/pkg/querier/tenantfederation/merge_queryable_test.go @@ -446,11 +446,42 @@ func TestMergeQueryable_Select(t *testing.T) { name: "should return only series for team-a and team-c tenants when there is a not-equals matcher for the team-b tenant", matchers: []*labels.Matcher{{Name: defaultTenantLabel, Value: "team-b", Type: labels.MatchNotEqual}}, expectedSeriesCount: 4, + expectedLabels: []labels.Labels{ + { + {Name: "__tenant_id__", Value: "team-a"}, + {Name: "instance", Value: "host1"}, + {Name: "tenant-team-a", Value: "static"}, + }, + { + {Name: "__tenant_id__", Value: "team-a"}, + {Name: "instance", Value: "host2.team-a"}, + }, + { + {Name: "__tenant_id__", Value: "team-c"}, + {Name: "instance", Value: "host1"}, + {Name: "tenant-team-c", Value: "static"}, + }, + { + {Name: "__tenant_id__", Value: "team-c"}, + {Name: "instance", Value: "host2.team-c"}, + }, + }, }, { name: "should return only series for team-b when there is an equals matcher for the team-b tenant", matchers: []*labels.Matcher{{Name: defaultTenantLabel, Value: "team-b", Type: labels.MatchEqual}}, expectedSeriesCount: 2, + expectedLabels: []labels.Labels{ + { + {Name: "__tenant_id__", Value: "team-b"}, + {Name: "instance", Value: "host1"}, + {Name: "tenant-team-b", Value: "static"}, + }, + { + {Name: "__tenant_id__", Value: "team-b"}, + {Name: "instance", Value: "host2.team-b"}, + }, + }, }, { name: "should return one series for each tenant when there is an equals matcher for the host1 instance", @@ -516,6 +547,19 @@ func TestMergeQueryable_Select(t *testing.T) { name: "should return only series for team-b when there is an equals matcher for team-b tenant", matchers: []*labels.Matcher{{Name: defaultTenantLabel, Value: "team-b", Type: labels.MatchEqual}}, expectedSeriesCount: 2, + expectedLabels: []labels.Labels{ + { + {Name: "__tenant_id__", Value: "team-b"}, + {Name: "instance", Value: "host1"}, + {Name: "original___tenant_id__", Value: "original-value"}, + {Name: "tenant-team-b", Value: "static"}, + }, + { + {Name: "__tenant_id__", Value: "team-b"}, + {Name: "instance", Value: "host2.team-b"}, + {Name: "original___tenant_id__", Value: "original-value"}, + }, + }, }, { name: "should return all series when there is an equals matcher for the original value of __tenant_id__ using the revised tenant label", From cd7d2298aeb366d108844821c90f3ec97c129a20 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Wed, 15 May 2024 15:32:32 -0700 Subject: [PATCH 2/5] fix response to omitempty (#5953) Signed-off-by: Ben Ye --- CHANGELOG.md | 4 ++++ pkg/ruler/api_test.go | 8 ++++---- pkg/util/api/response.go | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85f0b293a2..44a4af9467 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master / unreleased +## 1.17.1 2024-05-20 + +* [CHANGE] Query Frontend/Ruler: Omit empty data field in API response. #5953 + ## 1.17.0 2024-04-30 * [CHANGE] Azure Storage: Upgraded objstore dependency and support Azure Workload Identity Authentication. Added `connection_string` to support authenticating via SAS token. Marked `msi_resource` config as deprecating. #5645 diff --git a/pkg/ruler/api_test.go b/pkg/ruler/api_test.go index 6cbe5b594a..22f4537ba0 100644 --- a/pkg/ruler/api_test.go +++ b/pkg/ruler/api_test.go @@ -337,7 +337,7 @@ func TestRuler_DeleteNamespace(t *testing.T) { router.ServeHTTP(w, req) require.Equal(t, http.StatusAccepted, w.Code) - require.Equal(t, "{\"status\":\"success\",\"data\":null,\"errorType\":\"\",\"error\":\"\"}", w.Body.String()) + require.Equal(t, "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", w.Body.String()) // On Partial failures req = requestFor(t, http.MethodDelete, "https://localhost:8080/api/v1/rules/namespace2", nil, "user1") @@ -345,7 +345,7 @@ func TestRuler_DeleteNamespace(t *testing.T) { router.ServeHTTP(w, req) require.Equal(t, http.StatusInternalServerError, w.Code) - require.Equal(t, "{\"status\":\"error\",\"data\":null,\"errorType\":\"server_error\",\"error\":\"unable to delete rg\"}", w.Body.String()) + require.Equal(t, "{\"status\":\"error\",\"errorType\":\"server_error\",\"error\":\"unable to delete rg\"}", w.Body.String()) } func TestRuler_LimitsPerGroup(t *testing.T) { @@ -430,7 +430,7 @@ rules: - record: up_rule expr: up{} `, - output: "{\"status\":\"success\",\"data\":null,\"errorType\":\"\",\"error\":\"\"}", + output: "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", }, { name: "when exceeding the rule group limit after sending the first group", @@ -490,7 +490,7 @@ rules: expr: |2+ up{} `, - output: "{\"status\":\"success\",\"data\":null,\"errorType\":\"\",\"error\":\"\"}", + output: "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", }, { name: "when pushing group that CANNOT be safely converted from RuleGroupDesc to RuleGroup yaml", diff --git a/pkg/util/api/response.go b/pkg/util/api/response.go index 16e016bd5b..fd3be33ae1 100644 --- a/pkg/util/api/response.go +++ b/pkg/util/api/response.go @@ -19,7 +19,7 @@ const ( // Response defines the Prometheus response format. type Response struct { Status string `json:"status"` - Data interface{} `json:"data"` + Data interface{} `json:"data,omitempty"` ErrorType v1.ErrorType `json:"errorType"` Error string `json:"error"` Warnings []string `json:"warnings,omitempty"` From 801e16d7909c57eae931061906fbc70c260b3f14 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Thu, 16 May 2024 06:00:24 -0700 Subject: [PATCH 3/5] Omit empty for error and error types (#5954) --- CHANGELOG.md | 2 +- pkg/ruler/api_test.go | 6 +++--- pkg/util/api/response.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44a4af9467..6438bbf79d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ## 1.17.1 2024-05-20 -* [CHANGE] Query Frontend/Ruler: Omit empty data field in API response. #5953 +* [CHANGE] Query Frontend/Ruler: Omit empty data, errorType and error fields in API response. #5953 #5954 ## 1.17.0 2024-04-30 diff --git a/pkg/ruler/api_test.go b/pkg/ruler/api_test.go index 22f4537ba0..132ecfcf64 100644 --- a/pkg/ruler/api_test.go +++ b/pkg/ruler/api_test.go @@ -337,7 +337,7 @@ func TestRuler_DeleteNamespace(t *testing.T) { router.ServeHTTP(w, req) require.Equal(t, http.StatusAccepted, w.Code) - require.Equal(t, "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", w.Body.String()) + require.Equal(t, "{\"status\":\"success\"}", w.Body.String()) // On Partial failures req = requestFor(t, http.MethodDelete, "https://localhost:8080/api/v1/rules/namespace2", nil, "user1") @@ -430,7 +430,7 @@ rules: - record: up_rule expr: up{} `, - output: "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", + output: "{\"status\":\"success\"}", }, { name: "when exceeding the rule group limit after sending the first group", @@ -490,7 +490,7 @@ rules: expr: |2+ up{} `, - output: "{\"status\":\"success\",\"errorType\":\"\",\"error\":\"\"}", + output: "{\"status\":\"success\"}", }, { name: "when pushing group that CANNOT be safely converted from RuleGroupDesc to RuleGroup yaml", diff --git a/pkg/util/api/response.go b/pkg/util/api/response.go index fd3be33ae1..7e7c7713a2 100644 --- a/pkg/util/api/response.go +++ b/pkg/util/api/response.go @@ -20,8 +20,8 @@ const ( type Response struct { Status string `json:"status"` Data interface{} `json:"data,omitempty"` - ErrorType v1.ErrorType `json:"errorType"` - Error string `json:"error"` + ErrorType v1.ErrorType `json:"errorType,omitempty"` + Error string `json:"error,omitempty"` Warnings []string `json:"warnings,omitempty"` } From bffc52a6165b15fabc81efbdfd1fec4fc22cbb8c Mon Sep 17 00:00:00 2001 From: Charlie Le <3375195+CharlieTLe@users.noreply.github.com> Date: Sun, 19 May 2024 16:00:54 -0700 Subject: [PATCH 4/5] Ingester: Add `upload_compacted_blocks_enabled` config to ingester to parameterize uploading compacted blocks (#5959) In v1.15.2, ingesters configured with OOO samples ingestion enabled could hit this bug (https://github.com/cortexproject/cortex/issues/5402) where ingesters would not upload compacted blocks (https://github.com/thanos-io/thanos/issues/6462). In v1.16.1, ingesters are configured to always upload compacted blocks (https://github.com/cortexproject/cortex/pull/5625). In v1.17, ingesters stopped uploading compacted blocks (https://github.com/cortexproject/cortex/pull/5735). This can cause problems for users upgrading from v1.15.2 with OOO ingestion enabled to v1.17 because both versions are hard coded to disable uploading compacted blocks from the ingesters. The workaround was to downgrade from v1.17 to v1.16 to allow those compacted blocks to be uploaded (and eventually deleted). The new flag is set to true by default which reverts the behavior of the ingester uploading compacted blocks back to v1.16. Signed-off-by: Charlie Le --- CHANGELOG.md | 1 + docs/configuration/config-file-reference.md | 4 ++++ pkg/ingester/ingester.go | 8 +++++--- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6438bbf79d..58c5cc0e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ## 1.17.1 2024-05-20 * [CHANGE] Query Frontend/Ruler: Omit empty data, errorType and error fields in API response. #5953 #5954 +* [ENHANCEMENT] Ingester: Added `upload_compacted_blocks_enabled` config to ingester to parameterize uploading compacted blocks. #5959 ## 1.17.0 2024-04-30 diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 30bbee288c..e5341fe827 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -2916,6 +2916,10 @@ lifecycler: # CLI flag: -ingester.active-series-metrics-idle-timeout [active_series_metrics_idle_timeout: | default = 10m] +# Enable uploading compacted blocks. +# CLI flag: -ingester.upload-compacted-blocks-enabled +[upload_compacted_blocks_enabled: | default = true] + instance_limits: # Max ingestion rate (samples/sec) that ingester will accept. This limit is # per-ingester, not per-tenant. Additional push requests will be rejected. diff --git a/pkg/ingester/ingester.go b/pkg/ingester/ingester.go index 1b552c330e..65b8a61344 100644 --- a/pkg/ingester/ingester.go +++ b/pkg/ingester/ingester.go @@ -110,6 +110,9 @@ type Config struct { // Use blocks storage. BlocksStorageConfig cortex_tsdb.BlocksStorageConfig `yaml:"-"` + // UploadCompactedBlocksEnabled enables uploading compacted blocks. + UploadCompactedBlocksEnabled bool `yaml:"upload_compacted_blocks_enabled"` + // Injected at runtime and read from the distributor config, required // to accurately apply global limits. DistributorShardingStrategy string `yaml:"-"` @@ -143,6 +146,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.ActiveSeriesMetricsUpdatePeriod, "ingester.active-series-metrics-update-period", 1*time.Minute, "How often to update active series metrics.") f.DurationVar(&cfg.ActiveSeriesMetricsIdleTimeout, "ingester.active-series-metrics-idle-timeout", 10*time.Minute, "After what time a series is considered to be inactive.") + f.BoolVar(&cfg.UploadCompactedBlocksEnabled, "ingester.upload-compacted-blocks-enabled", true, "Enable uploading compacted blocks.") f.Float64Var(&cfg.DefaultLimits.MaxIngestionRate, "ingester.instance-limits.max-ingestion-rate", 0, "Max ingestion rate (samples/sec) that ingester will accept. This limit is per-ingester, not per-tenant. Additional push requests will be rejected. Current ingestion rate is computed as exponentially weighted moving average, updated every second. This limit only works when using blocks engine. 0 = unlimited.") f.Int64Var(&cfg.DefaultLimits.MaxInMemoryTenants, "ingester.instance-limits.max-tenants", 0, "Max users that this ingester can hold. Requests from additional users will be rejected. This limit only works when using blocks engine. 0 = unlimited.") f.Int64Var(&cfg.DefaultLimits.MaxInMemorySeries, "ingester.instance-limits.max-series", 0, "Max series that this ingester can hold (across all tenants). Requests to create additional series will be rejected. This limit only works when using blocks engine. 0 = unlimited.") @@ -2107,9 +2111,7 @@ func (i *Ingester) createTSDB(userID string) (*userTSDB, error) { func() labels.Labels { return l }, metadata.ReceiveSource, func() bool { - // There is no need to upload compacted blocks since OOO blocks - // won't be compacted due to overlap. - return false + return i.cfg.UploadCompactedBlocksEnabled }, true, // Allow out of order uploads. It's fine in Cortex's context. metadata.NoneFunc, From 22cc7d2f4ab7a3c746e183fedca3e00bddb6e528 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Sun, 19 May 2024 22:24:15 -0700 Subject: [PATCH 5/5] update changelog and version Signed-off-by: Ben Ye --- CHANGELOG.md | 1 + VERSION | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c5cc0e9c..73c55939de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [CHANGE] Query Frontend/Ruler: Omit empty data, errorType and error fields in API response. #5953 #5954 * [ENHANCEMENT] Ingester: Added `upload_compacted_blocks_enabled` config to ingester to parameterize uploading compacted blocks. #5959 +* [BUGFIX] Querier: Select correct tenant during query federation. #5943 ## 1.17.0 2024-04-30 diff --git a/VERSION b/VERSION index 092afa15df..511a76e6fa 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.17.0 +1.17.1