Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

markblocks tool to mark blocks for deletion or as non-compactable #1551

Merged
merged 28 commits into from
Mar 30, 2022

Conversation

colega
Copy link
Contributor

@colega colega commented Mar 25, 2022

What this PR does

I think we should have this committed in the repo rather than having to look for it in issue comments.

Which issue(s) this PR fixes or relates to

Ref: #1537

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pstibrany
Copy link
Member

pstibrany commented Mar 25, 2022

If we make this more general and allow uploading "deletion marker" files as well, we can get rid of our internal "delete_blocks.sh" script. This script also checks if block's meta.json file actually exists, before uploading marker file, and has support for multiple blocks at once. What do you think about extending this PR to cover these use cases (deletion marker, check for blocks, multiple blocks)?

(Our script also supports --dry-run, to only show what it would do. I often use that option, and would maybe even make it a default.)

@colega colega requested review from dimitarvdimitrov and removed request for pracucci March 25, 2022 15:47
@colega colega marked this pull request as draft March 25, 2022 16:05
Ref: grafana/mimir-squad#453

I think we should have this commited in the repo rather than having to
look for it in issue comments.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Also allow multiple blocks, and check for block & mark existence.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega changed the title Commit the tool for no-compact marks creation markblocks tool to mark blocks for deletion or as non-compactable Mar 29, 2022
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega marked this pull request as ready for review March 29, 2022 10:24
@colega colega requested a review from a team March 29, 2022 10:26
@colega
Copy link
Contributor Author

colega commented Mar 29, 2022

This is ready for review, I'd like to hear opinions on the FlagSet change to an interface. If we're not comfortable with that, I can rollback it and use plain flags, with the usual text saying something like:

you don't need to provide --reason flag if --mark delete

@pstibrany
Copy link
Member

pstibrany commented Mar 29, 2022

This is ready for review, I'd like to hear opinions on the FlagSet change to an interface. If we're not comfortable with that, I can rollback it and use plain flags, with the usual text saying something like:

you don't need to provide --reason flag if --mark delete

I think we should do this (revert to flag). Reason is optional anyway, not need to do lot of changes because of one conditional flag in ops tool.

@colega colega marked this pull request as draft March 29, 2022 10:58
@pstibrany
Copy link
Member

We can also make this two separate cli tools, if you think that works better (mark-no-compact, mark-delete). For "mark-delete" I expect to add "--partial" flag soon, to support partial blocks (they don't have "meta.json" file but we can still check for index or chunks/000001 instead).

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega self-assigned this Mar 29, 2022
@pstibrany
Copy link
Member

@jdbaldry adding you as a reviewer from the docs squad. Does this need any docs? This is a tool, however it still could be useful for the customers.

I plan to start referencing this tool from our playbooks. Should we eventually move it under cmd/ and make it part of the release? (Not in this PR)

@pstibrany
Copy link
Member

Nice, although I was actually wondering if we should the out of order chunks reason for the issue that triggered this.

There are no out-of-order chunks in source blocks in our original issue. Chunks have invalid min/max time specified, which causes that resulting block from compaction has out-of-order chunks.

tools/markblocks/main.go Outdated Show resolved Hide resolved
tools/markblocks/main.go Outdated Show resolved Hide resolved
@jdbaldry
Copy link
Member

@jdbaldry adding you as a reviewer from the docs squad. Does this need any docs? This is a tool, however it still could be useful for the customers.

When in doubt, I'd always say "document it". If nothing else a brief README with a summary of the intended use case and a single example of usage would really nicely complement the help output from the CLI tool. The docs don't need to be perfect and aimed at new users yet, just enough that anyone that comes across this tool can understand it without having to read the code.

@pstibrany
Copy link
Member

Btw, we have our tools documented in docs/internal/tools.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Copy link
Contributor Author

colega commented Mar 30, 2022

Addressed all the feedback, and wrote some docs under docs/internal/tools.

Also split the help output into basic and full, since there's an overwhelming amount of bucket configuration flags.

$ go run ./tools/markblocks -help
This tool creates marks for TSDB blocks used by Mimir and uploads them to the specified backend.

Usage:
        markblocks -tenant <tenant id> -mark <deletion|no-compact> [-details <details message>] [-dry-run] blockID [blockID2 blockID3 ...]

  -backend string
    	Backend storage to use. Supported backends are: s3, gcs, azure, swift, filesystem. Use -full-help to see help on backends configuration. (default "filesystem")
  -details string
    	Details field of the uploaded mark. Recommended. (default empty).
  -dry-run
    	Don't upload the markers generated, just print the intentions on the screen.
  -full-help
    	Show help for all flags, including the bucket backend configuration.
  -mark string
    	Mark type to create, valid options: deletion, no-compact. Required.
  -tenant string
    	Tenant ID of the owner of the block. Required.

tools/markblocks/main.go Outdated Show resolved Hide resolved
@pstibrany
Copy link
Member

Addressed all the feedback

I still see some unaddressed comments. (Split of help is very nice, would be nice to have in all our tools.)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Copy link
Contributor Author

colega commented Mar 30, 2022

I still see some unaddressed comments.

Shame on me, I should have reviewed the conversation again before stating that. 🤦

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for committing this :) left a nit and a comment about full-help and help-all

tools/markblocks/main.go Outdated Show resolved Hide resolved
tools/markblocks/main.go Outdated Show resolved Hide resolved
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you, let's get this in! This is very useful tool to have in Mimir, instead of just our internal scripts.

tools/markblocks/main.go Outdated Show resolved Hide resolved
tools/markblocks/main.go Outdated Show resolved Hide resolved
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega enabled auto-merge (squash) March 30, 2022 11:42
@colega colega merged commit 12a75c3 into main Mar 30, 2022
@colega colega deleted the create-no-compact-mark branch March 30, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants