Support for tenant bucket prefixing in compactor by willh-db · Pull Request #239 · databricks/thanos

willh-db · 2025-10-30T23:55:21Z

Changes

Read from tenant_prefixes list in bucket config (passed by the init container)
If it is empty or does not exist, compactor proceeds as normal
If it exists, create one bucket per tenant with the appropriate prefixing (v1/raw/tenant) and proceed as normal per tenant, using the existing goroutine system

Verification

Added unit test for multi-tenant compactor

Note

Diff looks messy with many line changes because of whitespace changes in for loop -- please use split view to see changes

hczhu-db · 2025-10-31T04:32:13Z

cmd/thanos/compact.go

+type TenantBucketConfig struct {
+	TenantPrefixes []string `yaml:"tenant_prefixes"`
+	// Other config fields can be added here if needed
+}


I've just realized the client.BucketConfig struct is in another repo as a vendor repo to Thanos repo. We can't easily extend that struct. We have to copy that struct to here.

Copy and extend the struct.

type MultiTenancyBucketConfig struct { Type objstore.ObjProvider `yaml:"type"` Config interface{} `yaml:"config"` Prefix string `yaml:"prefix" default:""` // Example value: "v1/raw/tenant_a,v1/raw/tenant_b,v1/raw/tenant_c" TenantPrefixes []string `yaml:"tenant_prefixes"` }

Generate N client.BucketConfig instances from the MultileTenancyBucketConfig instance. Each of the instances has a different client.BucketConfig.Prefix which is join(BucketConfig.Prefix, MultileTenancyBucketConfig.TenantPrefixes[i])

Later on (not on this PR), compactor's init container will generate an MultiTenancyBucketConfig instance for its compactor container.

For references

I've just realized the client.BucketConfig struct is in another repo as a vendor repo to Thanos repo. We can't easily extend that struct. We have to copy that struct to here.

Sounds good. I wrote it this way thinking the init container could pass the list of tenants via the client.BucketConfig.Config, but this is cleaner and more type safe.

hczhu-db · 2025-10-31T04:43:10Z

cmd/thanos/compact.go

+type TenantBucketConfig struct {
+	TenantPrefixes []string `yaml:"tenant_prefixes"`
+	// Other config fields can be added here if needed
+}


For references

hczhu-db · 2025-10-31T17:06:36Z

cmd/thanos/compact.go

+	// Determine tenant prefixes to use (if provided)
+	var multiTenancyBucketConfig MultiTenancyBucketConfig
+	if err := yaml.Unmarshal(confContentYaml, &multiTenancyBucketConfig); err != nil {
+		return errors.Wrap(err, "parse bucket config")


"failed to parse MultiTenancyBucketConfig"

hczhu-db · 2025-10-31T17:11:35Z

cmd/thanos/compact.go

+			Config: multiTenancyBucketConfig.Config,
+			Prefix: multiTenancyBucketConfig.Prefix + tenantPrefix,
+		}
+


Log something like "starting compaction loop for prefix: " + "bucketConf.prefix"

hczhu-db · 2025-10-31T17:13:30Z

cmd/thanos/compact.go

+		// Create bucket for this tenant
+		if tenantPrefix != "" {
+			level.Info(logger).Log("msg", "creating compactor bucket with tenant prefix", "prefix", tenantPrefix)
 		}


After this point, the tenant prefix is abstracted out and just becomes prefix. We don't need anything special after this point.

We can remove it.

hczhu-db · 2025-10-31T17:14:21Z

cmd/thanos/compact.go

+			if tenantPrefix != "" {
+				level.Info(logger).Log("msg", "azdatalake sdk wrapper enabled for tenant", "prefix", tenantPrefix, "name", bkt.Name())
+			} else {
+				level.Info(logger).Log("msg", "azdatalake sdk wrapper enabled", "name", bkt.Name())
+			}
 		}


Remove the if and just log something agnostic to the tenant prefix.

hczhu-db · 2025-10-31T17:14:42Z

cmd/thanos/compact.go

+		// Create tenant-specific logger
+		tenantLogger := logger
+		if tenantPrefix != "" {
+			tenantLogger = log.With(logger, "tenant_prefix", tenantPrefix)
 		}
