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

downsample: fix deadlock if error occurs #4962

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

GiedriusS
Copy link
Member

Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least downsampleConcurrency + 2 blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Fixes #4960.

Signed-off-by: Giedrius Statkevičius giedrius.statkevicius@vinted.com

Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least `downsampleConcurrency + 2` blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Copy link
Member

@wiardvanrij wiardvanrij left a comment

Choose a reason for hiding this comment

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

I've done a review, though it's a bit passed my experience level; it looks really good and it's clear what it fixes. So, thats the scope of my review :)

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.

Thanks! Great resolution 👍🏽

I don't thing this happens to default setups though as we don't have concurrency by default, right?

@bwplotka bwplotka merged commit 58c4220 into thanos-io:main Dec 17, 2021
@GiedriusS
Copy link
Member Author

Thanks! Great resolution 👍🏽

I don't thing this happens to default setups though as we don't have concurrency by default, right?

It does, the test shows that. :D It's enough to have an error occur while downsampling and to have some blocks in the downsampling backlog for this to occur. Most of the time it is enough to have two extra in the backlog but if you are lucky (depending on the scheduler) the deadlock might not happen

GiedriusS added a commit to GiedriusS/thanos that referenced this pull request Dec 17, 2021
Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least `downsampleConcurrency + 2` blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@bwplotka
Copy link
Member

👍🏽

GiedriusS added a commit that referenced this pull request Dec 20, 2021
Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least `downsampleConcurrency + 2` blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
squat pushed a commit that referenced this pull request Dec 20, 2021
* downsample: fix deadlock if error occurs (#4962)

Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least `downsampleConcurrency + 2` blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* VERSION: bump

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* Makefile: update SHA256

Update from
https://github.com/thanos-io/thanos/blob/main/Makefile#L16-L17.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* *: 0.23.2 -> 0.23.2-rc.1

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS added a commit that referenced this pull request Dec 20, 2021
* Update version

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* downsample: fix deadlock if error occurs (#4962)

Fix deadlock that occurs if there is some backlog of blocks to be
deduplicated, and an error occurs. For this to trigger, there needs to
be at least `downsampleConcurrency + 2` blocks in the backlog.

Add test to cover this regression. Affected versions 0.22.0 - 0.23.1.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* Makefile: update SHA256

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* Makefile/CHANGELOG: fix format

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock if error while downsampling
3 participants