Skip to content

Commit

Permalink
Don't include external labels in blocks uploaded by Ingester (grafana…
Browse files Browse the repository at this point in the history 
…#1972)

* Remove support for external labels.
* Fixed comments.
* Don't use TenantID label. Filter out the label during compaction.
* CHANGELOG.md
* Use public function from Thanos.
* Use new UploadBlock function, move GrpcContextMetadataTenantID constant.
* Rename tsdb2 import to mimir_tsdb.
* Fix tests.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
  • Loading branch information
@pstibrany
pstibrany authored Jun 3, 2022
1 parent 7a4b40a commit 8bff6d0
Show file tree
Hide file tree
Showing 27 changed files with 206 additions and 219 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* `-distributor.request-burst-limit`
* The following metric is exposed to tell how many requests have been rejected:
* `cortex_discarded_requests_total`
* [CHANGE] Blocks uploaded by ingester no longer contain `__org_id__` label. Compactor now ignores this label and will compact blocks with and without this label together. `mimirconvert` tool will remove the label from blocks as "unknown" label. #1972
* [ENHANCEMENT] Store-gateway: Add the experimental ability to run requests in a dedicated OS thread pool. This feature can be configured using `-store-gateway.thread-pool-size` and is disabled by default. Replaces the ability to run index header operations in a dedicated thread pool. #1660 #1812
* [ENHANCEMENT] Improved error messages to make them easier to understand; each now have a unique, global identifier that you can use to look up in the runbooks for more information. #1907 #1919 #1888 #1939 #1984
* [ENHANCEMENT] Memberlist KV: incoming messages are now processed on per-key goroutine. This may reduce loss of "maintanance" packets in busy memberlist installations, but use more CPU. New `memberlist_client_received_broadcasts_dropped_total` counter tracks number of dropped per-key messages. #1912
Expand Down
17 changes: 9 additions & 8 deletions cmd/metaconvert/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"os"
"os/signal"
"path"
"sort"
"syscall"

gklog "github.com/go-kit/log"
Expand Down Expand Up @@ -106,20 +107,20 @@ func convertTenantBlocks(ctx context.Context, userBucketClient objstore.Bucket,

updated := false

metaOrgID := meta.Thanos.Labels[mimir_tsdb.TenantIDExternalLabel]
if metaOrgID != tenant {
level.Warn(logger).Log("msg", "updating tenant label", "block", blockID.String(), "old_value", metaOrgID, "new_value", tenant)
updated = true
meta.Thanos.Labels[mimir_tsdb.TenantIDExternalLabel] = tenant
// Sort labels before processing to have stable output for testing.
var labels []string
for l := range meta.Thanos.Labels {
labels = append(labels, l)
}
sort.Strings(labels)

for l, v := range meta.Thanos.Labels {
for _, l := range labels {
switch l {
case mimir_tsdb.TenantIDExternalLabel, mimir_tsdb.IngesterIDExternalLabel, mimir_tsdb.CompactorShardIDExternalLabel:
case mimir_tsdb.CompactorShardIDExternalLabel:
continue
}

level.Warn(logger).Log("msg", "removing unknown label", "block", blockID.String(), "label", l, "value", v)
level.Warn(logger).Log("msg", "removing unknown label", "block", blockID.String(), "label", l, "value", meta.Thanos.Labels[l])
updated = true
delete(meta.Thanos.Labels, l)
}
Expand Down
73 changes: 30 additions & 43 deletions cmd/metaconvert/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package main

import (
"context"
"fmt"
"path"
"strings"
"testing"
Expand Down Expand Up @@ -67,8 +66,8 @@ func TestConvertTenantBlocks(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
"test": "label",
mimir_tsdb.TenantIDExternalLabel: "wrong tenant",
"test": "label",
"__org_id__": "wrong tenant",
},
},
},
Expand All @@ -80,9 +79,9 @@ func TestConvertTenantBlocks(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: "fake",
"__org_id__": "fake",
mimir_tsdb.CompactorShardIDExternalLabel: "1_of_10",
mimir_tsdb.IngesterIDExternalLabel: "ingester-1",
"__ingester_id__": "ingester-1",
},
},
},
Expand All @@ -94,7 +93,7 @@ func TestConvertTenantBlocks(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
"__org_id__": tenant,
},
},
},
Expand Down Expand Up @@ -127,9 +126,6 @@ func TestConvertTenantBlocks(t *testing.T) {

Thanos: metadata.Thanos{
Version: 10,
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
},
Downsample: metadata.ThanosDownsample{
Resolution: 15,
},
Expand All @@ -141,12 +137,6 @@ func TestConvertTenantBlocks(t *testing.T) {
BlockMeta: tsdb.BlockMeta{
ULID: blockWithWrongTenant,
},

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
},
},
},

