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

[extension/storage/filestorage] Add CleanupOnStart flag for compaction temporary files #32863

Merged
merged 15 commits into from
May 16, 2024

Conversation

pandres-varian
Copy link
Contributor

@pandres-varian pandres-varian commented May 6, 2024

Description:
This PR includes a new flag cleanup_on_start for the compaction section.
During compaction a copy of the database is created, when the process is unexpectedly terminated that temporary file is not removed. That could lead to disk exhaustion given the following scenario:

This mitigates the potential risk of those temporary files left in a short period of time, by this scenario or similar ones.

Testing:
Included corner case where two instances of the extensions are spawned and one is compacting while the other would attempt to cleanup.

Documentation:
Included description in the README of the new configuration flag

@pandres-varian pandres-varian requested review from djaglowski and a team as code owners May 6, 2024 08:49
Copy link

linux-foundation-easycla bot commented May 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
extension/storage/filestorage/README.md Outdated Show resolved Hide resolved
extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Very minor nit, but use errors.Join once

extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
extension/storage/filestorage/client.go Outdated Show resolved Hide resolved
Each extension instance can create multiple clients.
Moving the logic to extension start is better aligned with the
extension lifecycle and prevent potential collisions, all clients will share the same compaction folder already cleaned up.
Copy link
Member

@djaglowski djaglowski 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, just some lint issues to address. You can run many of the checks locally with make -C extension/storage/filestorage

extension/storage/filestorage/extension.go Outdated Show resolved Hide resolved
extension/storage/filestorage/extension.go Outdated Show resolved Hide resolved
@djaglowski djaglowski merged commit 4964cd8 into open-telemetry:main May 16, 2024
160 of 162 checks passed
@github-actions github-actions bot added this to the next release milestone May 16, 2024
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jul 11, 2024
…n temporary files (open-telemetry#32863)

**Description:**
This PR includes a new flag **cleanup_on_start** for the compaction
section.
During compaction a copy of the database is created, when the process is
unexpectedly terminated that temporary file is not removed. That could
lead to disk exhaustion given the following scenario:
- Process is killed with a big database to be compacted
- Compaction is enabled on start
- Process will take longer to compact than the allotted time for the
collector to reply health checks (see: open-telemetry#32456)
- Process is killed while compacting
- Big temporary file left

This mitigates the potential risk of those temporary files left in a
short period of time, by this scenario or similar ones.

**Testing:**
Included corner case where two instances of the extensions are spawned
and one is compacting while the other would attempt to cleanup.

**Documentation:** 
Included description in the README of the new configuration flag

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants