Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Ingesters: Delete blocks relative to when they were uploaded to storage #3816

Merged
merged 3 commits into from
Jan 13, 2023
Merged

Ingesters: Delete blocks relative to when they were uploaded to storage #3816

merged 3 commits into from
Jan 13, 2023

Conversation

jesusvazquez
Copy link
Member

@jesusvazquez jesusvazquez commented Dec 23, 2022 •
edited
Loading

This PR changes the logic of when blocks will be deleted on
ingesters. Before it was relative to the newest block for the tenant
minus a certain retention. Now it depends on whether or not shipping is
enabled.

  • If it is enabled, deletion will be relative to when the block was
    uploaded minus a certain retention.
  • If it is disabled, deletion will be relative to when the block was
    created minos a certain retention.

We do this for two reasons:

  • It will allow us to lower the retention on ingesters making them
    ligther.
  • If the out-of-order window is bigger than the retention time, with the
    previous logic blocks would be deleted right after upload, leaving a
    time where data would not be queryable.

@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch from 0b10001 to 931fa26 Compare December 23, 2022 13:10
@pracucci
Copy link
Collaborator

Have you considered making it based on when the block is created? You can read the creation timestamp from the block ULID.

@jesusvazquez
Copy link
Member Author

jesusvazquez commented Dec 23, 2022 •
edited
Loading

@pracucci ULID can give me that information but I actually need to know it was shipped because I have to wait until its shipped and re-scanned. I can now know the first part with this implementation and I can have an estimation as a constant for the second part.

If I did it relative to block creation time maybe its shipped late for whatever reason i.e errors uploading to gcs, and whenever it got uploaded it could also get deleted. If this happened there could be a time there were data disappears for queries until blocks are re-scanned by the store gateways.

@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch 3 times, most recently from 1c16137 to 650bfb6 Compare December 23, 2022 14:41
@colega
Copy link
Contributor

colega commented Dec 23, 2022

I think that doing it based on shipping time is safer: in most case it will be the same, but as @jesusvazquez mentions, in case of a block storage outage for whatever reason, we'll keep the block for enough time.

colega
colega reviewed Dec 23, 2022
View reviewed changes
pkg/ingester/user_tsdb.go Outdated Show resolved Hide resolved
@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch 2 times, most recently from 122d5df to df06de9 Compare January 2, 2023 15:55
@56quarters
Copy link
Contributor

Since this changes the mental model of how blocks are managed on ingesters, please make sure you update any relevant docs and runbooks.

@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch 2 times, most recently from 76311a3 to 31bf115 Compare January 10, 2023 15:19
@jesusvazquez
Copy link
Member Author

Hey @56quarters, I updated the docs and the reference help as well as added an entry in the CHANGELOG and updated the description of the blocks-storage.tsdb.retention-period flag.

Do you know of any other doc pointers I should update?

In the meantime I'll set this PR as ready to review.

@jesusvazquez jesusvazquez marked this pull request as ready for review January 10, 2023 15:21
@jesusvazquez jesusvazquez requested review from a team as code owners January 10, 2023 15:21
@jesusvazquez jesusvazquez self-assigned this Jan 10, 2023
@colega
Copy link
Contributor

colega commented Jan 11, 2023

This looks good to me, also the change makes a lot of sense. I did some pair programming on this though, so we'd need an approval from someone who's not me.

@pstibrany
Copy link
Member

Shipping is an optional feature, and there are users who run Mimir with shipping disabled. Is that case properly handled by the PR? (Just started the review)

pstibrany
pstibrany reviewed Jan 11, 2023
View reviewed changes
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.

Looks good, but I don't see above-mentioned case when shipping is disabled handled in the PR. With shipping disabled, perhaps we can check block creation time against the "deadline".

I'd also suggest adding test for "shipping disabled" case.

pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Show resolved Hide resolved
pkg/ingester/shipper.go Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch 5 times, most recently from bd8958f to 397640f Compare January 13, 2023 12:25
pstibrany
pstibrany approved these changes Jan 13, 2023
View reviewed changes
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, PR looks good to me. I've left some non-blocking comments.