blockWithManyMimirLabels: {
Expand All @@ -156,9 +146,7 @@ func TestConvertTenantBlocks(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
mimir_tsdb.CompactorShardIDExternalLabel: "1_of_10",
mimir_tsdb.IngesterIDExternalLabel: "ingester-1",
},
},
},
Expand All @@ -167,33 +155,33 @@ func TestConvertTenantBlocks(t *testing.T) {
BlockMeta: tsdb.BlockMeta{
ULID: blockWithNoChangesRequired,
},

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
},
},
},
}

for b, m := range expected {
meta, err := block.DownloadMeta(ctx, logger, bkt, b)
require.NoError(t, err)
require.Equal(t, m, meta)
require.NoError(t, err, b.String())

// Normalize empty map to nil to simplify tests.
if len(meta.Thanos.Labels) == 0 {
meta.Thanos.Labels = nil
}
require.Equal(t, m, meta, b.String())
}

assert.Equal(t, []string{
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000010000000000000000 old_value= new_value=target_tenant`,
`level=info tenant=target_tenant msg="changes required, uploading meta.json file" block=00000000010000000000000000`,
`level=info tenant=target_tenant msg="meta.json file uploaded successfully" block=00000000010000000000000000`,
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000020000000000000000 old_value="wrong tenant" new_value=target_tenant`,
`level=info tenant=target_tenant msg="no changes required" block=00000000010000000000000000`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000020000000000000000 label=__org_id__ value="wrong tenant"`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000020000000000000000 label=test value=label`,
`level=info tenant=target_tenant msg="changes required, uploading meta.json file" block=00000000020000000000000000`,
`level=info tenant=target_tenant msg="meta.json file uploaded successfully" block=00000000020000000000000000`,
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000030000000000000000 old_value=fake new_value=target_tenant`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000030000000000000000 label=__ingester_id__ value=ingester-1`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000030000000000000000 label=__org_id__ value=fake`,
`level=info tenant=target_tenant msg="changes required, uploading meta.json file" block=00000000030000000000000000`,
`level=info tenant=target_tenant msg="meta.json file uploaded successfully" block=00000000030000000000000000`,
`level=info tenant=target_tenant msg="no changes required" block=00000000040000000000000000`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000040000000000000000 label=__org_id__ value=target_tenant`,
`level=info tenant=target_tenant msg="changes required, uploading meta.json file" block=00000000040000000000000000`,
`level=info tenant=target_tenant msg="meta.json file uploaded successfully" block=00000000040000000000000000`,
}, strings.Split(strings.TrimSpace(logs.String()), "\n"))
}

