Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid index errors for deleted tenants #2781

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Avoid index errors for deleted tenants #2781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a55c53d
Catch backend.ErrDoesNotExist from WriteTenantIndex
zalegrala Aug 10, 2023
c3ecc4f
Handle tenant index deletion when already deleted
zalegrala Aug 10, 2023
565834b
Capture notfound from azure.Delete
zalegrala Aug 10, 2023
a14bed2
Capture notfound from gcs.Delete
zalegrala Aug 10, 2023
0a7dd6f
Update changelog
zalegrala Aug 10, 2023
c19e367
Add note for why we skip ErrDoesNotExist
zalegrala Aug 14, 2023
7c3001e
Avoid handling the error twice
zalegrala Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* [BUGFIX] Fix panic in metrics summary api [#2738](https://github.com/grafana/tempo/pull/2738) (@mdisibio)
* [BUGFIX] Fix node role auth IDMSv1 [#2760](https://github.com/grafana/tempo/pull/2760) (@coufalja)
* [BUGFIX] Only search ingester blocks that fall within the request time range. [#2783](https://github.com/grafana/tempo/pull/2783) (@joe-elliott)
* [BUGFIX] Fix incorrect metrics for index failures [#2781](https://github.com/grafana/tempo/pull/2781) (@zalegrala)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did this have any impact besides metrics/extra logging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I'm thinking of. We still leave the spurious objects in the backend, so perhaps we can have a conversation about cleaning that up also.


## v2.2.0 / 2023-07-31

Expand Down
2 changes: 1 addition & 1 deletion tempodb/backend/azure/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func (rw *readerWriter) Delete(ctx context.Context, name string, keypath backend
}

if _, err = blobURL.Delete(ctx, blob.DeleteSnapshotsOptionInclude, blob.BlobAccessConditions{}); err != nil {
return errors.Wrapf(err, "error deleting blob, name: %s", name)
return readError(err)
}
return nil
}
Expand Down
3 changes: 1 addition & 2 deletions tempodb/backend/gcs/gcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ func (rw *readerWriter) CloseAppend(_ context.Context, tracker backend.AppendTra
}

func (rw *readerWriter) Delete(ctx context.Context, name string, keypath backend.KeyPath, _ bool) error {
return rw.bucket.Object(backend.ObjectFileName(keypath, name)).
Delete(ctx)
return readError(rw.bucket.Object(backend.ObjectFileName(keypath, name)).Delete(ctx))
}

// List implements backend.Reader
Expand Down
3 changes: 2 additions & 1 deletion tempodb/backend/mocks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type MockRawWriter struct {
appendBuffer []byte
closeAppendCalled bool
deleteCalls map[string]map[string]int
err error
}

func (m *MockRawWriter) Write(_ context.Context, _ string, _ KeyPath, data io.Reader, size int64, _ bool) error {
Expand Down Expand Up @@ -89,7 +90,7 @@ func (m *MockRawWriter) Delete(_ context.Context, name string, keypath KeyPath,
}

m.deleteCalls[name][strings.Join(keypath, "/")]++
return nil
return m.err
}

// MockCompactor
Expand Down
6 changes: 5 additions & 1 deletion tempodb/backend/raw.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ func (w *writer) CloseAppend(ctx context.Context, tracker AppendTracker) error {
func (w *writer) WriteTenantIndex(ctx context.Context, tenantID string, meta []*BlockMeta, compactedMeta []*CompactedBlockMeta) error {
// If meta and compactedMeta are empty, call delete the tenant index.
if len(meta) == 0 && len(compactedMeta) == 0 {
return w.w.Delete(ctx, TenantIndexName, KeyPath([]string{tenantID}), false)
// Skip returning an error when the object is already deleted.
if err := w.w.Delete(ctx, TenantIndexName, KeyPath([]string{tenantID}), false); err != nil && err != ErrDoesNotExist {
joe-elliott marked this conversation as resolved.
Show resolved Hide resolved
return err
}
return nil
}

b := newTenantIndex(meta, compactedMeta)
Expand Down
6 changes: 6 additions & 0 deletions tempodb/backend/raw_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ func TestWriter(t *testing.T) {

expectedDeleteMap := map[string]map[string]int{TenantIndexName: {"test": 1}}
assert.Equal(t, expectedDeleteMap, w.(*writer).w.(*MockRawWriter).deleteCalls)

// When a backend returns ErrDoesNotExist, the tenant index should be deleted, but no error should be returned if the tenant index does not exist
m = &MockRawWriter{err: ErrDoesNotExist}
w = NewWriter(m)
err = w.WriteTenantIndex(ctx, "test", nil, nil)
assert.NoError(t, err)
}

func TestReader(t *testing.T) {
Expand Down
Loading