-		sy, err = compact.NewMetaSyncer(
-			logger,
-			reg,
-			insBkt,
-			cf,
-			duplicateBlocksFilter,
-			ignoreDeletionMarkFilter,
-			compactMetrics.blocksMarked.WithLabelValues(metadata.DeletionMarkFilename, ""),
-			compactMetrics.garbageCollectedBlocks,
-			syncMetasTimeout,
-		)
+


This is great!

This reverts commit fd8b9ee.

jnyi · 2025-12-02T00:23:19Z

cmd/thanos/compact.go

-	bkt, err := client.NewBucket(logger, confContentYaml, component.String(), nil)
-	if conf.enableFolderDeletion {
-		bkt, err = block.WrapWithAzDataLakeSdk(logger, confContentYaml, bkt)
-		level.Info(logger).Log("msg", "azdatalake sdk wrapper enabled", "name", bkt.Name())
-	}
-	if err != nil {
-		return err


I see a lot of code got shuffled rather than deleted which makes reviewing this pr hard. Wonder if it is possible to keep some of the unchanged code path the same? Might need to pay some efforts to find the right place to insert as well as introduce helper functions but may worth it (admit this function is ready long)

jnyi

really cool, comments most about styling, also thanks for adding unit tests!

jnyi · 2025-12-02T19:16:16Z

cmd/thanos/compact.go

+			insBkt,
+			conf.acceptMalformedIndex,
+			enableVerticalCompaction,
+			tenantReg,


same for this variable, is it possible to override instead

jnyi · 2025-12-02T19:16:47Z

cmd/thanos/compact.go

+			planner,
+			comp,
+			compact.DefaultBlockDeletableChecker{},
+			overlappingCallback,


if we don't change reg variable name, this line won't be changed.

jnyi · 2025-12-02T19:17:17Z

cmd/thanos/compact.go

-	if err := os.MkdirAll(compactDir, os.ModePerm); err != nil {
-		return errors.Wrap(err, "create working compact directory")
-	}
+	overlappingCallback := compact.NewOverlappingCompactionLifecycleCallback(reg, logger, conf.enableOverlappingRemoval)


no need to create temp variable which is only used once later in the old place

It is run once per tenant as it is within the loop but it shouldn't be an issue

jnyi · 2025-12-02T19:17:48Z

cmd/thanos/compact.go

+		if tenantPrefix != "" {
+			// For multi-tenant mode, we pass a nil registerer to avoid metric collisions
+			// TODO (willh-db): revisit metrics structure for multi-tenant mode
+			tenantReg = nil
+			insBkt = objstoretracing.WrapWithTraces(bkt)
+		} else {
+			tenantReg = reg
+			insBkt = objstoretracing.WrapWithTraces(objstore.WrapWithMetrics(bkt, extprom.WrapRegistererWithPrefix("thanos_", tenantReg), bkt.Name()))
 		}

-		progress.Set(compact.CleanBlocks)
-		compact.BestEffortCleanAbortedPartialUploads(ctx, logger, sy.Partial(), insBkt, compactMetrics.partialUploadDeleteAttempts, compactMetrics.blocksCleaned, compactMetrics.blockCleanupFailures)
-		if err := blocksCleaner.DeleteMarkedBlocks(ctx); err != nil {
-			return errors.Wrap(err, "cleaning marked blocks")
+		// Create tenant-specific logger
+		if tenantPrefix != "" {
+			logger = log.With(logger, "tenant_prefix", tenantPrefix)
 		}


nit: both use the same if condition, could merge it to simply

jnyi · 2025-12-02T19:22:03Z

cmd/thanos/compact.go

+				if tenantPrefix != "" {
+					f = baseMetaFetcher.NewMetaFetcher(nil, nil, "component", "globalBucketUI") // TODO (willh-db): revisit metrics here


some of the metrics might not be useful, so I don't think we need to alter all of them via this if-condition tenantPrefix != ""

jnyi · 2025-12-02T19:23:45Z

cmd/thanos/compact.go

+		bucketConf := &client.BucketConfig{
+			Type:   initialBucketConf.Type,
+			Config: initialBucketConf.Config,
+			Prefix: path.Join(initialBucketConf.Prefix, tenantPrefix),


just curious if tenantPrefix == "", it won't generate an empty folder in the middle?

Empty elements should be ignored: https://pkg.go.dev/path#Join

jnyi · 2025-12-02T19:26:57Z

cmd/thanos/compact.go

+		if tenantPrefix != "" {
+			// For multi-tenant mode, we pass a nil registerer to avoid metric collisions
+			// TODO (willh-db): revisit metrics structure for multi-tenant mode
+			tenantReg = nil


I'd suggest to use this function to augment tenantReg instead.

prometheus.Labels{"tenant": tenantPrefix}, reg, )

jnyi · 2025-12-02T19:28:08Z

cmd/thanos/compact.go

-		progress.Set(compact.SyncMeta)
-		if err := sy.SyncMetas(ctx); err != nil {
-			return errors.Wrap(err, "syncing metas")
+		if tenantPrefix != "" {


I see a lot of if conditions based on this, I'd suggest to create a bool variable instead:

isMultiTenantMode = tenantPrefix != "" if isMultiTenantMode { ... // do sth

jnyi

hi @willh-db , I actually had a better idea, WDYT?

The original runCompact function can be broken into 3 parts:

initial and parse config
setup bucket and use it in middle
post bucket setup after conf.wait

instead of injecting a for loop per tenant and change a umber of code inline, I am wondering if we could break the original function into 3 functions so most code lines retain its original position

func runCompact(args) {
   // part1

+ for tenants {
+ set up bucket per tenant
+  perBucketSetup(bucket_per_tenant, reg_per_tenant, logger_per_tenant)
+ }

+   postProcess()
+ }
+
+ func perBucketSetup() {. <-- add new func in the middle and call it from orignal with variable name unchanged.
   // part2 which we want to for-loop
   // most code don't need to change since we wrap it in a helper function
+ }
+ 
+ func postProcess() {
+ // part3
}

Compactor support tenant bucket prefixing

e2dbd29

willh-db requested review from hczhu-db and yuchen-db October 30, 2025 23:57

Pass linter test

3b7c382

hczhu-db suggested changes Oct 31, 2025

View reviewed changes

Using MultiTenancyBucketConfig

c81c5c0

willh-db requested a review from hczhu-db October 31, 2025 06:31

hczhu-db reviewed Oct 31, 2025

View reviewed changes

willh-db added 21 commits October 31, 2025 20:11

Changed logging

aa20425

Changed logging again

e4cf16b

Fixed NewBucket() issues

39a8f18

Changed function signature

fd8b9ee

Revert "Changed function signature"

d5184bc

This reverts commit fd8b9ee.

Fixed initialization error

4459d51

Debug messages

eaf92a8

Logging input for debugging

d0bd9b3

Handle empty object store config case

9ef964b

Removed debug messages

5370ee8

Fixed registries

a451954

Changed tenant reg label to tenant_prefix

753e560

Fixed reg vs tenantReg

24b3bc2

Fixed web UI duplicate conflicts

d7b40b6

Removed registerer from most tenant use cases

0824425

Set reg to nil for multitenant mode

912c489

Fixed nil pointer in insBkt

2e3f4a3

Fixes on reg

6635cad

Made sure reg is nil for tenants

89040d3

Brought back tenantReg

588dd12

Changed tenant config file method

31924f9

Fixed compact test

a90099a

willh-db marked this pull request as ready for review November 10, 2025 23:33

Changed cmd line flag

9a2f080

yuchen-db approved these changes Nov 11, 2025

View reviewed changes

willh-db requested a review from jnyi November 17, 2025 18:58

willh-db added 5 commits November 20, 2025 00:35

Merge branch 'db_main' into willh-db/compactor-tenant

5e19c0e

New version of tenant partitoning

fb8a05c

fixed tests

2da9e2e

Compute total shards

716be94

rename tenant-weights-file

e4e85a1

jnyi reviewed Dec 2, 2025

View reviewed changes

Changed tenantLogger and tenantReg

988af60

jnyi reviewed Dec 2, 2025

View reviewed changes

willh-db added 2 commits December 3, 2025 01:18

Changed tenantReg to reg and added baseReg

8f3fe1c

minor formatting

61bfc8c

jnyi reviewed Dec 3, 2025

View reviewed changes

willh-db added 2 commits December 8, 2025 20:42

Refactored into several functions

5339139

linter fix

1a7cc03

		if tenantPrefix != "" {
		f = baseMetaFetcher.NewMetaFetcher(nil, nil, "component", "globalBucketUI") // TODO (willh-db): revisit metrics here

Conversation

willh-db commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Note

Uh oh!

hczhu-db Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnyi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willh-db commented Oct 30, 2025 •

edited

Loading

hczhu-db Oct 31, 2025 •

edited

Loading

jnyi left a comment •

edited

Loading