From f5deb2efdd1da6b925851ec8db36ec4338e001fa Mon Sep 17 00:00:00 2001 From: ilangofman Date: Tue, 13 Jul 2021 14:58:09 -0400 Subject: [PATCH 01/21] Add endpoints for blocks series deletion API Signed-off-by: ilangofman --- pkg/api/api.go | 17 +- pkg/chunk/purger/blocks_purger.go | 237 ++++++++++++++ pkg/chunk/purger/blocks_purger_test.go | 202 ++++++++++++ pkg/chunk/purger/tenant_deletion_api.go | 41 +-- pkg/chunk/purger/tenant_deletion_api_test.go | 4 +- pkg/cortex/modules.go | 15 +- pkg/storage/tsdb/tombstones.go | 321 +++++++++++++++++++ 7 files changed, 787 insertions(+), 50 deletions(-) create mode 100644 pkg/chunk/purger/blocks_purger.go create mode 100644 pkg/chunk/purger/blocks_purger_test.go create mode 100644 pkg/storage/tsdb/tombstones.go diff --git a/pkg/api/api.go b/pkg/api/api.go index 7410906e17..f55038a45c 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -272,9 +272,20 @@ func (a *API) RegisterChunksPurger(store *purger.DeleteStore, deleteRequestCance a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(deleteRequestHandler.CancelDeleteRequestHandler), true, "PUT", "POST") } -func (a *API) RegisterTenantDeletion(api *purger.TenantDeletionAPI) { - a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(api.DeleteTenant), true, "POST") - a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(api.DeleteTenantStatus), true, "GET") +func (a *API) RegisterBlocksPurgerAPI(blocksPurger *purger.BlocksPurgerAPI) { + + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2GetAllDeleteRequestsHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.V2CancelDeleteRequestHandler), true, "PUT", "POST") + + // Legacy Routes + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.V2CancelDeleteRequestHandler), true, "PUT", "POST") + + // Tenant Deletion + a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(blocksPurger.DeleteTenant), true, "POST") + a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(blocksPurger.DeleteTenantStatus), true, "GET") } // RegisterRuler registers routes associated with the Ruler service. diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go new file mode 100644 index 0000000000..4ef4492d65 --- /dev/null +++ b/pkg/chunk/purger/blocks_purger.go @@ -0,0 +1,237 @@ +package purger + +import ( + "context" + "crypto/md5" + "encoding/hex" + "encoding/json" + fmt "fmt" + "net/http" + "strconv" + strings "strings" + "time" + + "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" + "github.com/pkg/errors" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/common/model" + "github.com/prometheus/prometheus/promql/parser" + "github.com/thanos-io/thanos/pkg/objstore" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" + "github.com/cortexproject/cortex/pkg/tenant" + "github.com/cortexproject/cortex/pkg/util" + util_log "github.com/cortexproject/cortex/pkg/util/log" +) + +type BlocksPurgerAPI struct { + bucketClient objstore.Bucket + logger log.Logger + cfgProvider bucket.TenantConfigProvider + deleteRequestCancelPeriod time.Duration +} + +func NewBlocksPurgerAPI(storageCfg cortex_tsdb.BlocksStorageConfig, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer, cancellationPeriod time.Duration) (*BlocksPurgerAPI, error) { + bucketClient, err := createBucketClient(storageCfg, logger, reg) + if err != nil { + return nil, err + } + + return newBlocksPurgerAPI(bucketClient, cfgProvider, logger, cancellationPeriod), nil +} + +func newBlocksPurgerAPI(bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, logger log.Logger, cancellationPeriod time.Duration) *BlocksPurgerAPI { + return &BlocksPurgerAPI{ + bucketClient: bkt, + cfgProvider: cfgProvider, + logger: logger, + deleteRequestCancelPeriod: cancellationPeriod, + } +} + +func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, reg prometheus.Registerer) (objstore.Bucket, error) { + bucketClient, err := bucket.NewClient(context.Background(), cfg.Bucket, "purger", logger, reg) + if err != nil { + return nil, errors.Wrap(err, "create bucket client") + } + + return bucketClient, nil +} + +func (api *BlocksPurgerAPI) V2AddDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { + + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + + params := r.URL.Query() + match := params["match[]"] + if len(match) == 0 { + http.Error(w, "selectors not set", http.StatusBadRequest) + return + } + + for i := range match { + _, err := parser.ParseMetricSelector(match[i]) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + } + + startParam := params.Get("start") + startTime := int64(0) + if startParam != "" { + startTime, err = util.ParseTime(startParam) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + } + + endParam := params.Get("end") + endTime := int64(model.Now()) + + if endParam != "" { + endTime, err = util.ParseTime(endParam) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + if endTime > int64(model.Now()) { + http.Error(w, "deletes in future not allowed", http.StatusBadRequest) + return + } + } + + if startTime > endTime { + http.Error(w, "start time can't be greater than end time", http.StatusBadRequest) + return + } + + requestId := getTombstoneRequestID(startTime, endTime, match) + curTime := time.Now().Unix() + t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestId, cortex_tsdb.StatePending) + + if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { + + if err == cortex_tsdb.ErrTombstoneAlreadyExists { + level.Error(util_log.Logger).Log("msg", "delete request tombstone with same information already exists", "err", err) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + level.Error(util_log.Logger).Log("msg", "error adding delete request to the store", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + w.WriteHeader(http.StatusNoContent) +} + +func (api *BlocksPurgerAPI) V2GetAllDeleteRequestsHandler(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + + deleteRequests, err := cortex_tsdb.GetAllDeleteRequestsForUser(ctx, api.bucketClient, api.cfgProvider, userID) + if err != nil { + level.Error(util_log.Logger).Log("msg", "error getting delete requests from the block store", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + if err := json.NewEncoder(w).Encode(deleteRequests); err != nil { + level.Error(util_log.Logger).Log("msg", "error marshalling response", "err", err) + http.Error(w, fmt.Sprintf("Error marshalling response: %v", err), http.StatusInternalServerError) + } + + w.WriteHeader(http.StatusOK) + +} + +func (api *BlocksPurgerAPI) V2CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { + + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + + params := r.URL.Query() + requestID := params.Get("request_id") + + deleteRequest, err := cortex_tsdb.GetDeleteRequestsForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + if err != nil { + level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + if deleteRequest == nil { + http.Error(w, "could not find delete request with given id", http.StatusBadRequest) + return + } + + if deleteRequest.State == cortex_tsdb.StateCancelled { + http.Error(w, "the request has already been previously deleted", http.StatusBadRequest) + return + } + + if deleteRequest.State == cortex_tsdb.StateProcessed { + http.Error(w, "deletion of request which is already processed is not allowed", http.StatusBadRequest) + return + } + + currentTime := int64(time.Now().Unix() * 1000) + timeElapsed := float64(currentTime - deleteRequest.RequestCreatedAt) + + if timeElapsed > float64(api.deleteRequestCancelPeriod.Milliseconds()) { + http.Error(w, fmt.Sprintf("deletion of request past the deadline of %s since its creation is not allowed", api.deleteRequestCancelPeriod.String()), http.StatusBadRequest) + return + } + + // create file with the cancelled state + _, err = cortex_tsdb.UpgradeTombstoneState(ctx, api.bucketClient, api.cfgProvider, deleteRequest, cortex_tsdb.StateCancelled) + if err != nil { + level.Error(util_log.Logger).Log("msg", "error cancelling the delete request", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + w.WriteHeader(http.StatusNoContent) +} + +// Calculates the tombstone file name based on a hash of the start time, end time and selectors +func getTombstoneRequestID(startTime int64, endTime int64, selectors []string) string { + // First make a string of the tombstone info + + var b strings.Builder + + b.WriteString(strconv.FormatInt(startTime, 10)) + b.WriteString(",") + b.WriteString(strconv.FormatInt(endTime, 10)) + + for _, s := range selectors { + b.WriteString(",") + b.WriteString(s) + } + + return getTombstoneHash(b.String()) +} + +func getTombstoneHash(s string) string { + data := []byte(s) + md5Bytes := md5.Sum(data) + return hex.EncodeToString(md5Bytes[:]) +} diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go new file mode 100644 index 0000000000..74e7df11f4 --- /dev/null +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -0,0 +1,202 @@ +package purger + +import ( + "context" + math "math" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + "time" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" + "github.com/go-kit/kit/log" + "github.com/stretchr/testify/require" + "github.com/thanos-io/thanos/pkg/objstore" + "github.com/weaveworks/common/user" +) + +func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { + for name, tc := range map[string]struct { + parameters url.Values + expectedHttpStatus int + }{ + "empty": { + parameters: nil, + expectedHttpStatus: http.StatusBadRequest, + }, + + "valid request": { + parameters: url.Values{ + "start": []string{"1"}, + "end": []string{"2"}, + "match[]": []string{"selector"}, + }, + expectedHttpStatus: http.StatusNoContent, + }, + + "end time in the future": { + parameters: url.Values{ + "start": []string{"1"}, + "end": []string{strconv.Itoa(math.MaxInt64)}, + "match[]": []string{"selector"}, + }, + expectedHttpStatus: http.StatusBadRequest, + }, + "the start time is after the end time": { + parameters: url.Values{ + "start": []string{"2"}, + "end": []string{"1"}, + "match[]": []string{"selector"}, + }, + expectedHttpStatus: http.StatusBadRequest, + }, + } { + t.Run(name, func(t *testing.T) { + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + u := &url.URL{ + RawQuery: tc.parameters.Encode(), + } + + req := &http.Request{ + Method: "GET", + RequestURI: u.String(), // This is what the httpgrpc code looks at. + URL: u, + Body: http.NoBody, + Header: http.Header{}, + } + + resp := httptest.NewRecorder() + api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + require.Equal(t, tc.expectedHttpStatus, resp.Code) + + }) + } +} + +func TestBlocksDeleteSeries_AddingSameRequestTwiceShouldFail(t *testing.T) { + + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + params := url.Values{ + "start": []string{"1"}, + "end": []string{"2"}, + "match[]": []string{"node_exporter"}, + } + + u := &url.URL{ + RawQuery: params.Encode(), + } + + req := &http.Request{ + Method: "GET", + RequestURI: u.String(), + URL: u, + Body: http.NoBody, + Header: http.Header{}, + } + + resp := httptest.NewRecorder() + api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + + // First request made should be okay + require.Equal(t, http.StatusNoContent, resp.Code) + + //second should not be accepted because the same exact request already exists + resp = httptest.NewRecorder() + api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + + require.Equal(t, http.StatusBadRequest, resp.Code) + +} + +func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { + + for name, tc := range map[string]struct { + createdAt int64 + requestState cortex_tsdb.BlockDeleteRequestState + cancellationPeriod time.Duration + cancelledFileExists bool + expectedHttpStatus int + }{ + "not allowed, grace period has passed": { + createdAt: 0, + requestState: cortex_tsdb.StatePending, + cancellationPeriod: time.Second, + cancelledFileExists: false, + expectedHttpStatus: http.StatusBadRequest, + }, + + "allowed, grace period not over yet": { + createdAt: time.Now().Unix() * 1000, + requestState: cortex_tsdb.StatePending, + cancellationPeriod: time.Hour, + cancelledFileExists: true, + expectedHttpStatus: http.StatusNoContent, + }, + "not allowed, deletion already occurred": { + createdAt: 0, + requestState: cortex_tsdb.StateProcessed, + cancellationPeriod: time.Second, + cancelledFileExists: false, + expectedHttpStatus: http.StatusBadRequest, + }, + "not allowed,request already cancelled": { + createdAt: 0, + requestState: cortex_tsdb.StateCancelled, + cancellationPeriod: time.Second, + cancelledFileExists: true, + expectedHttpStatus: http.StatusBadRequest, + }, + } { + t.Run(name, func(t *testing.T) { + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + //create the tombstone + tombstone := cortex_tsdb.NewTombstone("fake", tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) + err := cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, "fake", api.cfgProvider, tombstone) + require.NoError(t, err) + + params := url.Values{ + "request_id": []string{"request_id"}, + } + + u := &url.URL{ + RawQuery: params.Encode(), + } + + req := &http.Request{ + Method: "POST", + RequestURI: u.String(), // This is what the httpgrpc code looks at. + URL: u, + Body: http.NoBody, + Header: http.Header{}, + } + + resp := httptest.NewRecorder() + api.V2CancelDeleteRequestHandler(resp, req.WithContext(ctx)) + require.Equal(t, tc.expectedHttpStatus, resp.Code) + + // check if the cancelled tombstone file exists + userBkt := bucket.NewUserBucketClient("fake", bkt, api.cfgProvider) + exists, _ := cortex_tsdb.TombstoneExists(ctx, userBkt, "fake", "request_id", cortex_tsdb.StateCancelled) + require.Equal(t, tc.cancelledFileExists, exists) + + }) + } +} diff --git a/pkg/chunk/purger/tenant_deletion_api.go b/pkg/chunk/purger/tenant_deletion_api.go index 0babd0abbf..72c3a81a1a 100644 --- a/pkg/chunk/purger/tenant_deletion_api.go +++ b/pkg/chunk/purger/tenant_deletion_api.go @@ -6,12 +6,9 @@ import ( "strings" "time" - "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/oklog/ulid" "github.com/pkg/errors" - "github.com/prometheus/client_golang/prometheus" - "github.com/thanos-io/thanos/pkg/objstore" "github.com/cortexproject/cortex/pkg/storage/bucket" cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" @@ -19,30 +16,7 @@ import ( "github.com/cortexproject/cortex/pkg/util" ) -type TenantDeletionAPI struct { - bucketClient objstore.Bucket - logger log.Logger - cfgProvider bucket.TenantConfigProvider -} - -func NewTenantDeletionAPI(storageCfg cortex_tsdb.BlocksStorageConfig, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer) (*TenantDeletionAPI, error) { - bucketClient, err := createBucketClient(storageCfg, logger, reg) - if err != nil { - return nil, err - } - - return newTenantDeletionAPI(bucketClient, cfgProvider, logger), nil -} - -func newTenantDeletionAPI(bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, logger log.Logger) *TenantDeletionAPI { - return &TenantDeletionAPI{ - bucketClient: bkt, - cfgProvider: cfgProvider, - logger: logger, - } -} - -func (api *TenantDeletionAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { +func (api *BlocksPurgerAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { @@ -70,7 +44,7 @@ type DeleteTenantStatusResponse struct { BlocksDeleted bool `json:"blocks_deleted"` } -func (api *TenantDeletionAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { +func (api *BlocksPurgerAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { @@ -89,7 +63,7 @@ func (api *TenantDeletionAPI) DeleteTenantStatus(w http.ResponseWriter, r *http. util.WriteJSONResponse(w, result) } -func (api *TenantDeletionAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { +func (api *BlocksPurgerAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { var errBlockFound = errors.New("block found") userBucket := bucket.NewUserBucketClient(userID, api.bucketClient, api.cfgProvider) @@ -117,12 +91,3 @@ func (api *TenantDeletionAPI) isBlocksForUserDeleted(ctx context.Context, userID // No blocks found, all good. return true, nil } - -func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, reg prometheus.Registerer) (objstore.Bucket, error) { - bucketClient, err := bucket.NewClient(context.Background(), cfg.Bucket, "purger", logger, reg) - if err != nil { - return nil, errors.Wrap(err, "create bucket client") - } - - return bucketClient, nil -} diff --git a/pkg/chunk/purger/tenant_deletion_api_test.go b/pkg/chunk/purger/tenant_deletion_api_test.go index ae8686c171..3eba759c20 100644 --- a/pkg/chunk/purger/tenant_deletion_api_test.go +++ b/pkg/chunk/purger/tenant_deletion_api_test.go @@ -18,7 +18,7 @@ import ( func TestDeleteTenant(t *testing.T) { bkt := objstore.NewInMemBucket() - api := newTenantDeletionAPI(bkt, nil, log.NewNopLogger()) + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) { resp := httptest.NewRecorder() @@ -80,7 +80,7 @@ func TestDeleteTenantStatus(t *testing.T) { require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) } - api := newTenantDeletionAPI(bkt, nil, log.NewNopLogger()) + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) res, err := api.isBlocksForUserDeleted(context.Background(), username) require.NoError(t, err) diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index 5c36059e2e..9cca773810 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -79,6 +79,7 @@ const ( StoreGateway string = "store-gateway" MemberlistKV string = "memberlist-kv" ChunksPurger string = "chunks-purger" + BlocksPurger string = "blocks-purger" TenantDeletion string = "tenant-deletion" Purger string = "purger" QueryScheduler string = "query-scheduler" @@ -800,18 +801,18 @@ func (t *Cortex) initChunksPurger() (services.Service, error) { return t.Purger, nil } -func (t *Cortex) initTenantDeletionAPI() (services.Service, error) { - if t.Cfg.Storage.Engine != storage.StorageEngineBlocks { +func (t *Cortex) initBlocksPurger() (services.Service, error) { + if t.Cfg.Storage.Engine != storage.StorageEngineBlocks || !t.Cfg.PurgerConfig.Enable { return nil, nil } // t.RulerStorage can be nil when running in single-binary mode, and rule storage is not configured. - tenantDeletionAPI, err := purger.NewTenantDeletionAPI(t.Cfg.BlocksStorage, t.Overrides, util_log.Logger, prometheus.DefaultRegisterer) + blockPurger, err := purger.NewBlocksPurgerAPI(t.Cfg.BlocksStorage, t.Overrides, util_log.Logger, prometheus.DefaultRegisterer, t.Cfg.PurgerConfig.DeleteRequestCancelPeriod) if err != nil { return nil, err } - t.API.RegisterTenantDeletion(tenantDeletionAPI) + t.API.RegisterBlocksPurgerAPI(blockPurger) return nil, nil } @@ -857,7 +858,7 @@ func (t *Cortex) setupModuleManager() error { mm.RegisterModule(Compactor, t.initCompactor) mm.RegisterModule(StoreGateway, t.initStoreGateway) mm.RegisterModule(ChunksPurger, t.initChunksPurger, modules.UserInvisibleModule) - mm.RegisterModule(TenantDeletion, t.initTenantDeletionAPI, modules.UserInvisibleModule) + mm.RegisterModule(BlocksPurger, t.initBlocksPurger, modules.UserInvisibleModule) mm.RegisterModule(Purger, nil) mm.RegisterModule(QueryScheduler, t.initQueryScheduler) mm.RegisterModule(TenantFederation, t.initTenantFederation, modules.UserInvisibleModule) @@ -891,8 +892,8 @@ func (t *Cortex) setupModuleManager() error { Compactor: {API, MemberlistKV, Overrides}, StoreGateway: {API, Overrides, MemberlistKV}, ChunksPurger: {Store, DeleteRequestsStore, API}, - TenantDeletion: {Store, API, Overrides}, - Purger: {ChunksPurger, TenantDeletion}, + BlocksPurger: {Store, API, Overrides}, + Purger: {ChunksPurger, BlocksPurger}, TenantFederation: {Queryable}, All: {QueryFrontend, Querier, Ingester, Distributor, TableManager, Purger, StoreGateway, Ruler}, } diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go new file mode 100644 index 0000000000..0730c85359 --- /dev/null +++ b/pkg/storage/tsdb/tombstones.go @@ -0,0 +1,321 @@ +package tsdb + +import ( + "bytes" + "context" + "encoding/json" + "path" + "path/filepath" + "time" + + "github.com/go-kit/kit/log/level" + "github.com/pkg/errors" + "github.com/prometheus/prometheus/pkg/labels" + "github.com/prometheus/prometheus/promql/parser" + "github.com/thanos-io/thanos/pkg/objstore" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + util_log "github.com/cortexproject/cortex/pkg/util/log" +) + +type BlockDeleteRequestState string + +const ( + StatePending BlockDeleteRequestState = "pending" + StateProcessed BlockDeleteRequestState = "processed" + StateCancelled BlockDeleteRequestState = "deleted" +) + +// Relative to user-specific prefix. +const TombstonePath = "tombstones/" + +var ( + ErrTombstoneAlreadyExists = errors.New("the deletion tombstone with the same request information already exists") + ErrInvalidDeletionRequestState = errors.New("Deletion request filename extension indicating the state is invalid") + + PossibleDeletionState = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} +) + +type Tombstone struct { + RequestCreatedAt int64 `json:"request_created_at"` + StateCreatedAt int64 `json:"state_created_at"` + RequestID string `json:"request_id"` + StartTime int64 `json:"start_time"` + EndTime int64 `json:"end_time"` + Selectors []string `json:"selectors"` + Matchers []*labels.Matcher `json:"-"` + UserID string `json:"user_id"` + State BlockDeleteRequestState `json:"-"` +} + +func NewTombstone(userID string, requestTime int64, stateTime int64, startTime int64, endTime int64, selectors []string, requestId string, state BlockDeleteRequestState) *Tombstone { + return &Tombstone{ + RequestCreatedAt: requestTime, + StateCreatedAt: stateTime, + StartTime: startTime, + EndTime: endTime, + Selectors: selectors, + UserID: userID, + RequestID: requestId, + State: state, + } +} + +// Uploads a tombstone file to object sotre +func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, tombstone *Tombstone) error { + userBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) + + data, err := json.Marshal(tombstone) + if err != nil { + return errors.Wrap(err, "serialize tombstone") + } + + // Add the state of the request using the filename extension + filename := tombstone.RequestID + ".json." + string(tombstone.State) + + fullTombstonePath := path.Join(TombstonePath, filename) + + // Check if the tombstone already exists. Could be the case the same request was made + // and is already in the middle of deleting series. Creating a new tombstone would restart + // the progress + tombstoneExists, err := TombstoneExists(ctx, userBkt, userID, tombstone.RequestID, tombstone.State) + if err != nil { + level.Warn(util_log.Logger).Log("msg", "unable to check if the same tombstone already exists", "user", userID, "requestID", tombstone.RequestID, "err", err) + } else if tombstoneExists { + return ErrTombstoneAlreadyExists + } + + return errors.Wrap(userBkt.Upload(ctx, fullTombstonePath, bytes.NewReader(data)), "upload tombstone file") +} + +func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID string, requestID string, state BlockDeleteRequestState) (bool, error) { + // Add the state of the request using the filename extension + filename := requestID + ".json." + string(state) + fullTombstonePath := path.Join(TombstonePath, filename) + + exists, err := bkt.Exists(ctx, fullTombstonePath) + + if exists || err != nil { + return exists, err + } + + return false, nil +} + +func GetDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { + userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) + + var deleteRequest *Tombstone = nil + + for _, state := range PossibleDeletionState { + filename := requestId + ".json." + string(state) + + exists, err := TombstoneExists(ctx, userBucket, userID, requestId, state) + if err != nil { + return nil, err + } + if !exists { + continue + } + + t, err := readTombstoneFile(ctx, userBucket, userID, path.Join(TombstonePath, filename)) + if err != nil { + return nil, err + } + + //checking if there exists more than one file for the same tombstone + // If it is the case, then should remove the one with the older state + if deleteRequest == nil { + deleteRequest = t + } else { + deleteRequest, err = removeDuplicateTombstone(ctx, bkt, cfgProvider, t.UserID, t, deleteRequest) + if err != nil { + return nil, err + } + } + } + + return deleteRequest, nil +} + +func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string) ([]*Tombstone, error) { + userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) + + // add all the tombstones to a map and check for duplicates, + // if a key exists with the same request ID (but two different states) + tombstoneMap := make(map[string]*Tombstone) + err := userBucket.Iter(ctx, "tombstones/", func(s string) error { + t, err := readTombstoneFile(ctx, userBucket, userID, s) + if err != nil { + return err + } + + if _, exists := tombstoneMap[t.RequestID]; !exists { + tombstoneMap[t.RequestID] = t + } else { + newT, err := removeDuplicateTombstone(ctx, bkt, cfgProvider, t.UserID, t, tombstoneMap[t.RequestID]) + if err != nil { + return err + } + tombstoneMap[t.RequestID] = newT + } + return nil + }) + + if err != nil { + return nil, err + } + + deletionRequests := []*Tombstone{} + for _, t := range tombstoneMap { + deletionRequests = append(deletionRequests, t) + } + + return deletionRequests, nil +} + +func removeDuplicateTombstone(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, tombstoneA *Tombstone, tombstoneB *Tombstone) (*Tombstone, error) { + userLogger := util_log.WithUserID(userID, util_log.Logger) + + if tombstoneA.RequestID != tombstoneB.RequestID { + return nil, errors.New("Cannot run the remove duplicate function for 2 tombstones with different request ID's") + } else if tombstoneA.State == tombstoneB.State { + return tombstoneA, nil + } + + level.Info(userLogger).Log("msg", "Found multiple tombstones for the same deletion request ID but with different state'", "request ID", tombstoneA.RequestID) + + //find which tombstone should be removed + var toDelete, toKeep *Tombstone + + orderA, err := tombstoneA.GetStateOrder() + if err != nil { + return nil, err + } + + orderB, err := tombstoneB.GetStateOrder() + if err != nil { + return nil, err + } + + if orderA > orderB { + toKeep = tombstoneA + toDelete = tombstoneB + } else { + toKeep = tombstoneB + toDelete = tombstoneA + } + + level.Info(userLogger).Log("msg", "For deletion request %s", toDelete.RequestID, ", removing %s", toDelete.State, "state file, while keeping .", string(toKeep.State), "state file") + err = DeleteTombstoneFile(ctx, bkt, cfgProvider, toDelete) + + if err != nil { + return nil, errors.Wrap(err, "failed to delete duplicate tombstone") + } + + return toKeep, nil +} + +func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID string, tombstonePath string) (*Tombstone, error) { + filenameExtesion := filepath.Ext(tombstonePath) + + // Ensure that the state exists as the filename extension + if len(filenameExtesion) == 0 { + return nil, ErrInvalidDeletionRequestState + } + + state := BlockDeleteRequestState(filenameExtesion[1:]) + + if !isValidDeleteRequestState(state) { + return nil, errors.Wrapf(ErrInvalidDeletionRequestState, "Filename extension is invalid for tombstone: %s", tombstonePath) + + } + + r, err := bkt.Get(ctx, tombstonePath) + if err != nil { + return nil, errors.Wrapf(err, "failed to read tombstone object: %s", tombstonePath) + } + + tombstone := &Tombstone{} + err = json.NewDecoder(r).Decode(tombstone) + + // Close reader before dealing with decode error. + if closeErr := r.Close(); closeErr != nil { + level.Warn(util_log.Logger).Log("msg", "failed to close bucket reader", "err", closeErr) + } + + if err != nil { + return nil, errors.Wrapf(err, "failed to decode tombstone object: %s", tombstonePath) + } + + tombstone.State = BlockDeleteRequestState(state) + + // Convert the string selectors to label matchers + var parsedMatcher []*labels.Matcher + + for _, selector := range tombstone.Selectors { + parsedMatcher, err = parser.ParseMetricSelector(selector) + if err != nil { + return nil, errors.Wrapf(err, "error parsing metric selector") + } + tombstone.Matchers = append(tombstone.Matchers, parsedMatcher...) + } + + return tombstone, nil +} + +func UpgradeTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { + // Create the new tombstone, and will delete the previous tombstone + newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix(), t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) + + err := WriteTombstoneFile(ctx, bkt, newT.UserID, cfgProvider, newT) + if err != nil { + level.Error(util_log.Logger).Log("msg", "error creating file tombstone file with the updated state", "err", err) + return nil, err + } + + err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t) + if err != nil { + level.Error(util_log.Logger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) + } + + return newT, nil +} + +func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone) error { + userLogger := util_log.WithUserID(t.UserID, util_log.Logger) + userBucket := bucket.NewUserBucketClient(t.UserID, bkt, cfgProvider) + + // Create the full path of the tombstone by appending the state as the extension + filename := t.RequestID + ".json." + string(t.State) + fullTombstonePath := path.Join(TombstonePath, filename) + + level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) + + return errors.Wrap(userBucket.Delete(ctx, fullTombstonePath), "delete tombstone file") + +} + +func isValidDeleteRequestState(state BlockDeleteRequestState) bool { + switch state { + case + StatePending, + StateProcessed, + StateCancelled: + return true + } + return false +} + +func (t *Tombstone) GetStateOrder() (int, error) { + switch t.State { + case StatePending: + return 0, nil + case StateProcessed: + return 2, nil + case StateCancelled: + return 3, nil + } + + return -1, ErrInvalidDeletionRequestState +} From 220a47e357a22fdd13c39edcb6869a85a3845af1 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Wed, 14 Jul 2021 18:17:06 -0400 Subject: [PATCH 02/21] Add unit tests and address PR comments Signed-off-by: ilangofman --- pkg/api/api.go | 14 +- pkg/chunk/purger/blocks_purger.go | 91 ++++- pkg/chunk/purger/blocks_purger_test.go | 84 ++++- pkg/chunk/purger/tenant_deletion_api.go | 93 ------ pkg/chunk/purger/tenant_deletion_api_test.go | 90 ----- pkg/cortex/modules.go | 2 +- pkg/storage/tsdb/tombstones.go | 107 +++--- pkg/storage/tsdb/tombstones_test.go | 334 +++++++++++++++++++ 8 files changed, 569 insertions(+), 246 deletions(-) delete mode 100644 pkg/chunk/purger/tenant_deletion_api.go delete mode 100644 pkg/chunk/purger/tenant_deletion_api_test.go create mode 100644 pkg/storage/tsdb/tombstones_test.go diff --git a/pkg/api/api.go b/pkg/api/api.go index f55038a45c..2029f16f4a 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -272,16 +272,16 @@ func (a *API) RegisterChunksPurger(store *purger.DeleteStore, deleteRequestCance a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(deleteRequestHandler.CancelDeleteRequestHandler), true, "PUT", "POST") } -func (a *API) RegisterBlocksPurgerAPI(blocksPurger *purger.BlocksPurgerAPI) { +func (a *API) RegisterBlocksPurger(blocksPurger *purger.BlocksPurgerAPI) { - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2GetAllDeleteRequestsHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.V2CancelDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.GetAllDeleteRequestsHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") // Legacy Routes - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.V2CancelDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") // Tenant Deletion a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(blocksPurger.DeleteTenant), true, "POST") diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 4ef4492d65..97833a63ec 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -13,6 +13,7 @@ import ( "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" + "github.com/oklog/ulid" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" @@ -60,7 +61,7 @@ func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, return bucketClient, nil } -func (api *BlocksPurgerAPI) V2AddDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { +func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) @@ -116,7 +117,7 @@ func (api *BlocksPurgerAPI) V2AddDeleteRequestHandler(w http.ResponseWriter, r * } requestId := getTombstoneRequestID(startTime, endTime, match) - curTime := time.Now().Unix() + curTime := time.Now().Unix() * 1000 t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestId, cortex_tsdb.StatePending) if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { @@ -135,7 +136,7 @@ func (api *BlocksPurgerAPI) V2AddDeleteRequestHandler(w http.ResponseWriter, r * w.WriteHeader(http.StatusNoContent) } -func (api *BlocksPurgerAPI) V2GetAllDeleteRequestsHandler(w http.ResponseWriter, r *http.Request) { +func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { @@ -159,7 +160,7 @@ func (api *BlocksPurgerAPI) V2GetAllDeleteRequestsHandler(w http.ResponseWriter, } -func (api *BlocksPurgerAPI) V2CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { +func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) @@ -171,7 +172,7 @@ func (api *BlocksPurgerAPI) V2CancelDeleteRequestHandler(w http.ResponseWriter, params := r.URL.Query() requestID := params.Get("request_id") - deleteRequest, err := cortex_tsdb.GetDeleteRequestsForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + deleteRequest, err := cortex_tsdb.GetDeleteRequestById(ctx, api.bucketClient, api.cfgProvider, userID, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -202,7 +203,7 @@ func (api *BlocksPurgerAPI) V2CancelDeleteRequestHandler(w http.ResponseWriter, } // create file with the cancelled state - _, err = cortex_tsdb.UpgradeTombstoneState(ctx, api.bucketClient, api.cfgProvider, deleteRequest, cortex_tsdb.StateCancelled) + _, err = cortex_tsdb.UpdateTombstoneState(ctx, api.bucketClient, api.cfgProvider, deleteRequest, cortex_tsdb.StateCancelled) if err != nil { level.Error(util_log.Logger).Log("msg", "error cancelling the delete request", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -215,9 +216,7 @@ func (api *BlocksPurgerAPI) V2CancelDeleteRequestHandler(w http.ResponseWriter, // Calculates the tombstone file name based on a hash of the start time, end time and selectors func getTombstoneRequestID(startTime int64, endTime int64, selectors []string) string { // First make a string of the tombstone info - var b strings.Builder - b.WriteString(strconv.FormatInt(startTime, 10)) b.WriteString(",") b.WriteString(strconv.FormatInt(endTime, 10)) @@ -235,3 +234,79 @@ func getTombstoneHash(s string) string { md5Bytes := md5.Sum(data) return hex.EncodeToString(md5Bytes[:]) } + +func (api *BlocksPurgerAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + // When Cortex is running, it uses Auth Middleware for checking X-Scope-OrgID and injecting tenant into context. + // Auth Middleware sends http.StatusUnauthorized if X-Scope-OrgID is missing, so we do too here, for consistency. + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + + err = cortex_tsdb.WriteTenantDeletionMark(r.Context(), api.bucketClient, userID, api.cfgProvider, cortex_tsdb.NewTenantDeletionMark(time.Now())) + if err != nil { + level.Error(api.logger).Log("msg", "failed to write tenant deletion mark", "user", userID, "err", err) + + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + level.Info(api.logger).Log("msg", "tenant deletion mark in blocks storage created", "user", userID) + + w.WriteHeader(http.StatusOK) +} + +type DeleteTenantStatusResponse struct { + TenantID string `json:"tenant_id"` + BlocksDeleted bool `json:"blocks_deleted"` +} + +func (api *BlocksPurgerAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + result := DeleteTenantStatusResponse{} + result.TenantID = userID + result.BlocksDeleted, err = api.isBlocksForUserDeleted(ctx, userID) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + util.WriteJSONResponse(w, result) +} + +func (api *BlocksPurgerAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { + var errBlockFound = errors.New("block found") + + userBucket := bucket.NewUserBucketClient(userID, api.bucketClient, api.cfgProvider) + err := userBucket.Iter(ctx, "", func(s string) error { + s = strings.TrimSuffix(s, "/") + + _, err := ulid.Parse(s) + if err != nil { + // not block, keep looking + return nil + } + + // Used as shortcut to stop iteration. + return errBlockFound + }) + + if errors.Is(err, errBlockFound) { + return false, nil + } + + if err != nil { + return false, err + } + + // No blocks found, all good. + return true, nil +} diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 74e7df11f4..581bfa1655 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -1,16 +1,19 @@ package purger import ( + "bytes" "context" math "math" "net/http" "net/http/httptest" "net/url" + "path" "strconv" "testing" "time" "github.com/cortexproject/cortex/pkg/storage/bucket" + "github.com/cortexproject/cortex/pkg/storage/tsdb" cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" "github.com/go-kit/kit/log" "github.com/stretchr/testify/require" @@ -74,7 +77,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { } resp := httptest.NewRecorder() - api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) require.Equal(t, tc.expectedHttpStatus, resp.Code) }) @@ -108,14 +111,14 @@ func TestBlocksDeleteSeries_AddingSameRequestTwiceShouldFail(t *testing.T) { } resp := httptest.NewRecorder() - api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) // First request made should be okay require.Equal(t, http.StatusNoContent, resp.Code) //second should not be accepted because the same exact request already exists resp = httptest.NewRecorder() - api.V2AddDeleteRequestHandler(resp, req.WithContext(ctx)) + api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) require.Equal(t, http.StatusBadRequest, resp.Code) @@ -189,7 +192,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { } resp := httptest.NewRecorder() - api.V2CancelDeleteRequestHandler(resp, req.WithContext(ctx)) + api.CancelDeleteRequestHandler(resp, req.WithContext(ctx)) require.Equal(t, tc.expectedHttpStatus, resp.Code) // check if the cancelled tombstone file exists @@ -200,3 +203,76 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { }) } } + +func TestDeleteTenant(t *testing.T) { + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + { + resp := httptest.NewRecorder() + api.DeleteTenant(resp, &http.Request{}) + require.Equal(t, http.StatusUnauthorized, resp.Code) + } + + { + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + req := &http.Request{} + resp := httptest.NewRecorder() + api.DeleteTenant(resp, req.WithContext(ctx)) + + require.Equal(t, http.StatusOK, resp.Code) + objs := bkt.Objects() + require.NotNil(t, objs[path.Join("fake", tsdb.TenantDeletionMarkPath)]) + } +} + +func TestDeleteTenantStatus(t *testing.T) { + const username = "user" + + for name, tc := range map[string]struct { + objects map[string][]byte + expectedBlocksDeleted bool + }{ + "empty": { + objects: nil, + expectedBlocksDeleted: true, + }, + + "no user objects": { + objects: map[string][]byte{ + "different-user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), + }, + expectedBlocksDeleted: true, + }, + + "non-block files": { + objects: map[string][]byte{ + "user/deletion-mark.json": []byte("data"), + }, + expectedBlocksDeleted: true, + }, + + "block files": { + objects: map[string][]byte{ + "user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), + }, + expectedBlocksDeleted: false, + }, + } { + t.Run(name, func(t *testing.T) { + bkt := objstore.NewInMemBucket() + // "upload" objects + for objName, data := range tc.objects { + require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) + } + + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + res, err := api.isBlocksForUserDeleted(context.Background(), username) + require.NoError(t, err) + require.Equal(t, tc.expectedBlocksDeleted, res) + }) + } +} diff --git a/pkg/chunk/purger/tenant_deletion_api.go b/pkg/chunk/purger/tenant_deletion_api.go deleted file mode 100644 index 72c3a81a1a..0000000000 --- a/pkg/chunk/purger/tenant_deletion_api.go +++ /dev/null @@ -1,93 +0,0 @@ -package purger - -import ( - "context" - "net/http" - "strings" - "time" - - "github.com/go-kit/kit/log/level" - "github.com/oklog/ulid" - "github.com/pkg/errors" - - "github.com/cortexproject/cortex/pkg/storage/bucket" - cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" - "github.com/cortexproject/cortex/pkg/tenant" - "github.com/cortexproject/cortex/pkg/util" -) - -func (api *BlocksPurgerAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - userID, err := tenant.TenantID(ctx) - if err != nil { - // When Cortex is running, it uses Auth Middleware for checking X-Scope-OrgID and injecting tenant into context. - // Auth Middleware sends http.StatusUnauthorized if X-Scope-OrgID is missing, so we do too here, for consistency. - http.Error(w, err.Error(), http.StatusUnauthorized) - return - } - - err = cortex_tsdb.WriteTenantDeletionMark(r.Context(), api.bucketClient, userID, api.cfgProvider, cortex_tsdb.NewTenantDeletionMark(time.Now())) - if err != nil { - level.Error(api.logger).Log("msg", "failed to write tenant deletion mark", "user", userID, "err", err) - - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - level.Info(api.logger).Log("msg", "tenant deletion mark in blocks storage created", "user", userID) - - w.WriteHeader(http.StatusOK) -} - -type DeleteTenantStatusResponse struct { - TenantID string `json:"tenant_id"` - BlocksDeleted bool `json:"blocks_deleted"` -} - -func (api *BlocksPurgerAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - userID, err := tenant.TenantID(ctx) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - - result := DeleteTenantStatusResponse{} - result.TenantID = userID - result.BlocksDeleted, err = api.isBlocksForUserDeleted(ctx, userID) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - util.WriteJSONResponse(w, result) -} - -func (api *BlocksPurgerAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { - var errBlockFound = errors.New("block found") - - userBucket := bucket.NewUserBucketClient(userID, api.bucketClient, api.cfgProvider) - err := userBucket.Iter(ctx, "", func(s string) error { - s = strings.TrimSuffix(s, "/") - - _, err := ulid.Parse(s) - if err != nil { - // not block, keep looking - return nil - } - - // Used as shortcut to stop iteration. - return errBlockFound - }) - - if errors.Is(err, errBlockFound) { - return false, nil - } - - if err != nil { - return false, err - } - - // No blocks found, all good. - return true, nil -} diff --git a/pkg/chunk/purger/tenant_deletion_api_test.go b/pkg/chunk/purger/tenant_deletion_api_test.go deleted file mode 100644 index 3eba759c20..0000000000 --- a/pkg/chunk/purger/tenant_deletion_api_test.go +++ /dev/null @@ -1,90 +0,0 @@ -package purger - -import ( - "bytes" - "context" - "net/http" - "net/http/httptest" - "path" - "testing" - - "github.com/go-kit/kit/log" - "github.com/stretchr/testify/require" - "github.com/thanos-io/thanos/pkg/objstore" - "github.com/weaveworks/common/user" - - "github.com/cortexproject/cortex/pkg/storage/tsdb" -) - -func TestDeleteTenant(t *testing.T) { - bkt := objstore.NewInMemBucket() - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) - - { - resp := httptest.NewRecorder() - api.DeleteTenant(resp, &http.Request{}) - require.Equal(t, http.StatusUnauthorized, resp.Code) - } - - { - ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") - - req := &http.Request{} - resp := httptest.NewRecorder() - api.DeleteTenant(resp, req.WithContext(ctx)) - - require.Equal(t, http.StatusOK, resp.Code) - objs := bkt.Objects() - require.NotNil(t, objs[path.Join("fake", tsdb.TenantDeletionMarkPath)]) - } -} - -func TestDeleteTenantStatus(t *testing.T) { - const username = "user" - - for name, tc := range map[string]struct { - objects map[string][]byte - expectedBlocksDeleted bool - }{ - "empty": { - objects: nil, - expectedBlocksDeleted: true, - }, - - "no user objects": { - objects: map[string][]byte{ - "different-user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), - }, - expectedBlocksDeleted: true, - }, - - "non-block files": { - objects: map[string][]byte{ - "user/deletion-mark.json": []byte("data"), - }, - expectedBlocksDeleted: true, - }, - - "block files": { - objects: map[string][]byte{ - "user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), - }, - expectedBlocksDeleted: false, - }, - } { - t.Run(name, func(t *testing.T) { - bkt := objstore.NewInMemBucket() - // "upload" objects - for objName, data := range tc.objects { - require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) - } - - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) - - res, err := api.isBlocksForUserDeleted(context.Background(), username) - require.NoError(t, err) - require.Equal(t, tc.expectedBlocksDeleted, res) - }) - } -} diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index 9cca773810..31c64123f0 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -812,7 +812,7 @@ func (t *Cortex) initBlocksPurger() (services.Service, error) { return nil, err } - t.API.RegisterBlocksPurgerAPI(blockPurger) + t.API.RegisterBlocksPurger(blockPurger) return nil, nil } diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 0730c85359..081b33d948 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -30,10 +30,10 @@ const ( const TombstonePath = "tombstones/" var ( - ErrTombstoneAlreadyExists = errors.New("the deletion tombstone with the same request information already exists") + ErrTombstoneAlreadyExists = errors.New("The deletion tombstone with the same request information already exists") ErrInvalidDeletionRequestState = errors.New("Deletion request filename extension indicating the state is invalid") - PossibleDeletionState = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} + AllDeletionStates = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} ) type Tombstone struct { @@ -63,6 +63,7 @@ func NewTombstone(userID string, requestTime int64, stateTime int64, startTime i // Uploads a tombstone file to object sotre func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, tombstone *Tombstone) error { + userLogger := util_log.WithUserID(userID, util_log.Logger) userBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) data, err := json.Marshal(tombstone) @@ -71,7 +72,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, } // Add the state of the request using the filename extension - filename := tombstone.RequestID + ".json." + string(tombstone.State) + filename := tombstone.RequestID + "." + string(tombstone.State) + ".json" fullTombstonePath := path.Join(TombstonePath, filename) @@ -80,7 +81,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, // the progress tombstoneExists, err := TombstoneExists(ctx, userBkt, userID, tombstone.RequestID, tombstone.State) if err != nil { - level.Warn(util_log.Logger).Log("msg", "unable to check if the same tombstone already exists", "user", userID, "requestID", tombstone.RequestID, "err", err) + level.Error(userLogger).Log("msg", "unable to check if the same tombstone already exists", "requestID", tombstone.RequestID, "err", err) } else if tombstoneExists { return ErrTombstoneAlreadyExists } @@ -90,7 +91,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID string, requestID string, state BlockDeleteRequestState) (bool, error) { // Add the state of the request using the filename extension - filename := requestID + ".json." + string(state) + filename := requestID + "." + string(state) + ".json" fullTombstonePath := path.Join(TombstonePath, filename) exists, err := bkt.Exists(ctx, fullTombstonePath) @@ -102,40 +103,43 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } -func GetDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { +func GetDeleteRequestById(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { + userLogger := util_log.WithUserID(userID, util_log.Logger) userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - var deleteRequest *Tombstone = nil + found := []*Tombstone{} - for _, state := range PossibleDeletionState { - filename := requestId + ".json." + string(state) + for _, state := range AllDeletionStates { + filename := requestId + "." + string(state) + ".json" exists, err := TombstoneExists(ctx, userBucket, userID, requestId, state) if err != nil { return nil, err } - if !exists { - continue - } - - t, err := readTombstoneFile(ctx, userBucket, userID, path.Join(TombstonePath, filename)) - if err != nil { - return nil, err - } - //checking if there exists more than one file for the same tombstone - // If it is the case, then should remove the one with the older state - if deleteRequest == nil { - deleteRequest = t - } else { - deleteRequest, err = removeDuplicateTombstone(ctx, bkt, cfgProvider, t.UserID, t, deleteRequest) + if exists { + t, err := readTombstoneFile(ctx, userBucket, userID, path.Join(TombstonePath, filename)) if err != nil { return nil, err } + found = append(found, t) } } - return deleteRequest, nil + if len(found) == 0 { + return nil, nil + } + + // If there are multiple tombstones with the same request id but different state, should delete the files with the lower state + for _, ts := range found[0 : len(found)-1] { + level.Info(userLogger).Log("msg", "Found extra tombstone file with outdated state. Will delete it.", "requestID", ts.RequestID, "state", ts.State) + if err := DeleteTombstoneFile(ctx, bkt, cfgProvider, ts); err != nil { + level.Error(userLogger).Log("msg", "Unable to delete redundant tombstone file.", "request_id", ts.RequestID, "state", ts.State, "err", err) + } + } + + return found[len(found)-1], nil + } func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string) ([]*Tombstone, error) { @@ -207,9 +211,7 @@ func removeDuplicateTombstone(ctx context.Context, bkt objstore.Bucket, cfgProvi } level.Info(userLogger).Log("msg", "For deletion request %s", toDelete.RequestID, ", removing %s", toDelete.State, "state file, while keeping .", string(toKeep.State), "state file") - err = DeleteTombstoneFile(ctx, bkt, cfgProvider, toDelete) - - if err != nil { + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, toDelete); err != nil { return nil, errors.Wrap(err, "failed to delete duplicate tombstone") } @@ -217,15 +219,22 @@ func removeDuplicateTombstone(ctx context.Context, bkt objstore.Bucket, cfgProvi } func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID string, tombstonePath string) (*Tombstone, error) { + userLogger := util_log.WithUserID(userID, util_log.Logger) + + // request filename is in format of request_id + "." + state + ".json" + + // This should get the first extension which is .json filenameExtesion := filepath.Ext(tombstonePath) + filenameWithoutJSON := tombstonePath[0 : len(tombstonePath)-len(filenameExtesion)] + + stateExtension := filepath.Ext(filenameWithoutJSON) // Ensure that the state exists as the filename extension - if len(filenameExtesion) == 0 { + if len(stateExtension) == 0 { return nil, ErrInvalidDeletionRequestState } - state := BlockDeleteRequestState(filenameExtesion[1:]) - + state := BlockDeleteRequestState(stateExtension[1:]) if !isValidDeleteRequestState(state) { return nil, errors.Wrapf(ErrInvalidDeletionRequestState, "Filename extension is invalid for tombstone: %s", tombstonePath) @@ -241,7 +250,7 @@ func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID st // Close reader before dealing with decode error. if closeErr := r.Close(); closeErr != nil { - level.Warn(util_log.Logger).Log("msg", "failed to close bucket reader", "err", closeErr) + level.Warn(userLogger).Log("msg", "failed to close bucket reader", "err", closeErr) } if err != nil { @@ -250,33 +259,45 @@ func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID st tombstone.State = BlockDeleteRequestState(state) + tombstone.Matchers, err = parseMatchers(tombstone.Selectors) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse tombstone selectors for: %s", tombstonePath) + } + + return tombstone, nil +} + +func parseMatchers(selectors []string) ([]*labels.Matcher, error) { // Convert the string selectors to label matchers - var parsedMatcher []*labels.Matcher + var m []*labels.Matcher - for _, selector := range tombstone.Selectors { - parsedMatcher, err = parser.ParseMetricSelector(selector) + for _, selector := range selectors { + parsed, err := parser.ParseMetricSelector(selector) if err != nil { return nil, errors.Wrapf(err, "error parsing metric selector") } - tombstone.Matchers = append(tombstone.Matchers, parsedMatcher...) + //keep the matchers in a 1D array because the deletions are applied based + // on the "and" between all matchers. There is no need to + m = append(m, parsed...) } - return tombstone, nil + return m, nil } -func UpgradeTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { +func UpdateTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { + userLogger := util_log.WithUserID(t.UserID, util_log.Logger) // Create the new tombstone, and will delete the previous tombstone - newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix(), t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) + newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix()*1000, t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) + newT.Matchers = t.Matchers err := WriteTombstoneFile(ctx, bkt, newT.UserID, cfgProvider, newT) if err != nil { - level.Error(util_log.Logger).Log("msg", "error creating file tombstone file with the updated state", "err", err) + level.Error(userLogger).Log("msg", "error creating file tombstone file with the updated state", "err", err) return nil, err } - err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t) - if err != nil { - level.Error(util_log.Logger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t); err != nil { + level.Error(userLogger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) } return newT, nil @@ -287,7 +308,7 @@ func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider b userBucket := bucket.NewUserBucketClient(t.UserID, bkt, cfgProvider) // Create the full path of the tombstone by appending the state as the extension - filename := t.RequestID + ".json." + string(t.State) + filename := t.RequestID + "." + string(t.State) + ".json" fullTombstonePath := path.Join(TombstonePath, filename) level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go new file mode 100644 index 0000000000..191832bd22 --- /dev/null +++ b/pkg/storage/tsdb/tombstones_test.go @@ -0,0 +1,334 @@ +package tsdb + +import ( + "bytes" + "context" + "encoding/json" + "path" + "testing" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + "github.com/stretchr/testify/require" + "github.com/thanos-io/thanos/pkg/objstore" + "github.com/weaveworks/common/user" +) + +func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { + + username := "user" + requestID := "requestID" + + bkt := objstore.NewInMemBucket() + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + //create the tombstone + tombstone := NewTombstone("fake", 0, 0, 0, 1, []string{"match"}, requestID, StatePending) + err := WriteTombstoneFile(ctx, bkt, username, nil, tombstone) + require.NoError(t, err) + + filename := requestID + "." + string(StatePending) + ".json" + exists, _ := bkt.Exists(ctx, path.Join(username, TombstonePath, filename)) + require.True(t, exists) + + // Creating the same tombstone twice should result in an error + err = WriteTombstoneFile(ctx, bkt, username, nil, tombstone) + require.ErrorIs(t, err, ErrTombstoneAlreadyExists) + +} + +func TestTombstonesExists(t *testing.T) { + const username = "user" + const requestID = "requestID" + + for name, tc := range map[string]struct { + objects map[string][]byte + targetRequestState BlockDeleteRequestState + exists bool + }{ + "no tombstones exist": { + objects: nil, + targetRequestState: StatePending, + exists: false, + }, + + "tombstone exists but different state": { + objects: map[string][]byte{ + username + "/tombstones/" + requestID + "." + string(StateProcessed) + ".json": []byte("data"), + username + "/tombstones/" + requestID + "." + string(StateCancelled) + ".json": []byte("data"), + }, + targetRequestState: StatePending, + exists: false, + }, + + "tombstone exists with correct state": { + objects: map[string][]byte{ + username + "/tombstones/" + requestID + "." + string(StatePending) + ".json": []byte("data"), + }, + targetRequestState: StatePending, + exists: true, + }, + } { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + bkt := objstore.NewInMemBucket() + // "upload" sample tombstone files + for objName, data := range tc.objects { + require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) + } + + userBkt := bucket.NewUserBucketClient(username, bkt, nil) + + res, err := TombstoneExists(ctx, userBkt, username, requestID, tc.targetRequestState) + require.NoError(t, err) + require.Equal(t, tc.exists, res) + }) + } +} + +func TestTombstonesDeletion(t *testing.T) { + const username = "user" + const requestID = "requestID" + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + tPending := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, StatePending) + + tPendingPath := username + "/tombstones/" + requestID + "." + string(StatePending) + ".json" + tProcessedPath := username + "/tombstones/" + requestID + "." + string(StateProcessed) + ".json" + + bkt := objstore.NewInMemBucket() + // "upload" sample tombstone file + require.NoError(t, bkt.Upload(context.Background(), tPendingPath, bytes.NewReader([]byte("data")))) + require.NoError(t, bkt.Upload(context.Background(), tProcessedPath, bytes.NewReader([]byte("data")))) + + require.NoError(t, DeleteTombstoneFile(ctx, bkt, nil, tPending)) + + // make sure the pending tombstone was deleted + exists, _ := bkt.Exists(ctx, tPendingPath) + require.False(t, exists) + + // the processed tombstone with the same requestID should still be in the bucket + exists, _ = bkt.Exists(ctx, tProcessedPath) + require.True(t, exists) + +} + +func TestTombstoneUpdateState(t *testing.T) { + const username = "user" + const requestID = "requestID" + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + tPending := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, StatePending) + tPendingPath := username + "/tombstones/" + requestID + "." + string(StatePending) + ".json" + + bkt := objstore.NewInMemBucket() + // "upload" sample tombstone file + require.NoError(t, bkt.Upload(context.Background(), tPendingPath, bytes.NewReader([]byte("data")))) + + tProcessed, err := UpdateTombstoneState(ctx, bkt, nil, tPending, StateProcessed) + require.NoError(t, err) + + // make sure the pending tombstone was deleted + exists, _ := bkt.Exists(ctx, tPendingPath) + require.False(t, exists) + + // check that the new tombstone with the updated state has been created + tProcessedPath := username + "/tombstones/" + tProcessed.RequestID + "." + string(tProcessed.State) + ".json" + exists, _ = bkt.Exists(ctx, tProcessedPath) + require.True(t, exists) +} + +func TestTombstones_RemoveFilesWithDuplicateRequestIds(t *testing.T) { + const username = "user" + const requestID = "requestID" + + for name, tc := range map[string]struct { + remaningState BlockDeleteRequestState + deletedState BlockDeleteRequestState + }{ + "pending state tombstone file should be deleted if deleted file exists ": { + remaningState: StateCancelled, + deletedState: StatePending, + }, + "pending state tombstone file should be deleted if processed file exists ": { + remaningState: StateProcessed, + deletedState: StatePending, + }, + } { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + bkt := objstore.NewInMemBucket() + + // create the tombstones + tRemaining := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, tc.remaningState) + tToDelete := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, tc.deletedState) + + // "upload" sample tombstone files + tRemainPath := username + "/tombstones/" + requestID + "." + string(tc.remaningState) + ".json" + tDeletePath := username + "/tombstones/" + requestID + "." + string(tc.deletedState) + ".json" + + tRemainingJSON, _ := json.Marshal(tRemaining) + tToDeleteJSON, _ := json.Marshal(tToDelete) + + require.NoError(t, bkt.Upload(context.Background(), tRemainPath, bytes.NewReader([]byte(tRemainingJSON)))) + require.NoError(t, bkt.Upload(context.Background(), tDeletePath, bytes.NewReader([]byte(tToDeleteJSON)))) + + resultingT, err := removeDuplicateTombstone(ctx, bkt, nil, username, tRemaining, tToDelete) + require.NoError(t, err) + require.Equal(t, tRemaining, resultingT) + + //check that the tombstone has been deleted + exists, _ := bkt.Exists(ctx, tDeletePath) + require.False(t, exists) + + exists, _ = bkt.Exists(ctx, tRemainPath) + require.True(t, exists) + + //Test switching the order of the parameters passed in the function + + // reupload the files + require.NoError(t, bkt.Upload(context.Background(), tRemainPath, bytes.NewReader([]byte(tRemainingJSON)))) + require.NoError(t, bkt.Upload(context.Background(), tDeletePath, bytes.NewReader([]byte(tToDeleteJSON)))) + + resultingT, err = removeDuplicateTombstone(ctx, bkt, nil, username, tToDelete, tRemaining) + require.NoError(t, err) + require.Equal(t, tRemaining, resultingT) + + //check that the tombstone has been deleted + exists, _ = bkt.Exists(ctx, tDeletePath) + require.False(t, exists) + + exists, _ = bkt.Exists(ctx, tRemainPath) + require.True(t, exists) + }) + } +} + +func TestGetSingleTombstone(t *testing.T) { + const username = "user" + const requestID = "requestID" + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + // When getting a specific request id, there could be a case where multiple + // files exist for the same request but with different extensions to indicate a + // different state. If thats the case, then the older state should be deleted. + + // Add multiple tombstones with the same request id but different states + tPending := NewTombstone(username, 0, 0, 0, 0, []string{"node_exporter"}, requestID, StatePending) + tProcessed := NewTombstone(username, 10, 20, 30, 60, []string{"node_exporter"}, requestID, StateProcessed) + + bkt := objstore.NewInMemBucket() + // first add the tombstone files to the object store + require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tPending)) + require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tProcessed)) + + tRetrieved, err := GetDeleteRequestById(ctx, bkt, nil, username, requestID) + require.NoError(t, err) + + //verify that all the information was read correctly + require.Equal(t, tProcessed.StartTime, tRetrieved.StartTime) + require.Equal(t, tProcessed.EndTime, tRetrieved.EndTime) + require.Equal(t, tProcessed.RequestCreatedAt, tRetrieved.RequestCreatedAt) + require.Equal(t, tProcessed.StateCreatedAt, tRetrieved.StateCreatedAt) + require.Equal(t, tProcessed.Selectors, tRetrieved.Selectors) + require.Equal(t, tProcessed.RequestID, tRetrieved.RequestID) + require.Equal(t, tProcessed.UserID, tRetrieved.UserID) + require.Equal(t, tProcessed.State, tRetrieved.State) + + // make sure the pending tombstone was deleted + tPendingPath := username + "/tombstones/" + requestID + "." + string(StatePending) + ".json" + exists, _ := bkt.Exists(ctx, tPendingPath) + require.False(t, exists) + + // Get single tombstone that doesn't exist should return nil + tRetrieved, err = GetDeleteRequestById(ctx, bkt, nil, username, "unknownRequestID") + require.NoError(t, err) + require.Nil(t, tRetrieved) +} + +func TestGetAllTombstones(t *testing.T) { + const username = "user" + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + bkt := objstore.NewInMemBucket() + + tombstonesInput := []*Tombstone{ + NewTombstone(username, 0, 0, 0, 0, []string{}, "request1", StatePending), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request1", StateCancelled), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request2", StatePending), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request3", StatePending), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request4", StateProcessed), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request5", StateCancelled), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request6", StatePending), + NewTombstone(username, 0, 0, 0, 0, []string{}, "request6", StateProcessed), + } + + requiredOutput := map[string]BlockDeleteRequestState{ + "request1": StateCancelled, + "request2": StatePending, + "request3": StatePending, + "request4": StateProcessed, + "request5": StateCancelled, + "request6": StateProcessed, + } + + // add all tombstones to the bkt + for _, ts := range tombstonesInput { + require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, ts)) + } + + tombstonesOutput, err := GetAllDeleteRequestsForUser(ctx, bkt, nil, username) + require.NoError(t, err) + + outputMap := make(map[string]BlockDeleteRequestState) + for _, ts := range tombstonesOutput { + _, exists := outputMap[ts.RequestID] + // There should not be more than one ts for each request id + require.False(t, exists) + + outputMap[ts.RequestID] = ts.State + } + + require.Equal(t, requiredOutput, outputMap) +} + +func TestTombstoneReadWithInvalidFileName(t *testing.T) { + const username = "user" + const requestID = "requestID" + bkt := objstore.NewInMemBucket() + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, username) + + { + tInvalidPath := username + "/tombstones/" + requestID + "." + string(StatePending) + _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + + require.ErrorIs(t, err, ErrInvalidDeletionRequestState) + } + + { + tInvalidPath := username + "/tombstones/" + requestID + _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + + require.ErrorIs(t, err, ErrInvalidDeletionRequestState) + } + + { + tInvalidPath := username + "/tombstones/" + requestID + ".json." + string(StatePending) + _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + require.ErrorIs(t, err, ErrInvalidDeletionRequestState) + } + +} From 6f7d749420801e75cfe9d05c5151ee3feddb672f Mon Sep 17 00:00:00 2001 From: ilangofman Date: Wed, 14 Jul 2021 18:20:52 -0400 Subject: [PATCH 03/21] change 1 function name Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 2 +- pkg/storage/tsdb/tombstones.go | 2 +- pkg/storage/tsdb/tombstones_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 97833a63ec..218aa30141 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -172,7 +172,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r params := r.URL.Query() requestID := params.Get("request_id") - deleteRequest, err := cortex_tsdb.GetDeleteRequestById(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + deleteRequest, err := cortex_tsdb.GetDeleteRequestByIdForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 081b33d948..29c93de04e 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -103,7 +103,7 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } -func GetDeleteRequestById(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { +func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { userLogger := util_log.WithUserID(userID, util_log.Logger) userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 191832bd22..8159cb0a42 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -233,7 +233,7 @@ func TestGetSingleTombstone(t *testing.T) { require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tPending)) require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tProcessed)) - tRetrieved, err := GetDeleteRequestById(ctx, bkt, nil, username, requestID) + tRetrieved, err := GetDeleteRequestByIdForUser(ctx, bkt, nil, username, requestID) require.NoError(t, err) //verify that all the information was read correctly @@ -252,7 +252,7 @@ func TestGetSingleTombstone(t *testing.T) { require.False(t, exists) // Get single tombstone that doesn't exist should return nil - tRetrieved, err = GetDeleteRequestById(ctx, bkt, nil, username, "unknownRequestID") + tRetrieved, err = GetDeleteRequestByIdForUser(ctx, bkt, nil, username, "unknownRequestID") require.NoError(t, err) require.Nil(t, tRetrieved) } From 1f8917310bd72fa00ecbfc7272ced3ed118551c4 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Thu, 15 Jul 2021 14:31:54 -0400 Subject: [PATCH 04/21] Remove the deletion during get requests Signed-off-by: ilangofman --- pkg/storage/tsdb/tombstones.go | 50 +++++-------------- pkg/storage/tsdb/tombstones_test.go | 74 ----------------------------- 2 files changed, 12 insertions(+), 112 deletions(-) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 29c93de04e..c4663cb43e 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -104,7 +104,6 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri } func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { - userLogger := util_log.WithUserID(userID, util_log.Logger) userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) found := []*Tombstone{} @@ -130,14 +129,8 @@ func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgPr return nil, nil } - // If there are multiple tombstones with the same request id but different state, should delete the files with the lower state - for _, ts := range found[0 : len(found)-1] { - level.Info(userLogger).Log("msg", "Found extra tombstone file with outdated state. Will delete it.", "requestID", ts.RequestID, "state", ts.State) - if err := DeleteTombstoneFile(ctx, bkt, cfgProvider, ts); err != nil { - level.Error(userLogger).Log("msg", "Unable to delete redundant tombstone file.", "request_id", ts.RequestID, "state", ts.State, "err", err) - } - } - + // If there are multiple tombstones with the same request id but different state, want to return only the latest one + // The older states will be cleaned up by the compactor return found[len(found)-1], nil } @@ -157,7 +150,10 @@ func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgPr if _, exists := tombstoneMap[t.RequestID]; !exists { tombstoneMap[t.RequestID] = t } else { - newT, err := removeDuplicateTombstone(ctx, bkt, cfgProvider, t.UserID, t, tombstoneMap[t.RequestID]) + // if there is more than one tombstone for a given request, + // we only want to return the latest state. The older file + // will be cleaned by the compactor + newT, err := getLatestTombstateByState(t, tombstoneMap[t.RequestID]) if err != nil { return err } @@ -178,44 +174,22 @@ func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgPr return deletionRequests, nil } -func removeDuplicateTombstone(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, tombstoneA *Tombstone, tombstoneB *Tombstone) (*Tombstone, error) { - userLogger := util_log.WithUserID(userID, util_log.Logger) - - if tombstoneA.RequestID != tombstoneB.RequestID { - return nil, errors.New("Cannot run the remove duplicate function for 2 tombstones with different request ID's") - } else if tombstoneA.State == tombstoneB.State { - return tombstoneA, nil - } - - level.Info(userLogger).Log("msg", "Found multiple tombstones for the same deletion request ID but with different state'", "request ID", tombstoneA.RequestID) - - //find which tombstone should be removed - var toDelete, toKeep *Tombstone - - orderA, err := tombstoneA.GetStateOrder() +func getLatestTombstateByState(a *Tombstone, b *Tombstone) (*Tombstone, error) { + orderA, err := a.GetStateOrder() if err != nil { return nil, err } - orderB, err := tombstoneB.GetStateOrder() + orderB, err := b.GetStateOrder() if err != nil { return nil, err } - if orderA > orderB { - toKeep = tombstoneA - toDelete = tombstoneB - } else { - toKeep = tombstoneB - toDelete = tombstoneA - } - - level.Info(userLogger).Log("msg", "For deletion request %s", toDelete.RequestID, ", removing %s", toDelete.State, "state file, while keeping .", string(toKeep.State), "state file") - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, toDelete); err != nil { - return nil, errors.Wrap(err, "failed to delete duplicate tombstone") + if orderB > orderA { + return b, nil } - return toKeep, nil + return a, nil } func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID string, tombstonePath string) (*Tombstone, error) { diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 8159cb0a42..97f9857a42 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -3,7 +3,6 @@ package tsdb import ( "bytes" "context" - "encoding/json" "path" "testing" @@ -145,74 +144,6 @@ func TestTombstoneUpdateState(t *testing.T) { require.True(t, exists) } -func TestTombstones_RemoveFilesWithDuplicateRequestIds(t *testing.T) { - const username = "user" - const requestID = "requestID" - - for name, tc := range map[string]struct { - remaningState BlockDeleteRequestState - deletedState BlockDeleteRequestState - }{ - "pending state tombstone file should be deleted if deleted file exists ": { - remaningState: StateCancelled, - deletedState: StatePending, - }, - "pending state tombstone file should be deleted if processed file exists ": { - remaningState: StateProcessed, - deletedState: StatePending, - }, - } { - t.Run(name, func(t *testing.T) { - ctx := context.Background() - ctx = user.InjectOrgID(ctx, username) - - bkt := objstore.NewInMemBucket() - - // create the tombstones - tRemaining := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, tc.remaningState) - tToDelete := NewTombstone(username, 0, 0, 0, 0, []string{}, requestID, tc.deletedState) - - // "upload" sample tombstone files - tRemainPath := username + "/tombstones/" + requestID + "." + string(tc.remaningState) + ".json" - tDeletePath := username + "/tombstones/" + requestID + "." + string(tc.deletedState) + ".json" - - tRemainingJSON, _ := json.Marshal(tRemaining) - tToDeleteJSON, _ := json.Marshal(tToDelete) - - require.NoError(t, bkt.Upload(context.Background(), tRemainPath, bytes.NewReader([]byte(tRemainingJSON)))) - require.NoError(t, bkt.Upload(context.Background(), tDeletePath, bytes.NewReader([]byte(tToDeleteJSON)))) - - resultingT, err := removeDuplicateTombstone(ctx, bkt, nil, username, tRemaining, tToDelete) - require.NoError(t, err) - require.Equal(t, tRemaining, resultingT) - - //check that the tombstone has been deleted - exists, _ := bkt.Exists(ctx, tDeletePath) - require.False(t, exists) - - exists, _ = bkt.Exists(ctx, tRemainPath) - require.True(t, exists) - - //Test switching the order of the parameters passed in the function - - // reupload the files - require.NoError(t, bkt.Upload(context.Background(), tRemainPath, bytes.NewReader([]byte(tRemainingJSON)))) - require.NoError(t, bkt.Upload(context.Background(), tDeletePath, bytes.NewReader([]byte(tToDeleteJSON)))) - - resultingT, err = removeDuplicateTombstone(ctx, bkt, nil, username, tToDelete, tRemaining) - require.NoError(t, err) - require.Equal(t, tRemaining, resultingT) - - //check that the tombstone has been deleted - exists, _ = bkt.Exists(ctx, tDeletePath) - require.False(t, exists) - - exists, _ = bkt.Exists(ctx, tRemainPath) - require.True(t, exists) - }) - } -} - func TestGetSingleTombstone(t *testing.T) { const username = "user" const requestID = "requestID" @@ -246,11 +177,6 @@ func TestGetSingleTombstone(t *testing.T) { require.Equal(t, tProcessed.UserID, tRetrieved.UserID) require.Equal(t, tProcessed.State, tRetrieved.State) - // make sure the pending tombstone was deleted - tPendingPath := username + "/tombstones/" + requestID + "." + string(StatePending) + ".json" - exists, _ := bkt.Exists(ctx, tPendingPath) - require.False(t, exists) - // Get single tombstone that doesn't exist should return nil tRetrieved, err = GetDeleteRequestByIdForUser(ctx, bkt, nil, username, "unknownRequestID") require.NoError(t, err) From 2feb3ce08b0923c4e14cb2bee02da6a0d2d6b75c Mon Sep 17 00:00:00 2001 From: ilangofman Date: Thu, 15 Jul 2021 16:35:16 -0400 Subject: [PATCH 05/21] make the hashing consistent no matter the selector order Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 218aa30141..0dd1794ddf 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -7,6 +7,7 @@ import ( "encoding/json" fmt "fmt" "net/http" + "sort" "strconv" strings "strings" "time" @@ -221,6 +222,7 @@ func getTombstoneRequestID(startTime int64, endTime int64, selectors []string) s b.WriteString(",") b.WriteString(strconv.FormatInt(endTime, 10)) + sort.Strings(selectors) for _, s := range selectors { b.WriteString(",") b.WriteString(s) From 342d673eae284b14920ee1e10e08d329d756b125 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Sun, 18 Jul 2021 17:36:19 -0400 Subject: [PATCH 06/21] fix edge case Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 12 +++-- pkg/chunk/purger/blocks_purger_test.go | 71 ++++++++++++++++++++++++++ pkg/storage/tsdb/tombstones.go | 33 ++++++++++-- pkg/storage/tsdb/tombstones_test.go | 2 +- 4 files changed, 109 insertions(+), 9 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 0dd1794ddf..62650d2283 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -63,7 +63,6 @@ func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, } func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { @@ -118,18 +117,25 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht } requestId := getTombstoneRequestID(startTime, endTime, match) + + // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one + if err := cortex_tsdb.RemoveCancelledStateIfExists(ctx, api.bucketClient, userID, api.cfgProvider, requestId); err != nil { + level.Error(util_log.Logger).Log("msg", "removing cancelled tombstone state if it exists", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + curTime := time.Now().Unix() * 1000 t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestId, cortex_tsdb.StatePending) if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { - if err == cortex_tsdb.ErrTombstoneAlreadyExists { level.Error(util_log.Logger).Log("msg", "delete request tombstone with same information already exists", "err", err) http.Error(w, err.Error(), http.StatusBadRequest) return } - level.Error(util_log.Logger).Log("msg", "error adding delete request to the store", "err", err) + level.Error(util_log.Logger).Log("msg", "error adding delete request to the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return } diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 581bfa1655..08cba4b47e 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -124,6 +124,77 @@ func TestBlocksDeleteSeries_AddingSameRequestTwiceShouldFail(t *testing.T) { } +func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testing.T) { + + // If a tombstone has previously been cancelled, and a new request + // being made results in the same request id, the cancelled tombstone + // should be deleted from the bucket + + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + + ctx := context.Background() + ctx = user.InjectOrgID(ctx, userID) + + //first create a new tombstone + paramsCreate := url.Values{ + "start": []string{"1"}, + "end": []string{"2"}, + "match[]": []string{"node_exporter"}, + } + + uCreate := &url.URL{ + RawQuery: paramsCreate.Encode(), + } + + reqCreate := &http.Request{ + Method: "GET", + RequestURI: uCreate.String(), + URL: uCreate, + Body: http.NoBody, + Header: http.Header{}, + } + + resp := httptest.NewRecorder() + api.AddDeleteRequestHandler(resp, reqCreate.WithContext(ctx)) + require.Equal(t, http.StatusNoContent, resp.Code) + + //cancel the previous request + requestID := getTombstoneRequestID(1000, 2000, []string{"node_exporter"}) + paramsDelete := url.Values{ + "request_id": []string{requestID}, + } + uCancel := &url.URL{ + RawQuery: paramsDelete.Encode(), + } + + reqCancel := &http.Request{ + Method: "POST", + RequestURI: uCancel.String(), // This is what the httpgrpc code looks at. + URL: uCancel, + Body: http.NoBody, + Header: http.Header{}, + } + + resp = httptest.NewRecorder() + api.CancelDeleteRequestHandler(resp, reqCancel.WithContext(ctx)) + require.Equal(t, http.StatusNoContent, resp.Code) + + // check that the cancelled file exists + tCancelledPath := userID + "/tombstones/" + requestID + "." + string(cortex_tsdb.StateCancelled) + ".json" + exists, _ := bkt.Exists(ctx, tCancelledPath) + require.True(t, exists) + + // create a new request and make sure the cancelled file no longer exists + resp = httptest.NewRecorder() + api.AddDeleteRequestHandler(resp, reqCreate.WithContext(ctx)) + require.Equal(t, http.StatusNoContent, resp.Code) + + exists, _ = bkt.Exists(ctx, tCancelledPath) + require.False(t, exists) + +} + func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { for name, tc := range map[string]struct { diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index c4663cb43e..7b9154a6b1 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "path" "path/filepath" "time" @@ -103,6 +104,28 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } +func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestId string) error { + usrBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) + userLogger := util_log.WithUserID(userID, util_log.Logger) + + exists, err := TombstoneExists(ctx, usrBkt, userID, requestId, StateCancelled) + if err != nil { + level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestId, "err", err) + return err + } + + if exists { + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestId, StateCancelled); err != nil { + level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestId, "err", err) + return err + } + fmt.Print("DELETEd") + } else { + fmt.Println("DOES NOT EXIST") + } + return nil +} + func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) @@ -270,19 +293,19 @@ func UpdateTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider return nil, err } - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t); err != nil { + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t.UserID, t.RequestID, t.State); err != nil { level.Error(userLogger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) } return newT, nil } -func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone) error { - userLogger := util_log.WithUserID(t.UserID, util_log.Logger) - userBucket := bucket.NewUserBucketClient(t.UserID, bkt, cfgProvider) +func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userId string, requestId string, state BlockDeleteRequestState) error { + userLogger := util_log.WithUserID(userId, util_log.Logger) + userBucket := bucket.NewUserBucketClient(userId, bkt, cfgProvider) // Create the full path of the tombstone by appending the state as the extension - filename := t.RequestID + "." + string(t.State) + ".json" + filename := requestId + "." + string(state) + ".json" fullTombstonePath := path.Join(TombstonePath, filename) level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 97f9857a42..2d6d6d0b45 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -105,7 +105,7 @@ func TestTombstonesDeletion(t *testing.T) { require.NoError(t, bkt.Upload(context.Background(), tPendingPath, bytes.NewReader([]byte("data")))) require.NoError(t, bkt.Upload(context.Background(), tProcessedPath, bytes.NewReader([]byte("data")))) - require.NoError(t, DeleteTombstoneFile(ctx, bkt, nil, tPending)) + require.NoError(t, DeleteTombstoneFile(ctx, bkt, nil, tPending.UserID, tPending.RequestID, tPending.State)) // make sure the pending tombstone was deleted exists, _ := bkt.Exists(ctx, tPendingPath) From 361d166c894a87bab733df7766e1e08ebd59222d Mon Sep 17 00:00:00 2001 From: ilangofman Date: Sun, 18 Jul 2021 17:56:49 -0400 Subject: [PATCH 07/21] Fix feature flag Signed-off-by: ilangofman --- pkg/api/api.go | 19 ++++++++++--------- pkg/cortex/modules.go | 4 ++-- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 2029f16f4a..56f45e527c 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -272,17 +272,18 @@ func (a *API) RegisterChunksPurger(store *purger.DeleteStore, deleteRequestCance a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(deleteRequestHandler.CancelDeleteRequestHandler), true, "PUT", "POST") } -func (a *API) RegisterBlocksPurger(blocksPurger *purger.BlocksPurgerAPI) { +func (a *API) RegisterBlocksPurger(blocksPurger *purger.BlocksPurgerAPI, seriesDeletionEnabled bool) { - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.GetAllDeleteRequestsHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") - - // Legacy Routes - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") + if seriesDeletionEnabled { + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.GetAllDeleteRequestsHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") + // Legacy Routes + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") + } // Tenant Deletion a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(blocksPurger.DeleteTenant), true, "POST") a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(blocksPurger.DeleteTenantStatus), true, "GET") diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index 31c64123f0..201ba67839 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -802,7 +802,7 @@ func (t *Cortex) initChunksPurger() (services.Service, error) { } func (t *Cortex) initBlocksPurger() (services.Service, error) { - if t.Cfg.Storage.Engine != storage.StorageEngineBlocks || !t.Cfg.PurgerConfig.Enable { + if t.Cfg.Storage.Engine != storage.StorageEngineBlocks { return nil, nil } @@ -812,7 +812,7 @@ func (t *Cortex) initBlocksPurger() (services.Service, error) { return nil, err } - t.API.RegisterBlocksPurger(blockPurger) + t.API.RegisterBlocksPurger(blockPurger, t.Cfg.PurgerConfig.Enable) return nil, nil } From f1fc18670a54b9f9ccaded45b75c3346579c58fb Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 19 Jul 2021 13:07:15 -0400 Subject: [PATCH 08/21] refactor minor Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 18 ++++++---- pkg/chunk/purger/blocks_purger_test.go | 4 +-- pkg/storage/tsdb/tombstones.go | 46 ++++++++++++-------------- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 62650d2283..108f33772b 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -118,6 +118,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht requestId := getTombstoneRequestID(startTime, endTime, match) + // Since the request id is based on a hash of the parameters, there is a possibility that a tombstone could already exist for it // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one if err := cortex_tsdb.RemoveCancelledStateIfExists(ctx, api.bucketClient, userID, api.cfgProvider, requestId); err != nil { level.Error(util_log.Logger).Log("msg", "removing cancelled tombstone state if it exists", "err", err) @@ -125,16 +126,21 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht return } + prevT, err := cortex_tsdb.GetDeleteRequestByIdForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestId) + if err != nil { + level.Error(util_log.Logger).Log("msg", "error getting delete request by id", "err", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + if prevT != nil { + http.Error(w, "delete request tombstone with same information already exists", http.StatusBadRequest) + return + } + curTime := time.Now().Unix() * 1000 t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestId, cortex_tsdb.StatePending) if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { - if err == cortex_tsdb.ErrTombstoneAlreadyExists { - level.Error(util_log.Logger).Log("msg", "delete request tombstone with same information already exists", "err", err) - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - level.Error(util_log.Logger).Log("msg", "error adding delete request to the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 08cba4b47e..ab5dfc1d67 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -170,7 +170,7 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin reqCancel := &http.Request{ Method: "POST", - RequestURI: uCancel.String(), // This is what the httpgrpc code looks at. + RequestURI: uCancel.String(), URL: uCancel, Body: http.NoBody, Header: http.Header{}, @@ -256,7 +256,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { req := &http.Request{ Method: "POST", - RequestURI: u.String(), // This is what the httpgrpc code looks at. + RequestURI: u.String(), URL: u, Body: http.NoBody, Header: http.Header{}, diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 7b9154a6b1..61581dae67 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "encoding/json" - "fmt" "path" "path/filepath" "time" @@ -77,7 +76,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, fullTombstonePath := path.Join(TombstonePath, filename) - // Check if the tombstone already exists. Could be the case the same request was made + // Check if the tombstone already exists for the same state. Could be the case the same request was made // and is already in the middle of deleting series. Creating a new tombstone would restart // the progress tombstoneExists, err := TombstoneExists(ctx, userBkt, userID, tombstone.RequestID, tombstone.State) @@ -104,28 +103,6 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } -func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestId string) error { - usrBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - userLogger := util_log.WithUserID(userID, util_log.Logger) - - exists, err := TombstoneExists(ctx, usrBkt, userID, requestId, StateCancelled) - if err != nil { - level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestId, "err", err) - return err - } - - if exists { - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestId, StateCancelled); err != nil { - level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestId, "err", err) - return err - } - fmt.Print("DELETEd") - } else { - fmt.Println("DOES NOT EXIST") - } - return nil -} - func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) @@ -314,6 +291,27 @@ func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider b } +func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestId string) error { + usrBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) + userLogger := util_log.WithUserID(userID, util_log.Logger) + + exists, err := TombstoneExists(ctx, usrBkt, userID, requestId, StateCancelled) + if err != nil { + level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestId, "err", err) + return err + } + + if exists { + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestId, StateCancelled); err != nil { + level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestId, "err", err) + return err + } + level.Info(userLogger).Log("msg", "Removing tombstone file with previously cancelled state", "requestID", requestId, "err", err) + + } + return nil +} + func isValidDeleteRequestState(state BlockDeleteRequestState) bool { switch state { case From f9c16f6bbf3e7e42ebab4cd408e324fd2cc77807 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 19 Jul 2021 13:18:17 -0400 Subject: [PATCH 09/21] minor refactor Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 1 - pkg/chunk/purger/blocks_purger_test.go | 20 ++++++++++---------- pkg/storage/tsdb/tombstones_test.go | 4 ++-- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 108f33772b..599da684d8 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -174,7 +174,6 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r } func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index ab5dfc1d67..210a3cffe9 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -62,7 +62,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") + ctx = user.InjectOrgID(ctx, userID) u := &url.URL{ RawQuery: tc.parameters.Encode(), @@ -70,7 +70,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { req := &http.Request{ Method: "GET", - RequestURI: u.String(), // This is what the httpgrpc code looks at. + RequestURI: u.String(), URL: u, Body: http.NoBody, Header: http.Header{}, @@ -90,7 +90,7 @@ func TestBlocksDeleteSeries_AddingSameRequestTwiceShouldFail(t *testing.T) { api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") + ctx = user.InjectOrgID(ctx, userID) params := url.Values{ "start": []string{"1"}, @@ -239,11 +239,11 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") + ctx = user.InjectOrgID(ctx, userID) //create the tombstone - tombstone := cortex_tsdb.NewTombstone("fake", tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) - err := cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, "fake", api.cfgProvider, tombstone) + tombstone := cortex_tsdb.NewTombstone(userID, tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) + err := cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, tombstone) require.NoError(t, err) params := url.Values{ @@ -267,8 +267,8 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { require.Equal(t, tc.expectedHttpStatus, resp.Code) // check if the cancelled tombstone file exists - userBkt := bucket.NewUserBucketClient("fake", bkt, api.cfgProvider) - exists, _ := cortex_tsdb.TombstoneExists(ctx, userBkt, "fake", "request_id", cortex_tsdb.StateCancelled) + userBkt := bucket.NewUserBucketClient(userID, bkt, api.cfgProvider) + exists, _ := cortex_tsdb.TombstoneExists(ctx, userBkt, userID, "request_id", cortex_tsdb.StateCancelled) require.Equal(t, tc.cancelledFileExists, exists) }) @@ -287,7 +287,7 @@ func TestDeleteTenant(t *testing.T) { { ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") + ctx = user.InjectOrgID(ctx, userID) req := &http.Request{} resp := httptest.NewRecorder() @@ -295,7 +295,7 @@ func TestDeleteTenant(t *testing.T) { require.Equal(t, http.StatusOK, resp.Code) objs := bkt.Objects() - require.NotNil(t, objs[path.Join("fake", tsdb.TenantDeletionMarkPath)]) + require.NotNil(t, objs[path.Join(userID, tsdb.TenantDeletionMarkPath)]) } } diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 2d6d6d0b45..c858f8b18a 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -20,10 +20,10 @@ func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { bkt := objstore.NewInMemBucket() ctx := context.Background() - ctx = user.InjectOrgID(ctx, "fake") + ctx = user.InjectOrgID(ctx, username) //create the tombstone - tombstone := NewTombstone("fake", 0, 0, 0, 1, []string{"match"}, requestID, StatePending) + tombstone := NewTombstone(username, 0, 0, 0, 1, []string{"match"}, requestID, StatePending) err := WriteTombstoneFile(ctx, bkt, username, nil, tombstone) require.NoError(t, err) From 8f0338fd2e2db57c7c57121da07262e2768ee684 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 19 Jul 2021 13:59:24 -0400 Subject: [PATCH 10/21] Fix lint errors Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 12 ++++++----- pkg/chunk/purger/blocks_purger_test.go | 24 ++++++++++----------- pkg/storage/tsdb/tombstones.go | 30 +++++++++++++------------- pkg/storage/tsdb/tombstones_test.go | 4 ++-- 4 files changed, 36 insertions(+), 34 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 599da684d8..3f7693994e 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -116,17 +116,17 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht return } - requestId := getTombstoneRequestID(startTime, endTime, match) + requestID := getTombstoneRequestID(startTime, endTime, match) // Since the request id is based on a hash of the parameters, there is a possibility that a tombstone could already exist for it // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one - if err := cortex_tsdb.RemoveCancelledStateIfExists(ctx, api.bucketClient, userID, api.cfgProvider, requestId); err != nil { + if err := cortex_tsdb.RemoveCancelledStateIfExists(ctx, api.bucketClient, userID, api.cfgProvider, requestID); err != nil { level.Error(util_log.Logger).Log("msg", "removing cancelled tombstone state if it exists", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return } - prevT, err := cortex_tsdb.GetDeleteRequestByIdForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestId) + prevT, err := cortex_tsdb.GetDeleteRequestByIDForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request by id", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -138,7 +138,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht } curTime := time.Now().Unix() * 1000 - t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestId, cortex_tsdb.StatePending) + t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestID, cortex_tsdb.StatePending) if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { level.Error(util_log.Logger).Log("msg", "error adding delete request to the object store", "err", err) @@ -174,6 +174,8 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r } func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { + fmt.Println("CANCel PERIOD: ", api.deleteRequestCancelPeriod.Milliseconds()) + ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { @@ -184,7 +186,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r params := r.URL.Query() requestID := params.Get("request_id") - deleteRequest, err := cortex_tsdb.GetDeleteRequestByIdForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + deleteRequest, err := cortex_tsdb.GetDeleteRequestByIDForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 210a3cffe9..5de2a70530 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -24,11 +24,11 @@ import ( func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { for name, tc := range map[string]struct { parameters url.Values - expectedHttpStatus int + expectedHTTPStatus int }{ "empty": { parameters: nil, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, "valid request": { @@ -37,7 +37,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { "end": []string{"2"}, "match[]": []string{"selector"}, }, - expectedHttpStatus: http.StatusNoContent, + expectedHTTPStatus: http.StatusNoContent, }, "end time in the future": { @@ -46,7 +46,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { "end": []string{strconv.Itoa(math.MaxInt64)}, "match[]": []string{"selector"}, }, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, "the start time is after the end time": { parameters: url.Values{ @@ -54,7 +54,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { "end": []string{"1"}, "match[]": []string{"selector"}, }, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, } { t.Run(name, func(t *testing.T) { @@ -78,7 +78,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { resp := httptest.NewRecorder() api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) - require.Equal(t, tc.expectedHttpStatus, resp.Code) + require.Equal(t, tc.expectedHTTPStatus, resp.Code) }) } @@ -202,14 +202,14 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { requestState cortex_tsdb.BlockDeleteRequestState cancellationPeriod time.Duration cancelledFileExists bool - expectedHttpStatus int + expectedHTTPStatus int }{ "not allowed, grace period has passed": { createdAt: 0, requestState: cortex_tsdb.StatePending, cancellationPeriod: time.Second, cancelledFileExists: false, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, "allowed, grace period not over yet": { @@ -217,21 +217,21 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { requestState: cortex_tsdb.StatePending, cancellationPeriod: time.Hour, cancelledFileExists: true, - expectedHttpStatus: http.StatusNoContent, + expectedHTTPStatus: http.StatusNoContent, }, "not allowed, deletion already occurred": { createdAt: 0, requestState: cortex_tsdb.StateProcessed, cancellationPeriod: time.Second, cancelledFileExists: false, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, "not allowed,request already cancelled": { createdAt: 0, requestState: cortex_tsdb.StateCancelled, cancellationPeriod: time.Second, cancelledFileExists: true, - expectedHttpStatus: http.StatusBadRequest, + expectedHTTPStatus: http.StatusBadRequest, }, } { t.Run(name, func(t *testing.T) { @@ -264,7 +264,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { resp := httptest.NewRecorder() api.CancelDeleteRequestHandler(resp, req.WithContext(ctx)) - require.Equal(t, tc.expectedHttpStatus, resp.Code) + require.Equal(t, tc.expectedHTTPStatus, resp.Code) // check if the cancelled tombstone file exists userBkt := bucket.NewUserBucketClient(userID, bkt, api.cfgProvider) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 61581dae67..d244029b90 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -48,7 +48,7 @@ type Tombstone struct { State BlockDeleteRequestState `json:"-"` } -func NewTombstone(userID string, requestTime int64, stateTime int64, startTime int64, endTime int64, selectors []string, requestId string, state BlockDeleteRequestState) *Tombstone { +func NewTombstone(userID string, requestTime int64, stateTime int64, startTime int64, endTime int64, selectors []string, requestID string, state BlockDeleteRequestState) *Tombstone { return &Tombstone{ RequestCreatedAt: requestTime, StateCreatedAt: stateTime, @@ -56,7 +56,7 @@ func NewTombstone(userID string, requestTime int64, stateTime int64, startTime i EndTime: endTime, Selectors: selectors, UserID: userID, - RequestID: requestId, + RequestID: requestID, State: state, } } @@ -103,15 +103,15 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } -func GetDeleteRequestByIdForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestId string) (*Tombstone, error) { +func GetDeleteRequestByIDForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestID string) (*Tombstone, error) { userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) found := []*Tombstone{} for _, state := range AllDeletionStates { - filename := requestId + "." + string(state) + ".json" + filename := requestID + "." + string(state) + ".json" - exists, err := TombstoneExists(ctx, userBucket, userID, requestId, state) + exists, err := TombstoneExists(ctx, userBucket, userID, requestID, state) if err != nil { return nil, err } @@ -277,12 +277,12 @@ func UpdateTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider return newT, nil } -func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userId string, requestId string, state BlockDeleteRequestState) error { - userLogger := util_log.WithUserID(userId, util_log.Logger) - userBucket := bucket.NewUserBucketClient(userId, bkt, cfgProvider) +func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestID string, state BlockDeleteRequestState) error { + userLogger := util_log.WithUserID(userID, util_log.Logger) + userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) // Create the full path of the tombstone by appending the state as the extension - filename := requestId + "." + string(state) + ".json" + filename := requestID + "." + string(state) + ".json" fullTombstonePath := path.Join(TombstonePath, filename) level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) @@ -291,22 +291,22 @@ func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider b } -func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestId string) error { +func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestID string) error { usrBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) userLogger := util_log.WithUserID(userID, util_log.Logger) - exists, err := TombstoneExists(ctx, usrBkt, userID, requestId, StateCancelled) + exists, err := TombstoneExists(ctx, usrBkt, userID, requestID, StateCancelled) if err != nil { - level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestId, "err", err) + level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestID, "err", err) return err } if exists { - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestId, StateCancelled); err != nil { - level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestId, "err", err) + if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestID, StateCancelled); err != nil { + level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestID, "err", err) return err } - level.Info(userLogger).Log("msg", "Removing tombstone file with previously cancelled state", "requestID", requestId, "err", err) + level.Info(userLogger).Log("msg", "Removing tombstone file with previously cancelled state", "requestID", requestID, "err", err) } return nil diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index c858f8b18a..e0396e62a7 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -164,7 +164,7 @@ func TestGetSingleTombstone(t *testing.T) { require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tPending)) require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tProcessed)) - tRetrieved, err := GetDeleteRequestByIdForUser(ctx, bkt, nil, username, requestID) + tRetrieved, err := GetDeleteRequestByIDForUser(ctx, bkt, nil, username, requestID) require.NoError(t, err) //verify that all the information was read correctly @@ -178,7 +178,7 @@ func TestGetSingleTombstone(t *testing.T) { require.Equal(t, tProcessed.State, tRetrieved.State) // Get single tombstone that doesn't exist should return nil - tRetrieved, err = GetDeleteRequestByIdForUser(ctx, bkt, nil, username, "unknownRequestID") + tRetrieved, err = GetDeleteRequestByIDForUser(ctx, bkt, nil, username, "unknownRequestID") require.NoError(t, err) require.Nil(t, tRetrieved) } From a5e62f09f2fa35a7177dd7410f857d6a3395c3e0 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 19 Jul 2021 15:20:59 -0400 Subject: [PATCH 11/21] fix lint errors and add changelog Signed-off-by: ilangofman --- CHANGELOG.md | 1 + pkg/chunk/purger/blocks_purger_test.go | 7 ++++--- pkg/storage/tsdb/tombstones_test.go | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c09ebb845..0adfd57719 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - `-alertmanager.receivers-firewall.block.cidr-networks` renamed to `-alertmanager.receivers-firewall-block-cidr-networks` - `-alertmanager.receivers-firewall.block.private-addresses` renamed to `-alertmanager.receivers-firewall-block-private-addresses` * [CHANGE] Change default value of `-server.grpc.keepalive.min-time-between-pings` to `10s` and `-server.grpc.keepalive.ping-without-stream-allowed` to `true`. #4168 +* [FEATURE] Block Storage: Added Prometheus style API endpoints for series deletion. Needs to be enabled first by setting `--purger.enable` to `true`. This PR only handles the creating, getting and cancelling requests. Actual deletion and query time filtering will be part of future PRs. #4370 * [FEATURE] Querier: Added new `-querier.max-fetched-series-per-query` flag. When Cortex is running with blocks storage, the max series per query limit is enforced in the querier and applies to unique series received from ingesters and store-gateway (long-term storage). #4179 * [FEATURE] Alertmanager: Added rate-limits to notifiers. Rate limits used by all integrations can be configured using `-alertmanager.notification-rate-limit`, while per-integration rate limits can be specified via `-alertmanager.notification-rate-limit-per-integration` parameter. Both shared and per-integration limits can be overwritten using overrides mechanism. These limits are applied on individual (per-tenant) alertmanagers. Rate-limited notifications are failed notifications. It is possible to monitor rate-limited notifications via new `cortex_alertmanager_notification_rate_limited_total` metric. #4135 #4163 * [FEATURE] Added flag `-debug.block-profile-rate` to enable goroutine blocking events profiling. #4217 diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 5de2a70530..218a0f4380 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -12,13 +12,14 @@ import ( "testing" "time" - "github.com/cortexproject/cortex/pkg/storage/bucket" - "github.com/cortexproject/cortex/pkg/storage/tsdb" - cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" "github.com/go-kit/kit/log" "github.com/stretchr/testify/require" "github.com/thanos-io/thanos/pkg/objstore" "github.com/weaveworks/common/user" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + "github.com/cortexproject/cortex/pkg/storage/tsdb" + cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" ) func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index e0396e62a7..178f523dbb 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -6,10 +6,11 @@ import ( "path" "testing" - "github.com/cortexproject/cortex/pkg/storage/bucket" "github.com/stretchr/testify/require" "github.com/thanos-io/thanos/pkg/objstore" "github.com/weaveworks/common/user" + + "github.com/cortexproject/cortex/pkg/storage/bucket" ) func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { From 4bafb4eaf44fe93a4a7af077a7b6b6b9577ce63c Mon Sep 17 00:00:00 2001 From: ilangofman Date: Fri, 6 Aug 2021 14:09:37 -0400 Subject: [PATCH 12/21] fix Changelog Signed-off-by: ilangofman --- CHANGELOG.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 714ba78afc..0d7cfdee94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,14 +58,6 @@ * [CHANGE] Change default value of `-server.grpc.keepalive.min-time-between-pings` from `5m` to `10s` and `-server.grpc.keepalive.ping-without-stream-allowed` to `true`. #4168 * [CHANGE] Ingester: Change default value of `-ingester.active-series-metrics-enabled` to `true`. This incurs a small increase in memory usage, between 1.2% and 1.6% as measured on ingesters with 1.3M active series. #4257 * [CHANGE] Dependency: update go-redis from v8.2.3 to v8.9.0. #4236 -* [CHANGE] Memberlist: Expose default configuration values to the command line options. Note that setting these explicitly to zero will no longer cause the default to be used. If the default is desired, then do set the option. The following are affected: #4276 - - `-memberlist.stream-timeout` - - `-memberlist.retransmit-factor` - - `-memberlist.pull-push-interval` - - `-memberlist.gossip-interval` - - `-memberlist.gossip-nodes` - - `-memberlist.gossip-to-dead-nodes-time` - - `-memberlist.dead-node-reclaim-time` * [FEATURE] Querier: Added new `-querier.max-fetched-series-per-query` flag. When Cortex is running with blocks storage, the max series per query limit is enforced in the querier and applies to unique series received from ingesters and store-gateway (long-term storage). #4179 * [FEATURE] Querier/Ruler: Added new `-querier.max-fetched-chunk-bytes-per-query` flag. When Cortex is running with blocks storage, the max chunk bytes limit is enforced in the querier and ruler and limits the size of all aggregated chunks returned from ingesters and storage as bytes for a query. #4216 * [FEATURE] Alertmanager: support negative matchers, time-based muting - [upstream release notes](https://github.com/prometheus/alertmanager/releases/tag/v0.22.0). #4237 From 1dfaf28a4e9b6673fbc573c7c7dbecd6af3cb56e Mon Sep 17 00:00:00 2001 From: ilangofman Date: Fri, 6 Aug 2021 14:55:58 -0400 Subject: [PATCH 13/21] Remove extra print statement Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 3f7693994e..2e40346127 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -174,8 +174,6 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r } func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { - fmt.Println("CANCel PERIOD: ", api.deleteRequestCancelPeriod.Milliseconds()) - ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { From 08b4c1ec9908539b70219a64d196ad28761e018c Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 16 Aug 2021 10:44:34 -0700 Subject: [PATCH 14/21] Minor refactor Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 7 ++----- pkg/chunk/purger/blocks_purger_test.go | 4 ++-- pkg/chunk/purger/purger.go | 4 ++-- pkg/cortex/modules.go | 8 ++++---- pkg/storage/tsdb/tombstones.go | 25 +++++++++++++------------ 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 2e40346127..100c499a36 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -206,11 +206,8 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r return } - currentTime := int64(time.Now().Unix() * 1000) - timeElapsed := float64(currentTime - deleteRequest.RequestCreatedAt) - - if timeElapsed > float64(api.deleteRequestCancelPeriod.Milliseconds()) { - http.Error(w, fmt.Sprintf("deletion of request past the deadline of %s since its creation is not allowed", api.deleteRequestCancelPeriod.String()), http.StatusBadRequest) + if time.Since(deleteRequest.GetCreateTime()) > api.deleteRequestCancelPeriod { + http.Error(w, fmt.Sprintf("Cancellation of request past the deadline of %s since its creation is not allowed", api.deleteRequestCancelPeriod.String()), http.StatusBadRequest) return } diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 218a0f4380..054bf53ade 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -132,7 +132,7 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin // should be deleted from the bucket bkt := objstore.NewInMemBucket() - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), time.Minute*5) ctx := context.Background() ctx = user.InjectOrgID(ctx, userID) @@ -237,7 +237,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { } { t.Run(name, func(t *testing.T) { bkt := objstore.NewInMemBucket() - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), tc.cancellationPeriod) ctx := context.Background() ctx = user.InjectOrgID(ctx, userID) diff --git a/pkg/chunk/purger/purger.go b/pkg/chunk/purger/purger.go index faa62b1eba..036ea3ec05 100644 --- a/pkg/chunk/purger/purger.go +++ b/pkg/chunk/purger/purger.go @@ -87,7 +87,7 @@ type deleteRequestWithLogger struct { // Config holds config for chunks Purger type Config struct { - Enable bool `yaml:"enable"` + EnableSeriesDeletion bool `yaml:"enable"` NumWorkers int `yaml:"num_workers"` ObjectStoreType string `yaml:"object_store_type"` DeleteRequestCancelPeriod time.Duration `yaml:"delete_request_cancel_period"` @@ -95,7 +95,7 @@ type Config struct { // RegisterFlags registers CLI flags for Config func (cfg *Config) RegisterFlags(f *flag.FlagSet) { - f.BoolVar(&cfg.Enable, "purger.enable", false, "Enable purger to allow deletion of series. Be aware that Delete series feature is still experimental") + f.BoolVar(&cfg.EnableSeriesDeletion, "purger.enable", false, "Enable purger to allow deletion of series. Be aware that Delete series feature is still experimental") f.IntVar(&cfg.NumWorkers, "purger.num-workers", 2, "Number of workers executing delete plans in parallel") f.StringVar(&cfg.ObjectStoreType, "purger.object-store-type", "", "Name of the object store to use for storing delete plans") f.DurationVar(&cfg.DeleteRequestCancelPeriod, "purger.delete-request-cancel-period", 24*time.Hour, "Allow cancellation of delete request until duration after they are created. Data would be deleted only after delete requests have been older than this duration. Ideally this should be set to at least 24h.") diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index 94c5ff78ad..9cd1f222b3 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -480,7 +480,7 @@ func (t *Cortex) initChunkStore() (serv services.Service, err error) { } func (t *Cortex) initDeleteRequestsStore() (serv services.Service, err error) { - if t.Cfg.Storage.Engine != storage.StorageEngineChunks || !t.Cfg.PurgerConfig.Enable { + if t.Cfg.Storage.Engine != storage.StorageEngineChunks || !t.Cfg.PurgerConfig.EnableSeriesDeletion { // until we need to explicitly enable delete series support we need to do create TombstonesLoader without DeleteStore which acts as noop t.TombstonesLoader = purger.NewTombstonesLoader(nil, nil) @@ -617,7 +617,7 @@ func (t *Cortex) initTableManager() (services.Service, error) { util_log.CheckFatal("initializing bucket client", err) var extraTables []chunk.ExtraTables - if t.Cfg.PurgerConfig.Enable { + if t.Cfg.PurgerConfig.EnableSeriesDeletion { reg := prometheus.WrapRegistererWith( prometheus.Labels{"component": "table-manager-" + DeleteRequestsStore}, prometheus.DefaultRegisterer) @@ -783,7 +783,7 @@ func (t *Cortex) initMemberlistKV() (services.Service, error) { } func (t *Cortex) initChunksPurger() (services.Service, error) { - if t.Cfg.Storage.Engine != storage.StorageEngineChunks || !t.Cfg.PurgerConfig.Enable { + if t.Cfg.Storage.Engine != storage.StorageEngineChunks || !t.Cfg.PurgerConfig.EnableSeriesDeletion { return nil, nil } @@ -813,7 +813,7 @@ func (t *Cortex) initBlocksPurger() (services.Service, error) { return nil, err } - t.API.RegisterBlocksPurger(blockPurger, t.Cfg.PurgerConfig.Enable) + t.API.RegisterBlocksPurger(blockPurger, t.Cfg.PurgerConfig.EnableSeriesDeletion) return nil, nil } diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index d244029b90..074b850747 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -71,10 +71,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, return errors.Wrap(err, "serialize tombstone") } - // Add the state of the request using the filename extension - filename := tombstone.RequestID + "." + string(tombstone.State) + ".json" - - fullTombstonePath := path.Join(TombstonePath, filename) + fullTombstonePath := path.Join(TombstonePath, getTombstoneFileName(tombstone.RequestID, tombstone.State)) // Check if the tombstone already exists for the same state. Could be the case the same request was made // and is already in the middle of deleting series. Creating a new tombstone would restart @@ -90,9 +87,7 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, } func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID string, requestID string, state BlockDeleteRequestState) (bool, error) { - // Add the state of the request using the filename extension - filename := requestID + "." + string(state) + ".json" - fullTombstonePath := path.Join(TombstonePath, filename) + fullTombstonePath := path.Join(TombstonePath, getTombstoneFileName(requestID, state)) exists, err := bkt.Exists(ctx, fullTombstonePath) @@ -109,8 +104,7 @@ func GetDeleteRequestByIDForUser(ctx context.Context, bkt objstore.Bucket, cfgPr found := []*Tombstone{} for _, state := range AllDeletionStates { - filename := requestID + "." + string(state) + ".json" - + filename := getTombstoneFileName(requestID, state) exists, err := TombstoneExists(ctx, userBucket, userID, requestID, state) if err != nil { return nil, err @@ -141,7 +135,7 @@ func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgPr // add all the tombstones to a map and check for duplicates, // if a key exists with the same request ID (but two different states) tombstoneMap := make(map[string]*Tombstone) - err := userBucket.Iter(ctx, "tombstones/", func(s string) error { + err := userBucket.Iter(ctx, TombstonePath, func(s string) error { t, err := readTombstoneFile(ctx, userBucket, userID, s) if err != nil { return err @@ -281,8 +275,7 @@ func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider b userLogger := util_log.WithUserID(userID, util_log.Logger) userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - // Create the full path of the tombstone by appending the state as the extension - filename := requestID + "." + string(state) + ".json" + filename := getTombstoneFileName(requestID, state) fullTombstonePath := path.Join(TombstonePath, filename) level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) @@ -312,6 +305,14 @@ func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, user return nil } +func (t *Tombstone) GetCreateTime() time.Time { + return time.Unix(t.RequestCreatedAt/1000, 0) +} + +func getTombstoneFileName(requestID string, state BlockDeleteRequestState) string { + return requestID + "." + string(state) + ".json" +} + func isValidDeleteRequestState(state BlockDeleteRequestState) bool { switch state { case From b811d2e62979f54bb3761affab42889f67b62884 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 16 Aug 2021 16:05:49 -0700 Subject: [PATCH 15/21] Add a tombstone manager to simplify the creation/getting tombstones Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 18 ++- pkg/chunk/purger/blocks_purger_test.go | 8 +- pkg/storage/tsdb/tombstones.go | 199 ++++++++++++++----------- pkg/storage/tsdb/tombstones_test.go | 45 +++--- 4 files changed, 151 insertions(+), 119 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 100c499a36..ee02b95164 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -116,17 +116,18 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht return } - requestID := getTombstoneRequestID(startTime, endTime, match) + tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) + requestID := getTombstoneRequestID(startTime, endTime, match) // Since the request id is based on a hash of the parameters, there is a possibility that a tombstone could already exist for it // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one - if err := cortex_tsdb.RemoveCancelledStateIfExists(ctx, api.bucketClient, userID, api.cfgProvider, requestID); err != nil { + if err := tManager.RemoveCancelledStateIfExists(ctx, requestID); err != nil { level.Error(util_log.Logger).Log("msg", "removing cancelled tombstone state if it exists", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return } - prevT, err := cortex_tsdb.GetDeleteRequestByIDForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + prevT, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request by id", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -140,7 +141,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht curTime := time.Now().Unix() * 1000 t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestID, cortex_tsdb.StatePending) - if err = cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, t); err != nil { + if err = tManager.WriteTombstoneFile(ctx, t); err != nil { level.Error(util_log.Logger).Log("msg", "error adding delete request to the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -156,8 +157,8 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r http.Error(w, err.Error(), http.StatusUnauthorized) return } - - deleteRequests, err := cortex_tsdb.GetAllDeleteRequestsForUser(ctx, api.bucketClient, api.cfgProvider, userID) + tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) + deleteRequests, err := tManager.GetAllDeleteRequestsForUser(ctx) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete requests from the block store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -184,7 +185,8 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r params := r.URL.Query() requestID := params.Get("request_id") - deleteRequest, err := cortex_tsdb.GetDeleteRequestByIDForUser(ctx, api.bucketClient, api.cfgProvider, userID, requestID) + tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) + deleteRequest, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -212,7 +214,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r } // create file with the cancelled state - _, err = cortex_tsdb.UpdateTombstoneState(ctx, api.bucketClient, api.cfgProvider, deleteRequest, cortex_tsdb.StateCancelled) + _, err = tManager.UpdateTombstoneState(ctx, deleteRequest, cortex_tsdb.StateCancelled) if err != nil { level.Error(util_log.Logger).Log("msg", "error cancelling the delete request", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 054bf53ade..5793dd8c22 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -17,7 +17,6 @@ import ( "github.com/thanos-io/thanos/pkg/objstore" "github.com/weaveworks/common/user" - "github.com/cortexproject/cortex/pkg/storage/bucket" "github.com/cortexproject/cortex/pkg/storage/tsdb" cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" ) @@ -242,9 +241,11 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { ctx := context.Background() ctx = user.InjectOrgID(ctx, userID) + tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, log.NewNopLogger()) + //create the tombstone tombstone := cortex_tsdb.NewTombstone(userID, tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) - err := cortex_tsdb.WriteTombstoneFile(ctx, api.bucketClient, userID, api.cfgProvider, tombstone) + err := tManager.WriteTombstoneFile(ctx, tombstone) require.NoError(t, err) params := url.Values{ @@ -268,8 +269,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { require.Equal(t, tc.expectedHTTPStatus, resp.Code) // check if the cancelled tombstone file exists - userBkt := bucket.NewUserBucketClient(userID, bkt, api.cfgProvider) - exists, _ := cortex_tsdb.TombstoneExists(ctx, userBkt, userID, "request_id", cortex_tsdb.StateCancelled) + exists, _ := tManager.TombstoneExists(ctx, "request_id", cortex_tsdb.StateCancelled) require.Equal(t, tc.cancelledFileExists, exists) }) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 074b850747..fa6c0bb1ab 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -8,6 +8,7 @@ import ( "path/filepath" "time" + "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/pkg/errors" "github.com/prometheus/prometheus/pkg/labels" @@ -32,8 +33,9 @@ const TombstonePath = "tombstones/" var ( ErrTombstoneAlreadyExists = errors.New("The deletion tombstone with the same request information already exists") ErrInvalidDeletionRequestState = errors.New("Deletion request filename extension indicating the state is invalid") - - AllDeletionStates = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} + ErrTombstoneNotFound = errors.New("Tombstone file not found in the object store") + ErrTombstoneDecode = errors.New("Unable to read tombstone contents from file") + AllDeletionStates = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} ) type Tombstone struct { @@ -45,7 +47,7 @@ type Tombstone struct { Selectors []string `json:"selectors"` Matchers []*labels.Matcher `json:"-"` UserID string `json:"user_id"` - State BlockDeleteRequestState `json:"-"` + State BlockDeleteRequestState `json:"state"` } func NewTombstone(userID string, requestTime int64, stateTime int64, startTime int64, endTime int64, selectors []string, requestID string, state BlockDeleteRequestState) *Tombstone { @@ -61,11 +63,26 @@ func NewTombstone(userID string, requestTime int64, stateTime int64, startTime i } } -// Uploads a tombstone file to object sotre -func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, tombstone *Tombstone) error { - userLogger := util_log.WithUserID(userID, util_log.Logger) - userBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) +// TombstoneManager is responsible for reading and writing tombstone files to the bucket. +type TombstoneManager struct { + bkt objstore.InstrumentedBucket + logger log.Logger +} +func NewTombstoneManager( + bkt objstore.Bucket, + userID string, + cfgProvider bucket.TenantConfigProvider, + logger log.Logger) *TombstoneManager { + + return &TombstoneManager{ + bkt: bucket.NewUserBucketClient(userID, bkt, cfgProvider), + logger: util_log.WithUserID(userID, logger), + } +} + +// Uploads a tombstone file to object sotre +func (m *TombstoneManager) WriteTombstoneFile(ctx context.Context, tombstone *Tombstone) error { data, err := json.Marshal(tombstone) if err != nil { return errors.Wrap(err, "serialize tombstone") @@ -76,20 +93,19 @@ func WriteTombstoneFile(ctx context.Context, bkt objstore.Bucket, userID string, // Check if the tombstone already exists for the same state. Could be the case the same request was made // and is already in the middle of deleting series. Creating a new tombstone would restart // the progress - tombstoneExists, err := TombstoneExists(ctx, userBkt, userID, tombstone.RequestID, tombstone.State) + tombstoneExists, err := m.TombstoneExists(ctx, tombstone.RequestID, tombstone.State) if err != nil { - level.Error(userLogger).Log("msg", "unable to check if the same tombstone already exists", "requestID", tombstone.RequestID, "err", err) + level.Error(m.logger).Log("msg", "unable to check if the same tombstone already exists", "requestID", tombstone.RequestID, "err", err) } else if tombstoneExists { return ErrTombstoneAlreadyExists } - return errors.Wrap(userBkt.Upload(ctx, fullTombstonePath, bytes.NewReader(data)), "upload tombstone file") + return errors.Wrap(m.bkt.Upload(ctx, fullTombstonePath, bytes.NewReader(data)), "upload tombstone file") } -func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID string, requestID string, state BlockDeleteRequestState) (bool, error) { +func (m *TombstoneManager) TombstoneExists(ctx context.Context, requestID string, state BlockDeleteRequestState) (bool, error) { fullTombstonePath := path.Join(TombstonePath, getTombstoneFileName(requestID, state)) - - exists, err := bkt.Exists(ctx, fullTombstonePath) + exists, err := m.bkt.Exists(ctx, fullTombstonePath) if exists || err != nil { return exists, err @@ -98,20 +114,18 @@ func TombstoneExists(ctx context.Context, bkt objstore.BucketReader, userID stri return false, nil } -func GetDeleteRequestByIDForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestID string) (*Tombstone, error) { - userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - +func (m *TombstoneManager) GetDeleteRequestByIDForUser(ctx context.Context, requestID string) (*Tombstone, error) { found := []*Tombstone{} for _, state := range AllDeletionStates { filename := getTombstoneFileName(requestID, state) - exists, err := TombstoneExists(ctx, userBucket, userID, requestID, state) + exists, err := m.TombstoneExists(ctx, requestID, state) if err != nil { return nil, err } if exists { - t, err := readTombstoneFile(ctx, userBucket, userID, path.Join(TombstonePath, filename)) + t, err := m.ReadTombstoneFile(ctx, path.Join(TombstonePath, filename)) if err != nil { return nil, err } @@ -129,14 +143,12 @@ func GetDeleteRequestByIDForUser(ctx context.Context, bkt objstore.Bucket, cfgPr } -func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string) ([]*Tombstone, error) { - userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - +func (m *TombstoneManager) GetAllDeleteRequestsForUser(ctx context.Context) ([]*Tombstone, error) { // add all the tombstones to a map and check for duplicates, // if a key exists with the same request ID (but two different states) tombstoneMap := make(map[string]*Tombstone) - err := userBucket.Iter(ctx, TombstonePath, func(s string) error { - t, err := readTombstoneFile(ctx, userBucket, userID, s) + err := m.bkt.Iter(ctx, TombstonePath, func(s string) error { + t, err := m.ReadTombstoneFile(ctx, s) if err != nil { return err } @@ -147,7 +159,7 @@ func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgPr // if there is more than one tombstone for a given request, // we only want to return the latest state. The older file // will be cleaned by the compactor - newT, err := getLatestTombstateByState(t, tombstoneMap[t.RequestID]) + newT, err := m.getLatestTombstateByState(t, tombstoneMap[t.RequestID]) if err != nil { return err } @@ -168,13 +180,13 @@ func GetAllDeleteRequestsForUser(ctx context.Context, bkt objstore.Bucket, cfgPr return deletionRequests, nil } -func getLatestTombstateByState(a *Tombstone, b *Tombstone) (*Tombstone, error) { - orderA, err := a.GetStateOrder() +func (m *TombstoneManager) getLatestTombstateByState(a *Tombstone, b *Tombstone) (*Tombstone, error) { + orderA, err := a.State.GetStateOrder() if err != nil { return nil, err } - orderB, err := b.GetStateOrder() + orderB, err := b.State.GetStateOrder() if err != nil { return nil, err } @@ -186,29 +198,16 @@ func getLatestTombstateByState(a *Tombstone, b *Tombstone) (*Tombstone, error) { return a, nil } -func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID string, tombstonePath string) (*Tombstone, error) { - userLogger := util_log.WithUserID(userID, util_log.Logger) - - // request filename is in format of request_id + "." + state + ".json" - - // This should get the first extension which is .json - filenameExtesion := filepath.Ext(tombstonePath) - filenameWithoutJSON := tombstonePath[0 : len(tombstonePath)-len(filenameExtesion)] - - stateExtension := filepath.Ext(filenameWithoutJSON) - - // Ensure that the state exists as the filename extension - if len(stateExtension) == 0 { - return nil, ErrInvalidDeletionRequestState +func (m *TombstoneManager) ReadTombstoneFile(ctx context.Context, tombstonePath string) (*Tombstone, error) { + _, _, err := GetTombstoneStateAndRequestIDFromPath(tombstonePath) + if err != nil { + return nil, errors.Wrapf(err, "failed to get the requestID and state from filename: %s", tombstonePath) } - state := BlockDeleteRequestState(stateExtension[1:]) - if !isValidDeleteRequestState(state) { - return nil, errors.Wrapf(ErrInvalidDeletionRequestState, "Filename extension is invalid for tombstone: %s", tombstonePath) - + r, err := m.bkt.Get(ctx, tombstonePath) + if m.bkt.IsObjNotFoundErr(err) { + return nil, errors.Wrapf(ErrTombstoneNotFound, "tombstone file not found %s", tombstonePath) } - - r, err := bkt.Get(ctx, tombstonePath) if err != nil { return nil, errors.Wrapf(err, "failed to read tombstone object: %s", tombstonePath) } @@ -218,16 +217,14 @@ func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID st // Close reader before dealing with decode error. if closeErr := r.Close(); closeErr != nil { - level.Warn(userLogger).Log("msg", "failed to close bucket reader", "err", closeErr) + level.Warn(util_log.Logger).Log("msg", "failed to close bucket reader", "err", closeErr) } if err != nil { - return nil, errors.Wrapf(err, "failed to decode tombstone object: %s", tombstonePath) + return nil, errors.Wrapf(ErrTombstoneDecode, "failed to decode tombstone object: %s, err: %v", tombstonePath, err.Error()) } - tombstone.State = BlockDeleteRequestState(state) - - tombstone.Matchers, err = parseMatchers(tombstone.Selectors) + tombstone.Matchers, err = ParseMatchers(tombstone.Selectors) if err != nil { return nil, errors.Wrapf(err, "failed to parse tombstone selectors for: %s", tombstonePath) } @@ -235,76 +232,100 @@ func readTombstoneFile(ctx context.Context, bkt objstore.BucketReader, userID st return tombstone, nil } -func parseMatchers(selectors []string) ([]*labels.Matcher, error) { - // Convert the string selectors to label matchers - var m []*labels.Matcher - - for _, selector := range selectors { - parsed, err := parser.ParseMetricSelector(selector) - if err != nil { - return nil, errors.Wrapf(err, "error parsing metric selector") - } - //keep the matchers in a 1D array because the deletions are applied based - // on the "and" between all matchers. There is no need to - m = append(m, parsed...) - } - - return m, nil -} - -func UpdateTombstoneState(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { +func (m *TombstoneManager) UpdateTombstoneState(ctx context.Context, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { userLogger := util_log.WithUserID(t.UserID, util_log.Logger) // Create the new tombstone, and will delete the previous tombstone newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix()*1000, t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) newT.Matchers = t.Matchers - err := WriteTombstoneFile(ctx, bkt, newT.UserID, cfgProvider, newT) + err := m.WriteTombstoneFile(ctx, newT) if err != nil { level.Error(userLogger).Log("msg", "error creating file tombstone file with the updated state", "err", err) return nil, err } - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, t.UserID, t.RequestID, t.State); err != nil { + if err = m.DeleteTombstoneFile(ctx, t.RequestID, t.State); err != nil { level.Error(userLogger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) } return newT, nil } -func DeleteTombstoneFile(ctx context.Context, bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, userID string, requestID string, state BlockDeleteRequestState) error { - userLogger := util_log.WithUserID(userID, util_log.Logger) - userBucket := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - +func (m *TombstoneManager) DeleteTombstoneFile(ctx context.Context, requestID string, state BlockDeleteRequestState) error { filename := getTombstoneFileName(requestID, state) fullTombstonePath := path.Join(TombstonePath, filename) - level.Info(userLogger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) + level.Info(m.logger).Log("msg", "Deleting tombstone file", "file", fullTombstonePath) - return errors.Wrap(userBucket.Delete(ctx, fullTombstonePath), "delete tombstone file") + return errors.Wrap(m.bkt.Delete(ctx, fullTombstonePath), "delete tombstone file") } -func RemoveCancelledStateIfExists(ctx context.Context, bkt objstore.Bucket, userID string, cfgProvider bucket.TenantConfigProvider, requestID string) error { - usrBkt := bucket.NewUserBucketClient(userID, bkt, cfgProvider) - userLogger := util_log.WithUserID(userID, util_log.Logger) - - exists, err := TombstoneExists(ctx, usrBkt, userID, requestID, StateCancelled) +func (m *TombstoneManager) RemoveCancelledStateIfExists(ctx context.Context, requestID string) error { + exists, err := m.TombstoneExists(ctx, requestID, StateCancelled) if err != nil { - level.Error(userLogger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestID, "err", err) + level.Error(m.logger).Log("msg", "unable to check if the request has previously been cancelled", "requestID", requestID, "err", err) return err } if exists { - if err = DeleteTombstoneFile(ctx, bkt, cfgProvider, userID, requestID, StateCancelled); err != nil { - level.Error(userLogger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestID, "err", err) + if err = m.DeleteTombstoneFile(ctx, requestID, StateCancelled); err != nil { + level.Error(m.logger).Log("msg", "unable to delete tombstone with previously cancelled state", "requestID", requestID, "err", err) return err } - level.Info(userLogger).Log("msg", "Removing tombstone file with previously cancelled state", "requestID", requestID, "err", err) + level.Info(m.logger).Log("msg", "Removing tombstone file with previously cancelled state", "requestID", requestID, "err", err) } return nil } +func GetTombstoneStateAndRequestIDFromPath(tombstonePath string) (string, BlockDeleteRequestState, error) { + // This should get the first extension which is .json + filenameExtesion := filepath.Ext(tombstonePath) + filenameWithoutJSON := tombstonePath[0 : len(tombstonePath)-len(filenameExtesion)] + + stateExtension := filepath.Ext(filenameWithoutJSON) + requestID := filenameWithoutJSON[0 : len(filenameWithoutJSON)-len(stateExtension)] + + // Ensure that the state exists as the filename extension + if len(stateExtension) == 0 { + return "", "", ErrInvalidDeletionRequestState + } + + state := BlockDeleteRequestState(stateExtension[1:]) + if !isValidDeleteRequestState(state) { + return "", "", errors.Wrapf(ErrInvalidDeletionRequestState, "Filename extension is invalid for tombstone: %s", tombstonePath) + + } + + return requestID, state, nil +} + +func ParseMatchers(selectors []string) ([]*labels.Matcher, error) { + // Convert the string selectors to label matchers + var m []*labels.Matcher + + for _, selector := range selectors { + parsed, err := parser.ParseMetricSelector(selector) + if err != nil { + return nil, errors.Wrapf(err, "error parsing metric selector") + } + //keep the matchers in a 1D array because the deletions are applied based + // on the "and" between all matchers. There is no need to + m = append(m, parsed...) + } + + return m, nil +} + +func (t *Tombstone) GetFilename() string { + return t.RequestID + "." + string(t.State) + ".json" +} + +func (t *Tombstone) IsOverlappingInterval(minT int64, maxT int64) bool { + return t.StartTime <= maxT && minT < t.EndTime +} + func (t *Tombstone) GetCreateTime() time.Time { return time.Unix(t.RequestCreatedAt/1000, 0) } @@ -324,8 +345,8 @@ func isValidDeleteRequestState(state BlockDeleteRequestState) bool { return false } -func (t *Tombstone) GetStateOrder() (int, error) { - switch t.State { +func (s BlockDeleteRequestState) GetStateOrder() (int, error) { + switch s { case StatePending: return 0, nil case StateProcessed: diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 178f523dbb..3238f19bd1 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -6,11 +6,10 @@ import ( "path" "testing" + "github.com/go-kit/kit/log" "github.com/stretchr/testify/require" "github.com/thanos-io/thanos/pkg/objstore" "github.com/weaveworks/common/user" - - "github.com/cortexproject/cortex/pkg/storage/bucket" ) func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { @@ -23,9 +22,11 @@ func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { ctx := context.Background() ctx = user.InjectOrgID(ctx, username) + tManager := NewTombstoneManager(bkt, username, nil, log.NewNopLogger()) + //create the tombstone tombstone := NewTombstone(username, 0, 0, 0, 1, []string{"match"}, requestID, StatePending) - err := WriteTombstoneFile(ctx, bkt, username, nil, tombstone) + err := tManager.WriteTombstoneFile(ctx, tombstone) require.NoError(t, err) filename := requestID + "." + string(StatePending) + ".json" @@ -33,7 +34,7 @@ func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { require.True(t, exists) // Creating the same tombstone twice should result in an error - err = WriteTombstoneFile(ctx, bkt, username, nil, tombstone) + err = tManager.WriteTombstoneFile(ctx, tombstone) require.ErrorIs(t, err, ErrTombstoneAlreadyExists) } @@ -79,10 +80,9 @@ func TestTombstonesExists(t *testing.T) { for objName, data := range tc.objects { require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) } + tManager := NewTombstoneManager(bkt, username, nil, log.NewNopLogger()) - userBkt := bucket.NewUserBucketClient(username, bkt, nil) - - res, err := TombstoneExists(ctx, userBkt, username, requestID, tc.targetRequestState) + res, err := tManager.TombstoneExists(ctx, requestID, tc.targetRequestState) require.NoError(t, err) require.Equal(t, tc.exists, res) }) @@ -106,7 +106,8 @@ func TestTombstonesDeletion(t *testing.T) { require.NoError(t, bkt.Upload(context.Background(), tPendingPath, bytes.NewReader([]byte("data")))) require.NoError(t, bkt.Upload(context.Background(), tProcessedPath, bytes.NewReader([]byte("data")))) - require.NoError(t, DeleteTombstoneFile(ctx, bkt, nil, tPending.UserID, tPending.RequestID, tPending.State)) + tManager := NewTombstoneManager(bkt, tPending.UserID, nil, log.NewNopLogger()) + require.NoError(t, tManager.DeleteTombstoneFile(ctx, tPending.RequestID, tPending.State)) // make sure the pending tombstone was deleted exists, _ := bkt.Exists(ctx, tPendingPath) @@ -129,10 +130,12 @@ func TestTombstoneUpdateState(t *testing.T) { tPendingPath := username + "/tombstones/" + requestID + "." + string(StatePending) + ".json" bkt := objstore.NewInMemBucket() + tManager := NewTombstoneManager(bkt, username, nil, log.NewNopLogger()) + // "upload" sample tombstone file require.NoError(t, bkt.Upload(context.Background(), tPendingPath, bytes.NewReader([]byte("data")))) - tProcessed, err := UpdateTombstoneState(ctx, bkt, nil, tPending, StateProcessed) + tProcessed, err := tManager.UpdateTombstoneState(ctx, tPending, StateProcessed) require.NoError(t, err) // make sure the pending tombstone was deleted @@ -161,11 +164,13 @@ func TestGetSingleTombstone(t *testing.T) { tProcessed := NewTombstone(username, 10, 20, 30, 60, []string{"node_exporter"}, requestID, StateProcessed) bkt := objstore.NewInMemBucket() + tManager := NewTombstoneManager(bkt, tPending.UserID, nil, log.NewNopLogger()) + // first add the tombstone files to the object store - require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tPending)) - require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, tProcessed)) + require.NoError(t, tManager.WriteTombstoneFile(ctx, tPending)) + require.NoError(t, tManager.WriteTombstoneFile(ctx, tProcessed)) - tRetrieved, err := GetDeleteRequestByIDForUser(ctx, bkt, nil, username, requestID) + tRetrieved, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) require.NoError(t, err) //verify that all the information was read correctly @@ -179,7 +184,7 @@ func TestGetSingleTombstone(t *testing.T) { require.Equal(t, tProcessed.State, tRetrieved.State) // Get single tombstone that doesn't exist should return nil - tRetrieved, err = GetDeleteRequestByIDForUser(ctx, bkt, nil, username, "unknownRequestID") + tRetrieved, err = tManager.GetDeleteRequestByIDForUser(ctx, "unknownRequestID") require.NoError(t, err) require.Nil(t, tRetrieved) } @@ -190,6 +195,8 @@ func TestGetAllTombstones(t *testing.T) { ctx = user.InjectOrgID(ctx, username) bkt := objstore.NewInMemBucket() + tManager := NewTombstoneManager(bkt, username, nil, log.NewNopLogger()) + tombstonesInput := []*Tombstone{ NewTombstone(username, 0, 0, 0, 0, []string{}, "request1", StatePending), NewTombstone(username, 0, 0, 0, 0, []string{}, "request1", StateCancelled), @@ -212,10 +219,10 @@ func TestGetAllTombstones(t *testing.T) { // add all tombstones to the bkt for _, ts := range tombstonesInput { - require.NoError(t, WriteTombstoneFile(ctx, bkt, username, nil, ts)) + require.NoError(t, tManager.WriteTombstoneFile(ctx, ts)) } - tombstonesOutput, err := GetAllDeleteRequestsForUser(ctx, bkt, nil, username) + tombstonesOutput, err := tManager.GetAllDeleteRequestsForUser(ctx) require.NoError(t, err) outputMap := make(map[string]BlockDeleteRequestState) @@ -238,23 +245,25 @@ func TestTombstoneReadWithInvalidFileName(t *testing.T) { ctx := context.Background() ctx = user.InjectOrgID(ctx, username) + tManager := NewTombstoneManager(bkt, username, nil, log.NewNopLogger()) + { tInvalidPath := username + "/tombstones/" + requestID + "." + string(StatePending) - _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } { tInvalidPath := username + "/tombstones/" + requestID - _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } { tInvalidPath := username + "/tombstones/" + requestID + ".json." + string(StatePending) - _, err := readTombstoneFile(ctx, bkt, username, tInvalidPath) + _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } From a0524cdaab5da448aeb60c6539bb1d9305189121 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 16 Aug 2021 22:10:16 -0700 Subject: [PATCH 16/21] remove unneeded variable Signed-off-by: ilangofman --- pkg/storage/tsdb/tombstones.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index fa6c0bb1ab..2e129d87cd 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -233,19 +233,18 @@ func (m *TombstoneManager) ReadTombstoneFile(ctx context.Context, tombstonePath } func (m *TombstoneManager) UpdateTombstoneState(ctx context.Context, t *Tombstone, newState BlockDeleteRequestState) (*Tombstone, error) { - userLogger := util_log.WithUserID(t.UserID, util_log.Logger) // Create the new tombstone, and will delete the previous tombstone newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix()*1000, t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) newT.Matchers = t.Matchers err := m.WriteTombstoneFile(ctx, newT) if err != nil { - level.Error(userLogger).Log("msg", "error creating file tombstone file with the updated state", "err", err) + level.Error(m.logger).Log("msg", "error creating file tombstone file with the updated state", "err", err) return nil, err } if err = m.DeleteTombstoneFile(ctx, t.RequestID, t.State); err != nil { - level.Error(userLogger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) + level.Error(m.logger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) } return newT, nil From 3b0ec4920a342bfed23257e55079d81f8a2dea8f Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 16 Aug 2021 22:37:51 -0700 Subject: [PATCH 17/21] fix comment Signed-off-by: ilangofman --- pkg/storage/tsdb/tombstones.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 2e129d87cd..2edb9725f8 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -279,6 +279,8 @@ func (m *TombstoneManager) RemoveCancelledStateIfExists(ctx context.Context, req } func GetTombstoneStateAndRequestIDFromPath(tombstonePath string) (string, BlockDeleteRequestState, error) { + // The filename of the request should be ..json + // This should get the first extension which is .json filenameExtesion := filepath.Ext(tombstonePath) filenameWithoutJSON := tombstonePath[0 : len(tombstonePath)-len(filenameExtesion)] @@ -309,8 +311,8 @@ func ParseMatchers(selectors []string) ([]*labels.Matcher, error) { if err != nil { return nil, errors.Wrapf(err, "error parsing metric selector") } - //keep the matchers in a 1D array because the deletions are applied based - // on the "and" between all matchers. There is no need to + //keep the matchers in a 1D slice because the deletions are applied based + // on the "and" between all matchers. m = append(m, parsed...) } From 212f876c5605f7c9b45e9024eb1d87fc8e70f5e7 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Wed, 18 Aug 2021 17:20:04 -0700 Subject: [PATCH 18/21] Address PR comments Signed-off-by: ilangofman --- pkg/chunk/purger/blocks_purger.go | 41 ++++++----- pkg/chunk/purger/blocks_purger_test.go | 4 +- pkg/storage/tsdb/tombstones.go | 96 +++++++++++++------------- pkg/storage/tsdb/tombstones_test.go | 22 +++--- 4 files changed, 81 insertions(+), 82 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index ee02b95164..2aa70a3fbb 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -3,12 +3,12 @@ package purger import ( "context" "crypto/md5" + "encoding/binary" "encoding/hex" "encoding/json" fmt "fmt" "net/http" "sort" - "strconv" strings "strings" "time" @@ -118,7 +118,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) - requestID := getTombstoneRequestID(startTime, endTime, match) + requestID := getTombstoneHash(startTime, endTime, match) // Since the request id is based on a hash of the parameters, there is a possibility that a tombstone could already exist for it // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one if err := tManager.RemoveCancelledStateIfExists(ctx, requestID); err != nil { @@ -127,7 +127,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht return } - prevT, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) + prevT, err := tManager.GetTombstoneByIDForUser(ctx, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request by id", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -141,7 +141,7 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht curTime := time.Now().Unix() * 1000 t := cortex_tsdb.NewTombstone(userID, curTime, curTime, startTime, endTime, match, requestID, cortex_tsdb.StatePending) - if err = tManager.WriteTombstoneFile(ctx, t); err != nil { + if err = tManager.WriteTombstone(ctx, t); err != nil { level.Error(util_log.Logger).Log("msg", "error adding delete request to the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -158,7 +158,7 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r return } tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) - deleteRequests, err := tManager.GetAllDeleteRequestsForUser(ctx) + deleteRequests, err := tManager.GetAllTombstonesForUser(ctx) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete requests from the block store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -186,7 +186,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r requestID := params.Get("request_id") tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) - deleteRequest, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) + deleteRequest, err := tManager.GetTombstoneByIDForUser(ctx, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the object store", "err", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -224,26 +224,25 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r w.WriteHeader(http.StatusNoContent) } -// Calculates the tombstone file name based on a hash of the start time, end time and selectors -func getTombstoneRequestID(startTime int64, endTime int64, selectors []string) string { - // First make a string of the tombstone info - var b strings.Builder - b.WriteString(strconv.FormatInt(startTime, 10)) - b.WriteString(",") - b.WriteString(strconv.FormatInt(endTime, 10)) +func getTombstoneHash(startTime int64, endTime int64, selectors []string) string { + // Any delete request with the same start, end time and same selectors should result in the same hash + + hash := md5.New() + + bufStart := make([]byte, 8) + binary.LittleEndian.PutUint64(bufStart, uint64(startTime)) + bufEnd := make([]byte, 8) + binary.LittleEndian.PutUint64(bufEnd, uint64(startTime)) + + hash.Write(bufStart) + hash.Write(bufEnd) sort.Strings(selectors) for _, s := range selectors { - b.WriteString(",") - b.WriteString(s) + hash.Write([]byte(s)) } - return getTombstoneHash(b.String()) -} - -func getTombstoneHash(s string) string { - data := []byte(s) - md5Bytes := md5.Sum(data) + md5Bytes := md5.Sum(nil) return hex.EncodeToString(md5Bytes[:]) } diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 5793dd8c22..5a98711844 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -160,7 +160,7 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin require.Equal(t, http.StatusNoContent, resp.Code) //cancel the previous request - requestID := getTombstoneRequestID(1000, 2000, []string{"node_exporter"}) + requestID := getTombstoneHash(1000, 2000, []string{"node_exporter"}) paramsDelete := url.Values{ "request_id": []string{requestID}, } @@ -245,7 +245,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { //create the tombstone tombstone := cortex_tsdb.NewTombstone(userID, tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) - err := tManager.WriteTombstoneFile(ctx, tombstone) + err := tManager.WriteTombstone(ctx, tombstone) require.NoError(t, err) params := url.Values{ diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 2edb9725f8..18d3213826 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -32,8 +32,8 @@ const TombstonePath = "tombstones/" var ( ErrTombstoneAlreadyExists = errors.New("The deletion tombstone with the same request information already exists") - ErrInvalidDeletionRequestState = errors.New("Deletion request filename extension indicating the state is invalid") - ErrTombstoneNotFound = errors.New("Tombstone file not found in the object store") + ErrInvalidDeletionRequestState = errors.New("Tombstone filename extension indicating the deletion request state is invalid") + ErrTombstoneNotFound = errors.New("Tombstone not found in the object store") ErrTombstoneDecode = errors.New("Unable to read tombstone contents from file") AllDeletionStates = []BlockDeleteRequestState{StatePending, StateProcessed, StateCancelled} ) @@ -81,8 +81,8 @@ func NewTombstoneManager( } } -// Uploads a tombstone file to object sotre -func (m *TombstoneManager) WriteTombstoneFile(ctx context.Context, tombstone *Tombstone) error { +// Uploads a tombstone to object sotre +func (m *TombstoneManager) WriteTombstone(ctx context.Context, tombstone *Tombstone) error { data, err := json.Marshal(tombstone) if err != nil { return errors.Wrap(err, "serialize tombstone") @@ -100,7 +100,7 @@ func (m *TombstoneManager) WriteTombstoneFile(ctx context.Context, tombstone *To return ErrTombstoneAlreadyExists } - return errors.Wrap(m.bkt.Upload(ctx, fullTombstonePath, bytes.NewReader(data)), "upload tombstone file") + return errors.Wrap(m.bkt.Upload(ctx, fullTombstonePath, bytes.NewReader(data)), "upload tombstone") } func (m *TombstoneManager) TombstoneExists(ctx context.Context, requestID string, state BlockDeleteRequestState) (bool, error) { @@ -114,7 +114,7 @@ func (m *TombstoneManager) TombstoneExists(ctx context.Context, requestID string return false, nil } -func (m *TombstoneManager) GetDeleteRequestByIDForUser(ctx context.Context, requestID string) (*Tombstone, error) { +func (m *TombstoneManager) GetTombstoneByIDForUser(ctx context.Context, requestID string) (*Tombstone, error) { found := []*Tombstone{} for _, state := range AllDeletionStates { @@ -125,7 +125,7 @@ func (m *TombstoneManager) GetDeleteRequestByIDForUser(ctx context.Context, requ } if exists { - t, err := m.ReadTombstoneFile(ctx, path.Join(TombstonePath, filename)) + t, err := m.ReadTombstone(ctx, path.Join(TombstonePath, filename)) if err != nil { return nil, err } @@ -138,68 +138,68 @@ func (m *TombstoneManager) GetDeleteRequestByIDForUser(ctx context.Context, requ } // If there are multiple tombstones with the same request id but different state, want to return only the latest one - // The older states will be cleaned up by the compactor + // The older states will be cleaned up by the compactor (TODO future PR). return found[len(found)-1], nil } -func (m *TombstoneManager) GetAllDeleteRequestsForUser(ctx context.Context) ([]*Tombstone, error) { - // add all the tombstones to a map and check for duplicates, - // if a key exists with the same request ID (but two different states) - tombstoneMap := make(map[string]*Tombstone) +func (m *TombstoneManager) GetAllTombstonesForUser(ctx context.Context) ([]*Tombstone, error) { + // add all the tombstones to a map and check for duplicates: if a key exists with the same request ID (but two different states). + discovered := make(map[string]BlockDeleteRequestState) err := m.bkt.Iter(ctx, TombstonePath, func(s string) error { - t, err := m.ReadTombstoneFile(ctx, s) + requestID, state, err := GetStateAndRequestIDFromTombstonePath(filepath.Base(s)) if err != nil { return err } - if _, exists := tombstoneMap[t.RequestID]; !exists { - tombstoneMap[t.RequestID] = t + if prevState, exists := discovered[requestID]; !exists { + discovered[requestID] = state } else { - // if there is more than one tombstone for a given request, - // we only want to return the latest state. The older file - // will be cleaned by the compactor - newT, err := m.getLatestTombstateByState(t, tombstoneMap[t.RequestID]) + // If there is more than one tombstone for a given request, we only want the latest state. + // States can move only in a specific direction, so the later state will always win. + orderA, err := state.GetStateOrder() if err != nil { return err } - tombstoneMap[t.RequestID] = newT + orderB, err := prevState.GetStateOrder() + if err != nil { + return err + } + + // If the newer state found comes later in the order, then we replace the state in the map. + if orderA > orderB { + discovered[requestID] = state + } } return nil }) - - if err != nil { - return nil, err - } - - deletionRequests := []*Tombstone{} - for _, t := range tombstoneMap { - deletionRequests = append(deletionRequests, t) - } - - return deletionRequests, nil -} - -func (m *TombstoneManager) getLatestTombstateByState(a *Tombstone, b *Tombstone) (*Tombstone, error) { - orderA, err := a.State.GetStateOrder() - if err != nil { - return nil, err - } - - orderB, err := b.State.GetStateOrder() if err != nil { return nil, err } - if orderB > orderA { - return b, nil + out := []*Tombstone{} + for id, state := range discovered { + filename := getTombstoneFileName(id, state) + t, err := m.ReadTombstone(ctx, path.Join(TombstonePath, filename)) + if errors.Is(err, ErrTombstoneNotFound) { + // This could happen if the tombstone state changes and the tombstone file is deleted in between the "list objects" and now. + level.Warn(m.logger).Log("msg", "skipped missing tombstone file when loading all the tombstones", "requestID", id, "state", string(state)) + continue + } + if errors.Is(err, ErrTombstoneDecode) { + level.Error(m.logger).Log("msg", "skipped corrupted tombstone file when loading all the tombstones", "requestID", id, "state", state, "err", err) + continue + } + if err != nil { + return nil, err + } + out = append(out, t) } - - return a, nil + return out, nil } -func (m *TombstoneManager) ReadTombstoneFile(ctx context.Context, tombstonePath string) (*Tombstone, error) { - _, _, err := GetTombstoneStateAndRequestIDFromPath(tombstonePath) +func (m *TombstoneManager) ReadTombstone(ctx context.Context, tombstonePath string) (*Tombstone, error) { + _, _, err := GetStateAndRequestIDFromTombstonePath(tombstonePath) if err != nil { return nil, errors.Wrapf(err, "failed to get the requestID and state from filename: %s", tombstonePath) } @@ -237,7 +237,7 @@ func (m *TombstoneManager) UpdateTombstoneState(ctx context.Context, t *Tombston newT := NewTombstone(t.UserID, t.RequestCreatedAt, time.Now().Unix()*1000, t.StartTime, t.EndTime, t.Selectors, t.RequestID, newState) newT.Matchers = t.Matchers - err := m.WriteTombstoneFile(ctx, newT) + err := m.WriteTombstone(ctx, newT) if err != nil { level.Error(m.logger).Log("msg", "error creating file tombstone file with the updated state", "err", err) return nil, err @@ -278,7 +278,7 @@ func (m *TombstoneManager) RemoveCancelledStateIfExists(ctx context.Context, req return nil } -func GetTombstoneStateAndRequestIDFromPath(tombstonePath string) (string, BlockDeleteRequestState, error) { +func GetStateAndRequestIDFromTombstonePath(tombstonePath string) (string, BlockDeleteRequestState, error) { // The filename of the request should be ..json // This should get the first extension which is .json diff --git a/pkg/storage/tsdb/tombstones_test.go b/pkg/storage/tsdb/tombstones_test.go index 3238f19bd1..b7ec019f38 100644 --- a/pkg/storage/tsdb/tombstones_test.go +++ b/pkg/storage/tsdb/tombstones_test.go @@ -26,7 +26,7 @@ func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { //create the tombstone tombstone := NewTombstone(username, 0, 0, 0, 1, []string{"match"}, requestID, StatePending) - err := tManager.WriteTombstoneFile(ctx, tombstone) + err := tManager.WriteTombstone(ctx, tombstone) require.NoError(t, err) filename := requestID + "." + string(StatePending) + ".json" @@ -34,7 +34,7 @@ func TestTombstones_WritingSameTombstoneTwiceShouldFail(t *testing.T) { require.True(t, exists) // Creating the same tombstone twice should result in an error - err = tManager.WriteTombstoneFile(ctx, tombstone) + err = tManager.WriteTombstone(ctx, tombstone) require.ErrorIs(t, err, ErrTombstoneAlreadyExists) } @@ -167,10 +167,10 @@ func TestGetSingleTombstone(t *testing.T) { tManager := NewTombstoneManager(bkt, tPending.UserID, nil, log.NewNopLogger()) // first add the tombstone files to the object store - require.NoError(t, tManager.WriteTombstoneFile(ctx, tPending)) - require.NoError(t, tManager.WriteTombstoneFile(ctx, tProcessed)) + require.NoError(t, tManager.WriteTombstone(ctx, tPending)) + require.NoError(t, tManager.WriteTombstone(ctx, tProcessed)) - tRetrieved, err := tManager.GetDeleteRequestByIDForUser(ctx, requestID) + tRetrieved, err := tManager.GetTombstoneByIDForUser(ctx, requestID) require.NoError(t, err) //verify that all the information was read correctly @@ -184,7 +184,7 @@ func TestGetSingleTombstone(t *testing.T) { require.Equal(t, tProcessed.State, tRetrieved.State) // Get single tombstone that doesn't exist should return nil - tRetrieved, err = tManager.GetDeleteRequestByIDForUser(ctx, "unknownRequestID") + tRetrieved, err = tManager.GetTombstoneByIDForUser(ctx, "unknownRequestID") require.NoError(t, err) require.Nil(t, tRetrieved) } @@ -219,10 +219,10 @@ func TestGetAllTombstones(t *testing.T) { // add all tombstones to the bkt for _, ts := range tombstonesInput { - require.NoError(t, tManager.WriteTombstoneFile(ctx, ts)) + require.NoError(t, tManager.WriteTombstone(ctx, ts)) } - tombstonesOutput, err := tManager.GetAllDeleteRequestsForUser(ctx) + tombstonesOutput, err := tManager.GetAllTombstonesForUser(ctx) require.NoError(t, err) outputMap := make(map[string]BlockDeleteRequestState) @@ -249,21 +249,21 @@ func TestTombstoneReadWithInvalidFileName(t *testing.T) { { tInvalidPath := username + "/tombstones/" + requestID + "." + string(StatePending) - _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) + _, err := tManager.ReadTombstone(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } { tInvalidPath := username + "/tombstones/" + requestID - _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) + _, err := tManager.ReadTombstone(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } { tInvalidPath := username + "/tombstones/" + requestID + ".json." + string(StatePending) - _, err := tManager.ReadTombstoneFile(ctx, tInvalidPath) + _, err := tManager.ReadTombstone(ctx, tInvalidPath) require.ErrorIs(t, err, ErrInvalidDeletionRequestState) } From 7d6963d93013c77d2f6e924c2883a860b42cbbd9 Mon Sep 17 00:00:00 2001 From: ilangofman Date: Mon, 20 Sep 2021 08:33:05 -0400 Subject: [PATCH 19/21] reverse change of combining blocks purger and tenant deletion Signed-off-by: ilangofman --- pkg/api/api.go | 27 ++-- pkg/chunk/purger/blocks_purger.go | 91 +------------ pkg/chunk/purger/blocks_purger_test.go | 76 ----------- pkg/chunk/purger/tenant_deletion_api.go | 128 +++++++++++++++++++ pkg/chunk/purger/tenant_deletion_api_test.go | 90 +++++++++++++ pkg/cortex/modules.go | 23 +++- 6 files changed, 253 insertions(+), 182 deletions(-) create mode 100644 pkg/chunk/purger/tenant_deletion_api.go create mode 100644 pkg/chunk/purger/tenant_deletion_api_test.go diff --git a/pkg/api/api.go b/pkg/api/api.go index 6c9f817651..2bee440808 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -279,21 +279,22 @@ func (a *API) RegisterChunksPurger(store *purger.DeleteStore, deleteRequestCance a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(deleteRequestHandler.CancelDeleteRequestHandler), true, "PUT", "POST") } -func (a *API) RegisterBlocksPurger(blocksPurger *purger.BlocksPurgerAPI, seriesDeletionEnabled bool) { +func (a *API) RegisterTenantDeletion(api *purger.TenantDeletionAPI) { + a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(api.DeleteTenant), true, "POST") + a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(api.DeleteTenantStatus), true, "GET") +} - if seriesDeletionEnabled { - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.GetAllDeleteRequestsHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") +func (a *API) RegisterBlocksPurger(blocksPurger *purger.BlocksPurgerAPI) { + + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.GetAllDeleteRequestsHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") + + // Legacy Routes + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "GET") + a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") - // Legacy Routes - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "PUT", "POST") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.AddDeleteRequestHandler), true, "GET") - a.RegisterRoute(path.Join(a.cfg.LegacyHTTPPrefix, "/api/v1/admin/tsdb/cancel_delete_request"), http.HandlerFunc(blocksPurger.CancelDeleteRequestHandler), true, "PUT", "POST") - } - // Tenant Deletion - a.RegisterRoute("/purger/delete_tenant", http.HandlerFunc(blocksPurger.DeleteTenant), true, "POST") - a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(blocksPurger.DeleteTenantStatus), true, "GET") } // RegisterRuler registers routes associated with the Ruler service. diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 2aa70a3fbb..2d4dea9993 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -1,7 +1,6 @@ package purger import ( - "context" "crypto/md5" "encoding/binary" "encoding/hex" @@ -9,13 +8,10 @@ import ( fmt "fmt" "net/http" "sort" - strings "strings" "time" "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" - "github.com/oklog/ulid" - "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" "github.com/prometheus/prometheus/promql/parser" @@ -36,7 +32,7 @@ type BlocksPurgerAPI struct { } func NewBlocksPurgerAPI(storageCfg cortex_tsdb.BlocksStorageConfig, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer, cancellationPeriod time.Duration) (*BlocksPurgerAPI, error) { - bucketClient, err := createBucketClient(storageCfg, logger, reg) + bucketClient, err := createBucketClient(storageCfg, logger, "blocks-purger", reg) if err != nil { return nil, err } @@ -53,15 +49,6 @@ func newBlocksPurgerAPI(bkt objstore.Bucket, cfgProvider bucket.TenantConfigProv } } -func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, reg prometheus.Registerer) (objstore.Bucket, error) { - bucketClient, err := bucket.NewClient(context.Background(), cfg.Bucket, "purger", logger, reg) - if err != nil { - return nil, errors.Wrap(err, "create bucket client") - } - - return bucketClient, nil -} - func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *http.Request) { ctx := r.Context() userID, err := tenant.TenantID(ctx) @@ -245,79 +232,3 @@ func getTombstoneHash(startTime int64, endTime int64, selectors []string) string md5Bytes := md5.Sum(nil) return hex.EncodeToString(md5Bytes[:]) } - -func (api *BlocksPurgerAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - userID, err := tenant.TenantID(ctx) - if err != nil { - // When Cortex is running, it uses Auth Middleware for checking X-Scope-OrgID and injecting tenant into context. - // Auth Middleware sends http.StatusUnauthorized if X-Scope-OrgID is missing, so we do too here, for consistency. - http.Error(w, err.Error(), http.StatusUnauthorized) - return - } - - err = cortex_tsdb.WriteTenantDeletionMark(r.Context(), api.bucketClient, userID, api.cfgProvider, cortex_tsdb.NewTenantDeletionMark(time.Now())) - if err != nil { - level.Error(api.logger).Log("msg", "failed to write tenant deletion mark", "user", userID, "err", err) - - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - level.Info(api.logger).Log("msg", "tenant deletion mark in blocks storage created", "user", userID) - - w.WriteHeader(http.StatusOK) -} - -type DeleteTenantStatusResponse struct { - TenantID string `json:"tenant_id"` - BlocksDeleted bool `json:"blocks_deleted"` -} - -func (api *BlocksPurgerAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - userID, err := tenant.TenantID(ctx) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - - result := DeleteTenantStatusResponse{} - result.TenantID = userID - result.BlocksDeleted, err = api.isBlocksForUserDeleted(ctx, userID) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - util.WriteJSONResponse(w, result) -} - -func (api *BlocksPurgerAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { - var errBlockFound = errors.New("block found") - - userBucket := bucket.NewUserBucketClient(userID, api.bucketClient, api.cfgProvider) - err := userBucket.Iter(ctx, "", func(s string) error { - s = strings.TrimSuffix(s, "/") - - _, err := ulid.Parse(s) - if err != nil { - // not block, keep looking - return nil - } - - // Used as shortcut to stop iteration. - return errBlockFound - }) - - if errors.Is(err, errBlockFound) { - return false, nil - } - - if err != nil { - return false, err - } - - // No blocks found, all good. - return true, nil -} diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index 5a98711844..d4d79684c6 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -1,13 +1,11 @@ package purger import ( - "bytes" "context" math "math" "net/http" "net/http/httptest" "net/url" - "path" "strconv" "testing" "time" @@ -17,7 +15,6 @@ import ( "github.com/thanos-io/thanos/pkg/objstore" "github.com/weaveworks/common/user" - "github.com/cortexproject/cortex/pkg/storage/tsdb" cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" ) @@ -275,76 +272,3 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { }) } } - -func TestDeleteTenant(t *testing.T) { - bkt := objstore.NewInMemBucket() - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) - - { - resp := httptest.NewRecorder() - api.DeleteTenant(resp, &http.Request{}) - require.Equal(t, http.StatusUnauthorized, resp.Code) - } - - { - ctx := context.Background() - ctx = user.InjectOrgID(ctx, userID) - - req := &http.Request{} - resp := httptest.NewRecorder() - api.DeleteTenant(resp, req.WithContext(ctx)) - - require.Equal(t, http.StatusOK, resp.Code) - objs := bkt.Objects() - require.NotNil(t, objs[path.Join(userID, tsdb.TenantDeletionMarkPath)]) - } -} - -func TestDeleteTenantStatus(t *testing.T) { - const username = "user" - - for name, tc := range map[string]struct { - objects map[string][]byte - expectedBlocksDeleted bool - }{ - "empty": { - objects: nil, - expectedBlocksDeleted: true, - }, - - "no user objects": { - objects: map[string][]byte{ - "different-user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), - }, - expectedBlocksDeleted: true, - }, - - "non-block files": { - objects: map[string][]byte{ - "user/deletion-mark.json": []byte("data"), - }, - expectedBlocksDeleted: true, - }, - - "block files": { - objects: map[string][]byte{ - "user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), - }, - expectedBlocksDeleted: false, - }, - } { - t.Run(name, func(t *testing.T) { - bkt := objstore.NewInMemBucket() - // "upload" objects - for objName, data := range tc.objects { - require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) - } - - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) - - res, err := api.isBlocksForUserDeleted(context.Background(), username) - require.NoError(t, err) - require.Equal(t, tc.expectedBlocksDeleted, res) - }) - } -} diff --git a/pkg/chunk/purger/tenant_deletion_api.go b/pkg/chunk/purger/tenant_deletion_api.go new file mode 100644 index 0000000000..2777e29966 --- /dev/null +++ b/pkg/chunk/purger/tenant_deletion_api.go @@ -0,0 +1,128 @@ +package purger + +import ( + "context" + "net/http" + "strings" + "time" + + "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" + "github.com/oklog/ulid" + "github.com/pkg/errors" + "github.com/prometheus/client_golang/prometheus" + "github.com/thanos-io/thanos/pkg/objstore" + + "github.com/cortexproject/cortex/pkg/storage/bucket" + cortex_tsdb "github.com/cortexproject/cortex/pkg/storage/tsdb" + "github.com/cortexproject/cortex/pkg/tenant" + "github.com/cortexproject/cortex/pkg/util" +) + +type TenantDeletionAPI struct { + bucketClient objstore.Bucket + logger log.Logger + cfgProvider bucket.TenantConfigProvider +} + +func NewTenantDeletionAPI(storageCfg cortex_tsdb.BlocksStorageConfig, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer) (*TenantDeletionAPI, error) { + bucketClient, err := createBucketClient(storageCfg, logger, "tenant-deletion-purger", reg) + if err != nil { + return nil, err + } + + return newTenantDeletionAPI(bucketClient, cfgProvider, logger), nil +} + +func newTenantDeletionAPI(bkt objstore.Bucket, cfgProvider bucket.TenantConfigProvider, logger log.Logger) *TenantDeletionAPI { + return &TenantDeletionAPI{ + bucketClient: bkt, + cfgProvider: cfgProvider, + logger: logger, + } +} + +func (api *TenantDeletionAPI) DeleteTenant(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + // When Cortex is running, it uses Auth Middleware for checking X-Scope-OrgID and injecting tenant into context. + // Auth Middleware sends http.StatusUnauthorized if X-Scope-OrgID is missing, so we do too here, for consistency. + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + + err = cortex_tsdb.WriteTenantDeletionMark(r.Context(), api.bucketClient, userID, api.cfgProvider, cortex_tsdb.NewTenantDeletionMark(time.Now())) + if err != nil { + level.Error(api.logger).Log("msg", "failed to write tenant deletion mark", "user", userID, "err", err) + + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + level.Info(api.logger).Log("msg", "tenant deletion mark in blocks storage created", "user", userID) + + w.WriteHeader(http.StatusOK) +} + +type DeleteTenantStatusResponse struct { + TenantID string `json:"tenant_id"` + BlocksDeleted bool `json:"blocks_deleted"` +} + +func (api *TenantDeletionAPI) DeleteTenantStatus(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + userID, err := tenant.TenantID(ctx) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + result := DeleteTenantStatusResponse{} + result.TenantID = userID + result.BlocksDeleted, err = api.isBlocksForUserDeleted(ctx, userID) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + util.WriteJSONResponse(w, result) +} + +func (api *TenantDeletionAPI) isBlocksForUserDeleted(ctx context.Context, userID string) (bool, error) { + var errBlockFound = errors.New("block found") + + userBucket := bucket.NewUserBucketClient(userID, api.bucketClient, api.cfgProvider) + err := userBucket.Iter(ctx, "", func(s string) error { + s = strings.TrimSuffix(s, "/") + + _, err := ulid.Parse(s) + if err != nil { + // not block, keep looking + return nil + } + + // Used as shortcut to stop iteration. + return errBlockFound + }) + + if errors.Is(err, errBlockFound) { + return false, nil + } + + if err != nil { + return false, err + } + + // No blocks found, all good. + return true, nil +} + +func createBucketClient(cfg cortex_tsdb.BlocksStorageConfig, logger log.Logger, name string, reg prometheus.Registerer) (objstore.Bucket, error) { + bucketClient, err := bucket.NewClient(context.Background(), cfg.Bucket, name, logger, reg) + if err != nil { + return nil, errors.Wrap(err, "create bucket client") + } + + return bucketClient, nil +} diff --git a/pkg/chunk/purger/tenant_deletion_api_test.go b/pkg/chunk/purger/tenant_deletion_api_test.go new file mode 100644 index 0000000000..ae8686c171 --- /dev/null +++ b/pkg/chunk/purger/tenant_deletion_api_test.go @@ -0,0 +1,90 @@ +package purger + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "path" + "testing" + + "github.com/go-kit/kit/log" + "github.com/stretchr/testify/require" + "github.com/thanos-io/thanos/pkg/objstore" + "github.com/weaveworks/common/user" + + "github.com/cortexproject/cortex/pkg/storage/tsdb" +) + +func TestDeleteTenant(t *testing.T) { + bkt := objstore.NewInMemBucket() + api := newTenantDeletionAPI(bkt, nil, log.NewNopLogger()) + + { + resp := httptest.NewRecorder() + api.DeleteTenant(resp, &http.Request{}) + require.Equal(t, http.StatusUnauthorized, resp.Code) + } + + { + ctx := context.Background() + ctx = user.InjectOrgID(ctx, "fake") + + req := &http.Request{} + resp := httptest.NewRecorder() + api.DeleteTenant(resp, req.WithContext(ctx)) + + require.Equal(t, http.StatusOK, resp.Code) + objs := bkt.Objects() + require.NotNil(t, objs[path.Join("fake", tsdb.TenantDeletionMarkPath)]) + } +} + +func TestDeleteTenantStatus(t *testing.T) { + const username = "user" + + for name, tc := range map[string]struct { + objects map[string][]byte + expectedBlocksDeleted bool + }{ + "empty": { + objects: nil, + expectedBlocksDeleted: true, + }, + + "no user objects": { + objects: map[string][]byte{ + "different-user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), + }, + expectedBlocksDeleted: true, + }, + + "non-block files": { + objects: map[string][]byte{ + "user/deletion-mark.json": []byte("data"), + }, + expectedBlocksDeleted: true, + }, + + "block files": { + objects: map[string][]byte{ + "user/01EQK4QKFHVSZYVJ908Y7HH9E0/meta.json": []byte("data"), + }, + expectedBlocksDeleted: false, + }, + } { + t.Run(name, func(t *testing.T) { + bkt := objstore.NewInMemBucket() + // "upload" objects + for objName, data := range tc.objects { + require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader(data))) + } + + api := newTenantDeletionAPI(bkt, nil, log.NewNopLogger()) + + res, err := api.isBlocksForUserDeleted(context.Background(), username) + require.NoError(t, err) + require.Equal(t, tc.expectedBlocksDeleted, res) + }) + } +} diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index 9cd1f222b3..02e63e5b88 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -802,18 +802,33 @@ func (t *Cortex) initChunksPurger() (services.Service, error) { return t.Purger, nil } -func (t *Cortex) initBlocksPurger() (services.Service, error) { +func (t *Cortex) initTenantDeletionAPI() (services.Service, error) { if t.Cfg.Storage.Engine != storage.StorageEngineBlocks { return nil, nil } + // t.RulerStorage can be nil when running in single-binary mode, and rule storage is not configured. + tenantDeletionAPI, err := purger.NewTenantDeletionAPI(t.Cfg.BlocksStorage, t.Overrides, util_log.Logger, prometheus.DefaultRegisterer) + if err != nil { + return nil, err + } + + t.API.RegisterTenantDeletion(tenantDeletionAPI) + return nil, nil +} + +func (t *Cortex) initBlocksPurger() (services.Service, error) { + if t.Cfg.Storage.Engine != storage.StorageEngineBlocks || !t.Cfg.PurgerConfig.EnableSeriesDeletion { + return nil, nil + } + // t.RulerStorage can be nil when running in single-binary mode, and rule storage is not configured. blockPurger, err := purger.NewBlocksPurgerAPI(t.Cfg.BlocksStorage, t.Overrides, util_log.Logger, prometheus.DefaultRegisterer, t.Cfg.PurgerConfig.DeleteRequestCancelPeriod) if err != nil { return nil, err } - t.API.RegisterBlocksPurger(blockPurger, t.Cfg.PurgerConfig.EnableSeriesDeletion) + t.API.RegisterBlocksPurger(blockPurger) return nil, nil } @@ -859,6 +874,7 @@ func (t *Cortex) setupModuleManager() error { mm.RegisterModule(Compactor, t.initCompactor) mm.RegisterModule(StoreGateway, t.initStoreGateway) mm.RegisterModule(ChunksPurger, t.initChunksPurger, modules.UserInvisibleModule) + mm.RegisterModule(TenantDeletion, t.initTenantDeletionAPI, modules.UserInvisibleModule) mm.RegisterModule(BlocksPurger, t.initBlocksPurger, modules.UserInvisibleModule) mm.RegisterModule(Purger, nil) mm.RegisterModule(QueryScheduler, t.initQueryScheduler) @@ -893,8 +909,9 @@ func (t *Cortex) setupModuleManager() error { Compactor: {API, MemberlistKV, Overrides}, StoreGateway: {API, Overrides, MemberlistKV}, ChunksPurger: {Store, DeleteRequestsStore, API}, + TenantDeletion: {Store, API, Overrides}, BlocksPurger: {Store, API, Overrides}, - Purger: {ChunksPurger, BlocksPurger}, + Purger: {ChunksPurger, TenantDeletion, BlocksPurger}, TenantFederation: {Queryable}, All: {QueryFrontend, Querier, Ingester, Distributor, TableManager, Purger, StoreGateway, Ruler}, } From 9263ca61353b690f8b5c8f3b922c74e36aa7deb7 Mon Sep 17 00:00:00 2001 From: Ilan Gofman Date: Wed, 13 Jul 2022 10:59:01 -0400 Subject: [PATCH 20/21] Make changes based on PR comments Signed-off-by: Ilan Gofman --- pkg/chunk/purger/blocks_purger.go | 41 ++++++--- pkg/chunk/purger/blocks_purger_test.go | 122 ++++++++++++++++++------- pkg/storage/tsdb/tombstones.go | 36 ++++---- 3 files changed, 131 insertions(+), 68 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index 2d4dea9993..fea9e5c2c2 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -14,7 +14,7 @@ import ( "github.com/go-kit/kit/log/level" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" - "github.com/prometheus/prometheus/promql/parser" + "github.com/prometheus/prometheus/model/labels" "github.com/thanos-io/thanos/pkg/objstore" "github.com/cortexproject/cortex/pkg/storage/bucket" @@ -64,12 +64,10 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht return } - for i := range match { - _, err := parser.ParseMetricSelector(match[i]) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } + matchers, err := cortex_tsdb.ParseMatchers(match) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return } startParam := params.Get("start") @@ -105,12 +103,12 @@ func (api *BlocksPurgerAPI) AddDeleteRequestHandler(w http.ResponseWriter, r *ht tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) - requestID := getTombstoneHash(startTime, endTime, match) + requestID := getTombstoneHash(startTime, endTime, matchers) // Since the request id is based on a hash of the parameters, there is a possibility that a tombstone could already exist for it // if the request was previously cancelled, we need to remove the cancelled tombstone before adding the pending one if err := tManager.RemoveCancelledStateIfExists(ctx, requestID); err != nil { level.Error(util_log.Logger).Log("msg", "removing cancelled tombstone state if it exists", "err", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "Error checking previous delete requests and removing the past cancelled version of this request if it exists ", http.StatusInternalServerError) return } @@ -144,6 +142,7 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r http.Error(w, err.Error(), http.StatusUnauthorized) return } + tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) deleteRequests, err := tManager.GetAllTombstonesForUser(ctx) if err != nil { @@ -155,8 +154,10 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r if err := json.NewEncoder(w).Encode(deleteRequests); err != nil { level.Error(util_log.Logger).Log("msg", "error marshalling response", "err", err) http.Error(w, fmt.Sprintf("Error marshalling response: %v", err), http.StatusInternalServerError) + return } + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) } @@ -171,6 +172,10 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r params := r.URL.Query() requestID := params.Get("request_id") + if len(requestID) == 0 { + http.Error(w, "request_id not set", http.StatusBadRequest) + return + } tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, api.logger) deleteRequest, err := tManager.GetTombstoneByIDForUser(ctx, requestID) @@ -186,7 +191,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r } if deleteRequest.State == cortex_tsdb.StateCancelled { - http.Error(w, "the request has already been previously deleted", http.StatusBadRequest) + http.Error(w, "the series deletion request was cancelled previously", http.StatusAccepted) return } @@ -211,7 +216,7 @@ func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r w.WriteHeader(http.StatusNoContent) } -func getTombstoneHash(startTime int64, endTime int64, selectors []string) string { +func getTombstoneHash(startTime int64, endTime int64, selectors []*labels.Matcher) string { // Any delete request with the same start, end time and same selectors should result in the same hash hash := md5.New() @@ -224,11 +229,19 @@ func getTombstoneHash(startTime int64, endTime int64, selectors []string) string hash.Write(bufStart) hash.Write(bufEnd) - sort.Strings(selectors) - for _, s := range selectors { + // First we get the strings of the parsed matchers which + // then are sorted and hashed after. This is done so that logically + // equivalent deletion requests result in the same hash + selectorStrings := make([]string, len(selectors)) + for i, s := range selectors { + selectorStrings[i] = s.String() + } + + sort.Strings(selectorStrings) + for _, s := range selectorStrings { hash.Write([]byte(s)) } - md5Bytes := md5.Sum(nil) + md5Bytes := hash.Sum(nil) return hex.EncodeToString(md5Bytes[:]) } diff --git a/pkg/chunk/purger/blocks_purger_test.go b/pkg/chunk/purger/blocks_purger_test.go index d4d79684c6..a612814b9e 100644 --- a/pkg/chunk/purger/blocks_purger_test.go +++ b/pkg/chunk/purger/blocks_purger_test.go @@ -66,7 +66,7 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { } req := &http.Request{ - Method: "GET", + Method: "POST", RequestURI: u.String(), URL: u, Body: http.NoBody, @@ -83,42 +83,82 @@ func TestBlocksDeleteSeries_AddingDeletionRequests(t *testing.T) { func TestBlocksDeleteSeries_AddingSameRequestTwiceShouldFail(t *testing.T) { - bkt := objstore.NewInMemBucket() - api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) + for name, tc := range map[string]struct { + selectorsFirst []string + selectorsSecond []string + }{ + "exact same request twice should fail": { + selectorsFirst: []string{"process_start_time_seconds{job=\"prometheus\"}"}, + selectorsSecond: []string{"process_start_time_seconds{job=\"prometheus\"}"}, + }, + "same request but with extra whitespace should fail": { + selectorsFirst: []string{"process_start_time_seconds{job=\"prometheus\"}"}, + selectorsSecond: []string{"process_start_time_seconds{job= \"prometheus\"}"}, + }, + // Since a deletion request is performed using the AND of all provided selectors + // it doesn't matter if the selectors are provided together as one string or + // in separate strings. + "same request but split in multiple matchers should fail": { + selectorsFirst: []string{"process_start_time_seconds{job=\"prometheus\"}"}, + selectorsSecond: []string{"process_start_time_seconds", "{job= \"prometheus\"}"}, + }, + } { + t.Run(name, func(t *testing.T) { + bkt := objstore.NewInMemBucket() + api := newBlocksPurgerAPI(bkt, nil, log.NewNopLogger(), 0) - ctx := context.Background() - ctx = user.InjectOrgID(ctx, userID) + ctx := context.Background() + ctx = user.InjectOrgID(ctx, userID) - params := url.Values{ - "start": []string{"1"}, - "end": []string{"2"}, - "match[]": []string{"node_exporter"}, - } + params := url.Values{ + "start": []string{"1"}, + "end": []string{"2"}, + "match[]": tc.selectorsFirst, + } - u := &url.URL{ - RawQuery: params.Encode(), - } + u := &url.URL{ + RawQuery: params.Encode(), + } - req := &http.Request{ - Method: "GET", - RequestURI: u.String(), - URL: u, - Body: http.NoBody, - Header: http.Header{}, - } + req := &http.Request{ + Method: "POST", + RequestURI: u.String(), + URL: u, + Body: http.NoBody, + Header: http.Header{}, + } - resp := httptest.NewRecorder() - api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) + resp := httptest.NewRecorder() + api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) - // First request made should be okay - require.Equal(t, http.StatusNoContent, resp.Code) + // First request made should be okay + require.Equal(t, http.StatusNoContent, resp.Code) - //second should not be accepted because the same exact request already exists - resp = httptest.NewRecorder() - api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) + params = url.Values{ + "start": []string{"1"}, + "end": []string{"2"}, + "match[]": tc.selectorsSecond, + } + + u = &url.URL{ + RawQuery: params.Encode(), + } - require.Equal(t, http.StatusBadRequest, resp.Code) + req = &http.Request{ + Method: "POST", + RequestURI: u.String(), + URL: u, + Body: http.NoBody, + Header: http.Header{}, + } + + //second should not be accepted because the same exact request already exists + resp = httptest.NewRecorder() + api.AddDeleteRequestHandler(resp, req.WithContext(ctx)) + require.Equal(t, http.StatusBadRequest, resp.Code) + }) + } } func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testing.T) { @@ -145,7 +185,7 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin } reqCreate := &http.Request{ - Method: "GET", + Method: "POST", RequestURI: uCreate.String(), URL: uCreate, Body: http.NoBody, @@ -157,7 +197,8 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin require.Equal(t, http.StatusNoContent, resp.Code) //cancel the previous request - requestID := getTombstoneHash(1000, 2000, []string{"node_exporter"}) + selector, _ := cortex_tsdb.ParseMatchers([]string{"node_exporter"}) + requestID := getTombstoneHash(1000, 2000, selector) paramsDelete := url.Values{ "request_id": []string{requestID}, } @@ -192,9 +233,10 @@ func TestBlocksDeleteSeries_AddingNewRequestShouldDeleteCancelledState(t *testin } -func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { +func TestBlocksDeleteSeries_CancellingRequest(t *testing.T) { for name, tc := range map[string]struct { + requestID string createdAt int64 requestState cortex_tsdb.BlockDeleteRequestState cancellationPeriod time.Duration @@ -202,6 +244,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { expectedHTTPStatus int }{ "not allowed, grace period has passed": { + requestID: "requestID", createdAt: 0, requestState: cortex_tsdb.StatePending, cancellationPeriod: time.Second, @@ -210,6 +253,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { }, "allowed, grace period not over yet": { + requestID: "requestID", createdAt: time.Now().Unix() * 1000, requestState: cortex_tsdb.StatePending, cancellationPeriod: time.Hour, @@ -217,6 +261,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { expectedHTTPStatus: http.StatusNoContent, }, "not allowed, deletion already occurred": { + requestID: "requestID", createdAt: 0, requestState: cortex_tsdb.StateProcessed, cancellationPeriod: time.Second, @@ -224,10 +269,19 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { expectedHTTPStatus: http.StatusBadRequest, }, "not allowed,request already cancelled": { + requestID: "requestID", createdAt: 0, requestState: cortex_tsdb.StateCancelled, cancellationPeriod: time.Second, cancelledFileExists: true, + expectedHTTPStatus: http.StatusAccepted, + }, + "not allowed, request id missing": { + requestID: "", + createdAt: 0, + requestState: cortex_tsdb.StatePending, + cancellationPeriod: time.Second, + cancelledFileExists: false, expectedHTTPStatus: http.StatusBadRequest, }, } { @@ -241,12 +295,12 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { tManager := cortex_tsdb.NewTombstoneManager(api.bucketClient, userID, api.cfgProvider, log.NewNopLogger()) //create the tombstone - tombstone := cortex_tsdb.NewTombstone(userID, tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, "request_id", tc.requestState) + tombstone := cortex_tsdb.NewTombstone(userID, tc.createdAt, tc.createdAt, 0, 1, []string{"match"}, tc.requestID, tc.requestState) err := tManager.WriteTombstone(ctx, tombstone) require.NoError(t, err) params := url.Values{ - "request_id": []string{"request_id"}, + "request_id": []string{tc.requestID}, } u := &url.URL{ @@ -266,7 +320,7 @@ func TestBlocksDeleteSeries_CancellingRequestl(t *testing.T) { require.Equal(t, tc.expectedHTTPStatus, resp.Code) // check if the cancelled tombstone file exists - exists, _ := tManager.TombstoneExists(ctx, "request_id", cortex_tsdb.StateCancelled) + exists, _ := tManager.TombstoneExists(ctx, tc.requestID, cortex_tsdb.StateCancelled) require.Equal(t, tc.cancelledFileExists, exists) }) diff --git a/pkg/storage/tsdb/tombstones.go b/pkg/storage/tsdb/tombstones.go index 18d3213826..a55bf07107 100644 --- a/pkg/storage/tsdb/tombstones.go +++ b/pkg/storage/tsdb/tombstones.go @@ -11,7 +11,7 @@ import ( "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/pkg/errors" - "github.com/prometheus/prometheus/pkg/labels" + "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/promql/parser" "github.com/thanos-io/thanos/pkg/objstore" @@ -119,18 +119,14 @@ func (m *TombstoneManager) GetTombstoneByIDForUser(ctx context.Context, requestI for _, state := range AllDeletionStates { filename := getTombstoneFileName(requestID, state) - exists, err := m.TombstoneExists(ctx, requestID, state) - if err != nil { - return nil, err - } - if exists { - t, err := m.ReadTombstone(ctx, path.Join(TombstonePath, filename)) - if err != nil { - return nil, err - } - found = append(found, t) + t, err := m.ReadTombstone(ctx, path.Join(TombstonePath, filename)) + if errors.Is(err, ErrTombstoneNotFound) { + continue + } else if err != nil { + return nil, err } + found = append(found, t) } if len(found) == 0 { @@ -147,28 +143,28 @@ func (m *TombstoneManager) GetAllTombstonesForUser(ctx context.Context) ([]*Tomb // add all the tombstones to a map and check for duplicates: if a key exists with the same request ID (but two different states). discovered := make(map[string]BlockDeleteRequestState) err := m.bkt.Iter(ctx, TombstonePath, func(s string) error { - requestID, state, err := GetStateAndRequestIDFromTombstonePath(filepath.Base(s)) + requestID, newerState, err := GetStateAndRequestIDFromTombstonePath(filepath.Base(s)) if err != nil { return err } if prevState, exists := discovered[requestID]; !exists { - discovered[requestID] = state + discovered[requestID] = newerState } else { // If there is more than one tombstone for a given request, we only want the latest state. // States can move only in a specific direction, so the later state will always win. - orderA, err := state.GetStateOrder() + newerStateOrder, err := newerState.getStateOrder() if err != nil { return err } - orderB, err := prevState.GetStateOrder() + prevStateOrder, err := prevState.getStateOrder() if err != nil { return err } // If the newer state found comes later in the order, then we replace the state in the map. - if orderA > orderB { - discovered[requestID] = state + if newerStateOrder > prevStateOrder { + discovered[requestID] = newerState } } return nil @@ -239,12 +235,12 @@ func (m *TombstoneManager) UpdateTombstoneState(ctx context.Context, t *Tombston err := m.WriteTombstone(ctx, newT) if err != nil { - level.Error(m.logger).Log("msg", "error creating file tombstone file with the updated state", "err", err) + level.Error(m.logger).Log("msg", "error creating file tombstone file with the updated state", "requestID", t.RequestID, "updated state", newState, "err", err) return nil, err } if err = m.DeleteTombstoneFile(ctx, t.RequestID, t.State); err != nil { - level.Error(m.logger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "err", err) + level.Error(m.logger).Log("msg", "Created file with updated state but unable to delete previous state. Will retry next time tombstones are loaded", "requestID", t.RequestID, "previous state", t.State, "updated state", newState, err) } return newT, nil @@ -346,7 +342,7 @@ func isValidDeleteRequestState(state BlockDeleteRequestState) bool { return false } -func (s BlockDeleteRequestState) GetStateOrder() (int, error) { +func (s BlockDeleteRequestState) getStateOrder() (int, error) { switch s { case StatePending: return 0, nil From 06cb9219fcd459afafb72f5ebe978713e133291c Mon Sep 17 00:00:00 2001 From: Ilan Gofman Date: Wed, 13 Jul 2022 11:21:06 -0400 Subject: [PATCH 21/21] fix return content type of get delete requests Signed-off-by: Ilan Gofman --- pkg/chunk/purger/blocks_purger.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/chunk/purger/blocks_purger.go b/pkg/chunk/purger/blocks_purger.go index fea9e5c2c2..802f5b6b1f 100644 --- a/pkg/chunk/purger/blocks_purger.go +++ b/pkg/chunk/purger/blocks_purger.go @@ -151,15 +151,12 @@ func (api *BlocksPurgerAPI) GetAllDeleteRequestsHandler(w http.ResponseWriter, r return } + w.Header().Set("Content-Type", "application/json") if err := json.NewEncoder(w).Encode(deleteRequests); err != nil { level.Error(util_log.Logger).Log("msg", "error marshalling response", "err", err) http.Error(w, fmt.Sprintf("Error marshalling response: %v", err), http.StatusInternalServerError) return } - - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - } func (api *BlocksPurgerAPI) CancelDeleteRequestHandler(w http.ResponseWriter, r *http.Request) {