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

Prevent read on TSDB once closeAllTSDB function has been called #4304

Merged
merged 15 commits into from
Jul 23, 2021
Merged

Prevent read on TSDB once closeAllTSDB function has been called #4304

merged 15 commits into from
Jul 23, 2021

Conversation

ilangofman
Copy link
Contributor

@ilangofman ilangofman commented Jun 22, 2021 •
edited
Loading

What this PR does:
Added another variable in the TSDBState that prevents any read occurring on the TSDB once the ingester has called the closeAllTSDB function.

Which issue(s) this PR fixes:
Fixes #3350

Checklist

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

pstibrany
pstibrany reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@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 for working on this. I think we should just use ingester's state to reject queries. In practice difference between entering Stopping and calling closeAllTSDB is tiny.

pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
bboreham
bboreham reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Would it be less repetitive to do the check in i.getTSDB() ?

pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
@pstibrany
Copy link
Contributor

Would it be less repetitive to do the check in i.getTSDB() ?

I advocated against that. I think having it on read path is more explicit than returning nil from getTSDB.

@bboreham
Copy link
Contributor

having it on read path is more explicit

Sorry, I missed that discussion. Did you talk about returning an error?

@ilangofman
Copy link
Contributor Author

Sorry, I missed that discussion. Did you talk about returning an error?

I can change it to return an error from getTSDB. Would that be more preferable?

@pstibrany
Copy link
Contributor

Sorry, I missed that discussion. Did you talk about returning an error?

I can change it to return an error from getTSDB. Would that be more preferable?

My opinion is that simple "getter" should simply return element from map, and not check for internal state of the ingester. This state only affects read-path, where it should be checked.

@bboreham
Copy link
Contributor

This state only affects read-path

Not sure I get this part. We wouldn't want to reopen a TSDB for write, would we?

@pstibrany
Copy link
Contributor

Not sure I get this part. We wouldn't want to reopen a TSDB for write, would we?

Right, we wouldn't. What I was trying to say is to add checks to read methods for this state – because that is where it will be more explicit, instead of getter.

On write path, we could either use getOrCreateTSDB or preferably add check for ingester state at the beginning of v2Push. Later in that method we also check for stopped flag, but we don't actually call i.stopIncomingRequests() in blocks code.

pracucci
pracucci reviewed Jun 29, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
@ilangofman
Prevent read on TSDB once closeAllTSDB function has been called
89151bd 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Fix formatting issues
7445aa5 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Address PR comments and remove unit test no longer required
efcdc9c 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Remove closed bool no longer used
3df93f0 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Remove error no longer used
224815b 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Remove comment and change return var
2ba3ba2 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Update log message to debug
0267a21 
Signed-off-by: ilangofman <igofman99@gmail.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 30, 2021
@ilangofman ilangofman force-pushed the fix-ingester-tsdb-close-bug branch from 27d9865 to 0267a21 Compare June 30, 2021 03:32
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jun 30, 2021
@ilangofman
Moved log message to separate function
d0a8ea5 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
change function name for checking if tsdb is closing
feb4f42 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
ingester should read/write only when state is running for block store
3c4798b 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman ilangofman force-pushed the fix-ingester-tsdb-close-bug branch from 18387bf to 3c4798b Compare July 2, 2021 20:01
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 2, 2021
ilangofman and others added 3 commits July 5, 2021 09:59
@ilangofman
Move running check to ingester v2 file
140d525 
Signed-off-by: ilangofman <igofman99@gmail.com>
@ilangofman
Merge branch 'master' into fix-ingester-tsdb-close-bug
4324b93
@ilangofman
Remove extra space
b7ac2c4 
Signed-off-by: ilangofman <igofman99@gmail.com>
bboreham
bboreham reviewed Jul 15, 2021
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Looking good now, but I think we can get away without the duplication in Push().

pkg/ingester/ingester.go Outdated Show resolved Hide resolved
@ilangofman
Remove duplication from push func
3427813 
Signed-off-by: ilangofman <igofman99@gmail.com>
bboreham
bboreham approved these changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Great! 🎉

treid314
treid314 approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@treid314 treid314 left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

pstibrany
pstibrany approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@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.

LGTM, thank you!

@pstibrany pstibrany merged commit dc43fee into cortexproject:master Jul 23, 2021
alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
@ilangofman @alvinlin123
Prevent read on TSDB once closeAllTSDB function has been called (cort…
3ab9e8c 
…exproject#4304)

* Prevent read on TSDB once closeAllTSDB function has been called

Signed-off-by: ilangofman <igofman99@gmail.com>

* Fix formatting issues

Signed-off-by: ilangofman <igofman99@gmail.com>

* Address PR comments and remove unit test no longer required

Signed-off-by: ilangofman <igofman99@gmail.com>

* Remove closed bool no longer used

Signed-off-by: ilangofman <igofman99@gmail.com>

* Remove error no longer used

Signed-off-by: ilangofman <igofman99@gmail.com>

* Remove comment and change return var

Signed-off-by: ilangofman <igofman99@gmail.com>

* Update log message to debug

Signed-off-by: ilangofman <igofman99@gmail.com>

* Moved log message to separate function

Signed-off-by: ilangofman <igofman99@gmail.com>

* change function name for checking if tsdb is closing

Signed-off-by: ilangofman <igofman99@gmail.com>

* ingester should read/write only when state is running for block store

Signed-off-by: ilangofman <igofman99@gmail.com>

* Move running check to ingester v2 file

Signed-off-by: ilangofman <igofman99@gmail.com>

* Remove extra space

Signed-off-by: ilangofman <igofman99@gmail.com>

* Remove duplication from push func

Signed-off-by: ilangofman <igofman99@gmail.com>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

@bboreham bboreham bboreham approved these changes

@pstibrany pstibrany pstibrany approved these changes

@treid314 treid314 treid314 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ingester trying to open a TSDB block index reader while shutting down
5 participants
@ilangofman @pstibrany @bboreham @treid314 @pracucci