Expand Down Expand Up @@ -242,8 +230,8 @@ func TestConvertTenantBlocksDryMode(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
"test": "label",
mimir_tsdb.TenantIDExternalLabel: "wrong tenant",
"test": "label",
"__org_id__": "wrong tenant",
},
},
},
Expand All @@ -255,9 +243,9 @@ func TestConvertTenantBlocksDryMode(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: "fake",
"__org_id__": "fake",
mimir_tsdb.CompactorShardIDExternalLabel: "1_of_10",
mimir_tsdb.IngesterIDExternalLabel: "ingester-1",
"__ingester_id__": "ingester-1",
},
},
},
Expand All @@ -269,7 +257,7 @@ func TestConvertTenantBlocksDryMode(t *testing.T) {

Thanos: metadata.Thanos{
Labels: map[string]string{
mimir_tsdb.TenantIDExternalLabel: tenant,
"__org_id__": tenant,
},
},
},
Expand All @@ -291,16 +279,15 @@ func TestConvertTenantBlocksDryMode(t *testing.T) {
require.Equal(t, m, meta)
}

fmt.Println(logs.String())

assert.Equal(t, []string{
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000010000000000000000 old_value= new_value=target_tenant`,
`level=warn tenant=target_tenant msg="changes required, not uploading back due to dry run" block=00000000010000000000000000`,
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000020000000000000000 old_value="wrong tenant" new_value=target_tenant`,
`level=info tenant=target_tenant msg="no changes required" block=00000000010000000000000000`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000020000000000000000 label=__org_id__ value="wrong tenant"`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000020000000000000000 label=test value=label`,
`level=warn tenant=target_tenant msg="changes required, not uploading back due to dry run" block=00000000020000000000000000`,
`level=warn tenant=target_tenant msg="updating tenant label" block=00000000030000000000000000 old_value=fake new_value=target_tenant`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000030000000000000000 label=__ingester_id__ value=ingester-1`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000030000000000000000 label=__org_id__ value=fake`,
`level=warn tenant=target_tenant msg="changes required, not uploading back due to dry run" block=00000000030000000000000000`,
`level=info tenant=target_tenant msg="no changes required" block=00000000040000000000000000`,
`level=warn tenant=target_tenant msg="removing unknown label" block=00000000040000000000000000 label=__org_id__ value=target_tenant`,
`level=warn tenant=target_tenant msg="changes required, not uploading back due to dry run" block=00000000040000000000000000`,
}, strings.Split(strings.TrimSpace(logs.String()), "\n"))
}
4 changes: 2 additions & 2 deletions pkg/compactor/bucket_compactor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ func (c *BucketCompactor) runCompactionJob(ctx context.Context, job *Job) (shoul
}