pkg/ingester/shipper.go
@@ -233,41 +230,63 @@ func (s *Shipper) blockMetasFromOldest() (metas []*metadata.Meta, _ error) {
return metas, nil
}

func readShippedBlocks(dir string) (map[ulid.ULID]struct{}, error) {
func readShippedBlocks(dir string) (map[ulid.ULID]time.Time, error) {
Copy link
Member

Choose a reason for hiding this comment

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

[nit] We can simplify shipper code a bit if we return map[ulid.ULID]model.Time here.

pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pkg/ingester/user_tsdb.go Show resolved Hide resolved
pkg/ingester/user_tsdb.go Outdated Show resolved Hide resolved
pkg/ingester/user_tsdb.go Outdated Show resolved Hide resolved
pkg/ingester/user_tsdb.go Outdated Show resolved Hide resolved
@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch 4 times, most recently from 5124791 to 40ff081 Compare January 13, 2023 13:48
pstibrany
pstibrany approved these changes Jan 13, 2023
View reviewed changes
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 very much!

@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch from 40ff081 to ed76885 Compare January 13, 2023 13:53
@jesusvazquez
Copy link
Member Author

Thank you for the reviews @pstibrany !! 🎉

@pstibrany pstibrany enabled auto-merge (squash) January 13, 2023 14:00
pracucci
pracucci reviewed Jan 13, 2023
View reviewed changes
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
pracucci
pracucci reviewed Jan 13, 2023
View reviewed changes
pkg/ingester/shipper.go Outdated
}

// Keep backwards compatibility with the previous Mimir version.
// TODO Remove in Mimir 2.7.0
Copy link
Collaborator

@pracucci pracucci Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please replace 2.7 with 2.8 across the file. We'll announce in 2.6, so we can remove starting 2.8.

@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch from ed76885 to ce92779 Compare January 13, 2023 14:04
@jesusvazquez
Update block retention based on shipping
00a3516 
This commit changes the logic of when blocks will be deleted on
ingesters. Before it was relative to the newest block for the tenant
minus a certain retention. Now it depends on whether or not shipping is
enabled.
- If it is enabled, deletion will be relative to when the block was
  uploaded minus a certain retention.
- If it is disabled, deletion will be relative to when the block was
  created minos a certain retention.

We do this for two reasons:
- It will allow us to lower the retention on ingesters making them
  ligther.
- If the out-of-order window is bigger than the retention time, with the
  previous logic blocks would be deleted right after upload, leaving a
time where data would not be queryable.

Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>
@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch from ce92779 to 00a3516 Compare January 13, 2023 14:08
pracucci
pracucci approved these changes Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Nice job! LGTM 👏

pracucci
pracucci reviewed Jan 13, 2023
View reviewed changes
pkg/ingester/shipper.go Outdated Show resolved Hide resolved
@jesusvazquez
Update TODO comment to be removed after Mimir 2.8.0
03a700f 
Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>
@pracucci pracucci enabled auto-merge (squash) January 13, 2023 14:14
@jesusvazquez
overriden is a misspelling of overridden
26be232 
Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>
@jesusvazquez jesusvazquez force-pushed the jvp/add-uploaded-time-to-shipped-blocks branch from c2360b6 to 26be232 Compare January 13, 2023 14:21
@pracucci pracucci merged commit 572bd1d into main Jan 13, 2023
@pracucci pracucci deleted the jvp/add-uploaded-time-to-shipped-blocks branch January 13, 2023 14:38
@pstibrany pstibrany mentioned this pull request Jan 16, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colega colega colega left review comments

@pracucci pracucci pracucci approved these changes

@pstibrany pstibrany pstibrany approved these changes

Assignees

@jesusvazquez jesusvazquez

Labels
component/ingester
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jesusvazquez @pracucci @colega @56quarters @pstibrany