-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Apply upload criteria to memory buffering; Upload the memory buffer to S3 on exit. #17
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
Merged
Merged
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
fff3c95
latest
9abc3bf
latest
b7300e8
latest
960258d
latest
bf688b0
latest
22e1166
latest
2a1bea7
update linter
73e035a
latest
a7fa236
latest
446ffc8
latest
f0c6d4d
latest
2d8a893
latest
be48488
latest
3040e40
Merge branch 'main' of https://github.com/davemarco/fluent-bit-clp-fo…
3d2171d
merge commit
f51eeeb
fix reverted changes in merge
a53e99a
latest
8b1e4c0
latest
7ed89e8
latest
23b12be
david review
4d85d1c
fix lint
62c96d7
david review
93411c2
Update internal/irzstd/disk.go
davemarco fc1ec4c
david review
dbd9d64
david review
5c58aa5
Merge branch 'main' of https://github.com/davemarco/fluent-bit-clp-fo…
f818e28
latest
6ec3667
latest
a42dfca
latest
8eff7e4
latest
a3e31a5
latest
7d60165
lint
da2a955
latest
9499c9c
latest
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Package exit provides functions for gracefully shutting down the plugin. Exit functions are only | ||
| // called when Fluent Bit receives a kill signal, not during an abrupt crash. The plugin is given | ||
| // limited time to clean up resources before Fluent Bit terminates it. | ||
|
|
||
| package exit | ||
|
|
||
| import ( | ||
| "github.com/y-scope/fluent-bit-clp/internal/outctx" | ||
| ) | ||
|
|
||
| // Fs gracefully exits the plugin by closing files. | ||
| // | ||
| // Parameters: | ||
| // - ctx: Plugin context | ||
| // | ||
| // Returns: | ||
| // - err: Error closing file | ||
| func Fs(ctx *outctx.S3Context) error { | ||
| for _, eventManager := range ctx.EventManagers { | ||
| err := eventManager.Writer.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| eventManager.Writer = nil | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // S3 gracefully exits the plugin by flushing buffered data to S3. Makes a best-effort attempt, | ||
| // however Fluent Bit may kill the plugin before the upload completes. | ||
| // | ||
| // Parameters: | ||
| // - ctx: Plugin context | ||
| // | ||
| // Returns: | ||
| // - err: Error closing file | ||
| func S3(ctx *outctx.S3Context) error { | ||
| for _, eventManager := range ctx.EventManagers { | ||
| empty, err := eventManager.Writer.CheckEmpty() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if empty { | ||
| continue | ||
| } | ||
| err = eventManager.ToS3(ctx.Config, ctx.Uploader) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = eventManager.Writer.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| eventManager.Writer = nil | ||
| } | ||
|
|
||
| return nil | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,9 +111,7 @@ func decodeMsgpack(dec *codec.Decoder, config outctx.S3Config) ([]ffi.LogEvent, | |
| } | ||
| } | ||
|
|
||
| // Checks if criteria are met to upload to s3. If useDiskBuffer is false, then the chunk is always | ||
| // uploaded so always returns true. If useDiskBuffer is true, check if Zstd buffer size is greater | ||
| // than upload size. | ||
| // Checks whether Zstd buffer size is greater than or equal to upload size. | ||
| // | ||
| // Parameters: | ||
| // - eventManager: Manager for Fluent Bit events with the same tag | ||
|
|
@@ -123,10 +121,6 @@ func decodeMsgpack(dec *codec.Decoder, config outctx.S3Config) ([]ffi.LogEvent, | |
| // - readyToUpload: Boolean if upload criteria met or not | ||
| // - err: Error getting Zstd buffer size | ||
| func checkUploadCriteriaMet(eventManager *outctx.EventManager, uploadSizeMb int) (bool, error) { | ||
| if !eventManager.Writer.GetUseDiskBuffer() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one change changes buffer behaviour to not immediately upload |
||
| return true, nil | ||
| } | ||
|
|
||
| bufferSize, err := eventManager.Writer.GetZstdOutputSize() | ||
| if err != nil { | ||
| return false, fmt.Errorf("error could not get size of buffer: %w", err) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this method doesn't match what it does and to some degree doesn't really match its use case either.
I feel it is confusing that during a graceful exit we don't upload to S3 when using disk buffering. Instead we wait until restart for recovery to handle this. If I stop fluentbit, I'd expect all the logs available to be uploaded.
I feel adding a flush method would fit the use case more.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify — are you suggesting introducing a separate Flush() method on the writer, in addition to CheckEmpty()?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the expectation, but I was hesitant to upload on graceful shutdown in disk buffer mode because it can introduce duplication or corruption issues.
If we upload but the process is forcefully exited before the disk files are properly truncated, recovery could upload them again and create duplicates. Forcing additional flushes from the IR file to the Zstd file during shutdown could also be slow and, if interrupted mid-write, leave files in an inconsistent state.
One of the reasons to select disk buffering mode is fault tolerance; exiting quickly and relying on upload on restart is the safe path.
If we want “upload everything on stop” behavior for disk buffering, we could consider making it a configurable option if that’s something users would expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
For the behaviour, I think it is fair as long as we document everything.