begin := time.Now()
if err := block.Upload(ctx, jobLogger, c.bkt, bdir, job.hashFunc); err != nil {
if err := mimit_tsdb.UploadBlock(ctx, jobLogger, c.bkt, bdir, nil); err != nil {
return errors.Wrapf(err, "upload of %s failed", blockToUpload.ulid)
}

Expand Down Expand Up @@ -562,7 +562,7 @@ func RepairIssue347(ctx context.Context, logger log.Logger, bkt objstore.Bucket,
}

level.Info(logger).Log("msg", "uploading repaired block", "newID", resid)
if err = block.Upload(ctx, logger, bkt, filepath.Join(tmpdir, resid.String()), metadata.NoneFunc); err != nil {
if err = mimit_tsdb.UploadBlock(ctx, logger, bkt, filepath.Join(tmpdir, resid.String()), nil); err != nil {
return errors.Wrapf(err, "upload of %s failed", resid)
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/compactor/bucket_compactor_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/thanos-io/thanos/pkg/block/metadata"
"github.com/thanos-io/thanos/pkg/objstore"

mimir_tsdb "github.com/grafana/mimir/pkg/storage/tsdb"
"github.com/grafana/mimir/pkg/storage/tsdb/bucketindex"
)

Expand Down Expand Up @@ -451,7 +452,7 @@ func createAndUpload(t testing.TB, bkt objstore.Bucket, blocks []blockgenSpec, b
for _, b := range blocks {
id, meta := createBlock(ctx, t, prepareDir, b)
metas = append(metas, meta)
require.NoError(t, block.Upload(ctx, log.NewNopLogger(), bkt, filepath.Join(prepareDir, id.String()), metadata.NoneFunc))
require.NoError(t, mimir_tsdb.UploadBlock(ctx, log.NewNopLogger(), bkt, filepath.Join(prepareDir, id.String()), nil))
}
for _, b := range blocksWithOutOfOrderChunks {
id, meta := createBlock(ctx, t, prepareDir, b)
Expand All @@ -460,7 +461,7 @@ func createAndUpload(t testing.TB, bkt objstore.Bucket, blocks []blockgenSpec, b
require.NoError(t, err)

metas = append(metas, meta)
require.NoError(t, block.Upload(ctx, log.NewNopLogger(), bkt, filepath.Join(prepareDir, id.String()), metadata.NoneFunc))
require.NoError(t, mimir_tsdb.UploadBlock(ctx, log.NewNopLogger(), bkt, filepath.Join(prepareDir, id.String()), nil))
}

return metas
Expand Down
7 changes: 6 additions & 1 deletion pkg/compactor/compactor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,12 @@ func (c *MultitenantCompactor) compactUser(ctx context.Context, userID string) e
fetcherFilters := []block.MetadataFilter{
// Remove the ingester ID because we don't shard blocks anymore, while still
// honoring the shard ID if sharding was done in the past.
NewLabelRemoverFilter([]string{mimir_tsdb.IngesterIDExternalLabel}),
// Remove TenantID external label to make sure that we compact blocks with and without the label
// together.
NewLabelRemoverFilter([]string{
mimir_tsdb.DeprecatedTenantIDExternalLabel,
mimir_tsdb.DeprecatedIngesterIDExternalLabel,
}),
block.NewConsistencyDelayMetaFilter(ulogger, c.compactorCfg.ConsistencyDelay, reg),
excludeMarkedForDeletionFilter,
deduplicateBlocksFilter,
Expand Down
30 changes: 22 additions & 8 deletions pkg/compactor/label_remover_filter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,31 @@ func TestLabelRemoverFilter(t *testing.T) {
input map[ulid.ULID]map[string]string
expected map[ulid.ULID]map[string]string
}{
"should remove cpnfigured labels": {
labels: []string{mimir_tsdb.IngesterIDExternalLabel},
"should remove configured labels": {
labels: []string{mimir_tsdb.DeprecatedIngesterIDExternalLabel},
input: map[ulid.ULID]map[string]string{
block1: {mimir_tsdb.IngesterIDExternalLabel: "ingester-0", mimir_tsdb.TenantIDExternalLabel: "user-1"},
block2: {mimir_tsdb.IngesterIDExternalLabel: "ingester-0", mimir_tsdb.TenantIDExternalLabel: "user-1"},
block3: {mimir_tsdb.IngesterIDExternalLabel: "ingester-0", mimir_tsdb.TenantIDExternalLabel: "user-1"},
block1: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block2: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block3: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
},
expected: map[ulid.ULID]map[string]string{
block1: {mimir_tsdb.TenantIDExternalLabel: "user-1"},
block2: {mimir_tsdb.TenantIDExternalLabel: "user-1"},
block3: {mimir_tsdb.TenantIDExternalLabel: "user-1"},
block1: {mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block2: {mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block3: {mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
},
},

"should remove configured labels 2": {
labels: []string{mimir_tsdb.DeprecatedIngesterIDExternalLabel, mimir_tsdb.DeprecatedTenantIDExternalLabel},
input: map[ulid.ULID]map[string]string{
block1: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block2: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
block3: {mimir_tsdb.DeprecatedIngesterIDExternalLabel: "ingester-0", mimir_tsdb.DeprecatedTenantIDExternalLabel: "user-1"},
},
expected: map[ulid.ULID]map[string]string{
block1: {},
block2: {},
block3: {},
},
},
}
Expand Down
Loading

0 comments on commit 8bff6d0

Please sign in to comment.