Add tombstone cli and mask store series using tombstones #4790

yeya24 · 2021-10-18T06:23:50Z

Signed-off-by: yeya24 [email protected]

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

This pr continues the previous tombstones related work. It includes two prs:

Verification

TODO:
Unit tests and E2E tests TBD.

GiedriusS · 2021-10-18T20:39:13Z

pkg/tombstone/tombstone.go

+	if err := bkt.Iter(ctx, TombstoneDir, func(name string) error {
+		tombstoneFilename := path.Join("", name)
+		tombstoneFile, err := bkt.Get(ctx, tombstoneFilename)
+		if err != nil {


Do we need to check whether it is 404 or something else?

pkg/tombstone/tombstone.go

GiedriusS · 2021-10-18T20:42:52Z

pkg/tombstone/tombstone.go

+		Matchers:     matchers,
+		MinTime:      minTime,
+		MaxTime:      maxTime,
+		CreationTime: timestamp.FromTime(time.Now()),


For mocking in tests you'll probably want to pass this as a parameter instead so that you could mock it

GiedriusS · 2021-10-18T20:43:40Z

pkg/store/bucket.go

@@ -1443,6 +1475,13 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR
 	}, nil
 }

+func (s *BucketStore) SyncTombstones(ctx context.Context) (err error) {
+	s.tombstonesMtx.Lock()
+	s.tombstones, err = tombstone.ReadTombstones(ctx, s.bkt, s.logger)


This could hold the lock for a long time. I suggest retrieving first and then locking -> switching -> unlocking

Yeah, that would be better. I will add ReadTombstones as part of the BucketStore method

I guess I misunderstood your suggestion when I first commented on this 🤣 I think I just applied what you suggested. Thanks

pkg/store/bucket.go

GiedriusS

I think we are still missing https://github.com/thanos-io/thanos/pull/3075/files#r477281247 to avoid unnecessary postings lookups? ;P

yeya24 · 2021-10-18T23:08:22Z

I think we are still missing https://github.com/thanos-io/thanos/pull/3075/files#r477281247 to avoid unnecessary postings lookups? ;P

I am not sure but I guess what I am doing now is what Bartek meant in https://github.com/thanos-io/thanos/pull/3075/files#r477281247?

Now for tombstones matchers, we don't expand postings. Instead we just match tombstone matchers with each series labels we get.

yeya24 · 2021-10-18T23:11:00Z

pkg/store/bucket.go

+				if val := lset.Get(matcher.Name); val != "" {
+					if matcher.Matches(val) {
+						if skipChunks {
+							continue PostingsLoop


I am wondering about the case of SkipChunks. If the series matches tombstones, do we just skip that series labels? What about cases of LabelNames and LabelValues?
I think Prometheus doesn't care about these and tombstones only affect non-metadata queries.

Maybe we could add an e2e test for this case? 🤔

yeya24 · 2021-10-19T01:24:28Z

pkg/tombstone/tombstone.go

+	CreationTime int64             `json:"creationTime"`
+	Author       string            `json:"author"`
+	Reason       string            `json:"reason"`
+}


Also question about the tombstone file. How can we support multi-tenancy with the current format?
I guess we want to load some tombstones only for some blocks or tenants. Then does it make sense to add external labels here or should we add external labels to matchers here?

I would vote for the 1st option 👍 if none are specified then the tombstone could apply to all blocks

If we use labels to represent the external labels (tenants) for each tombstone then it doesn't support matching two tenants. We can either match 1 tenant or all tenants (specify no labels).

yeya24 · 2021-11-04T06:30:26Z

I am still thinking about per block tombstones.
For global tombstones, when should we clean up the data in each block and delete the tombstones?

After #4620 is done, there is a block API to do action per block level. In this case, users can easily add & modify tombstones directly in the UI per block level. WDYT?

stale · 2022-01-09T00:33:36Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

yeya24 · 2022-06-11T08:17:21Z

Think about the tombstone approach a little bit more.
Initially, I think it would be super expensive for the compactor to check series for every blocks using global tombstone files. As the compactor doesn't download blocks indexes at start, it has to download all the block index locally to check if the block has series to be rewritten, even if the block doesn't match the series marchers at all. That's why I think per block tombstone makes more sense cuz it is clear what blocks to rewrite.

But per block tombstone has its issues as well like UX, scalability, etc. To overcome the issue from the global tombstone approach, I'm wondering if we can have another gRPC API on store gateway to get the block IDs that matches specific matchers. Then the compactor can leverage this API to get the index information and perform series deletion.

Signed-off-by: yeya24 <[email protected]>

Signed-off-by: Ben Ye <[email protected]>

pkg/block/tombstone/tombstone.go

cmd/thanos/store.go

cmd/thanos/tools_bucket.go

pkg/block/tombstone/tombstone.go

pkg/compact/compact.go

sonatype-lift · 2022-06-18T09:29:04Z

pkg/tombstone/fetcher.go

+	syncs    prometheus.Counter
+	g        singleflight.Group
+
+	mtx    sync.Mutex


U1000: field mtx is unused

Reply with "@sonatype-lift help" for more info.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

pkg/tombstone/fetcher.go

sonatype-lift · 2022-06-18T09:29:16Z

pkg/tombstone/fetcher.go

+import (
+	"context"
+	"encoding/json"
+	"github.com/golang/groupcache/singleflight"


goimports: File is not goimports-ed

Reply with "@sonatype-lift help" for more info.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

cmd/thanos/store.go

pkg/tombstone/fetcher.go

sonatype-lift · 2022-06-18T09:29:22Z

pkg/tombstone/fetcher.go

+}
+
+type Fetcher struct {
+	bkt    objstore.InstrumentedBucketReader


structcheck: bkt is unused

Reply with "@sonatype-lift help" for more info.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

pkg/tombstone/fetcher.go

Signed-off-by: Ben Ye <[email protected]>

yeya24 · 2022-06-19T01:56:27Z

cmd/thanos/tools_bucket.go

@@ -153,6 +155,13 @@ type bucketMarkBlockConfig struct {
 	blockIDs []string
 }

+type bucketDeleteConfig struct {


I am wondering if we need to rename it because delete command might be ambiguous, whether it deletes series or deletes the whole block.
Maybe we can rename it to delete-series or create-tombstone, as we may need undelete-series or remove-tombstone in the future...

yeya24 · 2022-06-19T01:56:54Z

cmd/thanos/store.go

@@ -123,6 +124,12 @@ func (sc *storeConfig) registerFlag(cmd extkingpin.FlagClause) {
 	cmd.Flag("block-meta-fetch-concurrency", "Number of goroutines to use when fetching block metadata from object storage.").
 		Default("32").IntVar(&sc.blockMetaFetchConcurrency)

+	cmd.Flag("sync-tombstone-duration", "Repeat interval for syncing the tombstones between local and remote view.").


Wondering if we need another flag to enable/disable tombstone masking.

pkg/tombstone/fetcher.go

Signed-off-by: Ben Ye <[email protected]>

yeya24 · 2022-06-20T04:47:41Z

pkg/store/bucket.go

+					tombstoneIntervals = tombstoneIntervals.Add(promtombstones.Interval{Mint: ts.MinTime, Maxt: ts.MaxTime})
+				}
+			}
+		}


I am wondering which way is better. Now we are matching tombstones at the query time, which is online processing. Another way is to build an in-memory Prometheus tombstone for each matched block when we load tombstones each time. Since block contents are immutable, we can cache the in-mem tombstone results each time based on the tombstone ids.
Ideally, the second option is better in performance. But happy to hear more ideas.

pkg/store/bucket_test.go

Signed-off-by: Ben Ye <[email protected]>

sonatype-lift · 2022-06-20T23:38:13Z

pkg/tombstone/tombstone.go

+	return bkt.Upload(ctx, tsPath, bytes.NewBuffer(b))
+}
+
+// RemoveTombstone removes the tombstone from object storage


godot: Comment should end in a period

Reply with "@sonatype-lift help" for more info.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-06-20T23:38:15Z

pkg/tombstone/cache.go

+		}
+	}
+	for _, ts := range m.tombstones {
+		ts.Iter(func(id storage.SeriesRef, ivs tombstones.Intervals) error {


errcheck: Error return value of ts.Iter is not checked

Reply with "@sonatype-lift help" for more info.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

stale · 2022-09-21T00:13:18Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

yeya24 force-pushed the tombstones branch 3 times, most recently from d8a7b73 to 689ceb1 Compare October 18, 2021 07:26

GiedriusS reviewed Oct 18, 2021

View reviewed changes

yeya24 commented Oct 18, 2021

View reviewed changes

yeya24 commented Oct 19, 2021

View reviewed changes

stale bot added the stale label Jan 9, 2022

stale bot closed this Mar 2, 2022

yeya24 reopened this Jun 11, 2022

pull-request-size bot added the size/L label Jun 11, 2022

stale bot removed the stale label Jun 11, 2022

yeya24 and others added 3 commits June 15, 2022 16:51

add tombstone cli and mask store series using tombstones

9b8a888

Signed-off-by: yeya24 <[email protected]>

update imports

bd4c341

Signed-off-by: Ben Ye <[email protected]>

add sync tombstone duration flag

09c8316

Signed-off-by: Ben Ye <[email protected]>

yeya24 force-pushed the tombstones branch from 689ceb1 to 09c8316 Compare June 16, 2022 00:44

yeya24 marked this pull request as ready for review June 16, 2022 00:44