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

compact: do not cleanup blocks on boot #3532

Merged
merged 1 commit into from
Dec 5, 2020
Merged

compact: do not cleanup blocks on boot #3532

merged 1 commit into from
Dec 5, 2020

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Dec 2, 2020 •
edited
Loading

Do not clean up blocks on boot because in some very bad cases there could
be thousands of blocks ready-to-be deleted and doing that makes Thanos
Compact exceed initialDelaySeconds on k8s.

Signed-off-by: Giedrius Statkevičius [email protected]

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

Changes

  • Removed cleanup on boot; added another metric which shows how many loops were performed. Used that metric in tests.

Verification

Updated e2e tests that pass.

Fixes #3395.

@GiedriusS GiedriusS force-pushed the remove_initiaL_cleanup branch 2 times, most recently from eb3ed62 to cf9ef37 Compare December 2, 2020 21:58
@GiedriusS GiedriusS marked this pull request as ready for review December 2, 2020 21:59
@kesor
Copy link

kesor commented Dec 3, 2020

This doesn't look like it addresses #3395 correctly. What should actually happen, is the HTTP socket should become open at the start of the process. And when either the cleanup, or compaction, or whatever starts failing the health check needs to start failing - or maybe the process might die because of errors. If there was a good reason to add cleanup at the start of the process, why remove it just to make the HTTP socket available?

@GiedriusS
Copy link
Member Author

This doesn't look like it addresses #3395 correctly. What should actually happen, is the HTTP socket should become open at the start of the process. And when either the cleanup, or compaction, or whatever starts failing the health check needs to start failing - or maybe the process might die because of errors. If there was a good reason to add cleanup at the start of the process, why remove it just to make the HTTP socket available?

That's true on a very pedantic level but syncing the metas + doing an initial cleanup potentially can take a lot of time because some people had many left-over blocks. That's where the complaints were coming from. With these changes now we barely do anything on boot before enabling the HTTP socket so it should be seamless just like before.

@metalmatze
Copy link
Contributor

It'd be great if we can merge this into release-0.17 first and then add it to v0.17.2. I'll merge it back to master afterwards.

bwplotka
bwplotka approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

This simplifies a lot things, thanks 💪

@bwplotka bwplotka changed the base branch from master to release-0.17 December 3, 2020 16:53
@bwplotka
Copy link
Member

bwplotka commented Dec 3, 2020

I based this branch to release-0.17 but looks like we need to rebase git commit as well 🤗 can you help @GiedriusS

Once this is merged we can do release 0.17.2

@GiedriusS
compact: do not cleanup blocks on boot
c9178db 
Do not cleanup blocks on boot because in some very bad cases there could
be thousands of blocks ready-to-be deleted and doing that makes Thanos
Compact exceed `initialDelaySeconds` on k8s.

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS
Copy link
Member Author

@metalmatze @bwplotka I have rebased this PR, PTAL.

@bwplotka bwplotka merged commit d57813b into thanos-io:release-0.17 Dec 5, 2020
@GiedriusS GiedriusS mentioned this pull request Dec 7, 2020
Closed
metalmatze pushed a commit to metalmatze/thanos that referenced this pull request Dec 9, 2020
@GiedriusS @metalmatze
compact: do not cleanup blocks on boot (thanos-io#3532)
785de24 
Do not cleanup blocks on boot because in some very bad cases there could
be thousands of blocks ready-to-be deleted and doing that makes Thanos
Compact exceed `initialDelaySeconds` on k8s.

Signed-off-by: Giedrius Statkevičius <[email protected]>
bwplotka added a commit that referenced this pull request Dec 9, 2020
@metalmatze @yeya24
@2nick @bwplotka @GiedriusS
Merge release-0.17 branch back (#3553)
076985c 
* Fix query frontend regression on release v0.17.0 (#3480)

* query-frontend: make POST-request to downstream url for labels and series api endpoints (#3444)

Signed-off-by: Alexander Tunik <[email protected]>

* remove default response cache config

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

* ensure order when merging multiple responses

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

Co-authored-by: Alexander Tunik <[email protected]>

* *: Set debug.SetPanicOnFault(true) so we can recover seg faults. (#3498)

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Prepare v0.17.1 release (#3505)

Signed-off-by: Matthias Loibl <[email protected]>

* fix index out of bound bug when comparing ZLabelSets (#3520)

* fix index out of bound bug when comparing ZLabelSets

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

* fix param parsing error message

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

* address comment feedbacks

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

* compact: do not cleanup blocks on boot (#3532)

Do not cleanup blocks on boot because in some very bad cases there could
be thousands of blocks ready-to-be deleted and doing that makes Thanos
Compact exceed `initialDelaySeconds` on k8s.

Signed-off-by: Giedrius Statkevičius <[email protected]>

* Prepare v0.17.2 (#3543)

Signed-off-by: Matthias Loibl <[email protected]>

* Properly rebase CHANGELOG.md after merging release-0.17

Signed-off-by: Matthias Loibl <[email protected]>

Co-authored-by: Ben Ye <[email protected]>
Co-authored-by: Alexander Tunik <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]>
Co-authored-by: Giedrius Statkevičius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
component: compact
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compactor: HTTP socket is only opened after initial cleanup of compacted blocks is complete
4 participants
@GiedriusS @kesor @metalmatze @bwplotka