out_logrotate: add new out plugin #11110
Conversation
SagiROosto
requested review from
celalettin1286,
cosmo0920,
edsiper,
fujimotos,
niedbalski and
patrick-stephens
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new out_logrotate output plugin with sources, header, CMake entries, runtime tests, an example config, and build-option wiring (FLB_OUT_LOGROTATE enabled under FLB_ALL and Windows defaults). Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant In as Input Plugin
participant Out as out_logrotate
participant FS as File System
participant GZ as Gzip (optional)
end
In->>Out: flush event chunk
Out->>FS: ensure directory exists / open current file
Out->>Out: check/track current file size
alt size >= Max_Size
Out->>FS: rename current -> rotated (timestamped)
alt Gzip enabled
Out->>GZ: stream-compress rotated file
GZ-->>FS: write rotated .gz
FS->>Out: remove uncompressed rotated file
end
Out->>FS: cleanup old rotated files (Max_Files)
Out->>FS: create new current file
end
Out->>Out: format records (JSON/CSV/LTSV/PLAIN/TEMPLATE/MSGPACK)
Out->>FS: append formatted records to current file
Out-->>In: acknowledge flush
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txtcmake/windows-setup.cmakeplugins/out_logrotate/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (2)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (9)
flb_create(143-225)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
plugins/out_logrotate/logrotate.c (6)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (8)
conf/fluent-bit-logrotate.conf (1)
1-21: LGTM! Configuration demonstrates plugin features well.The example configuration effectively showcases the logrotate plugin's key features: size-based rotation (Max_Size 10M), file retention (Max_Files 5), gzip compression, and automatic directory creation.
Note: The path
/tmp/logsis Linux-specific. While this is acceptable for an example, consider documenting that users on Windows would need to adjust the path (e.g.,C:\logs).plugins/out_logrotate/CMakeLists.txt (1)
1-5: LGTM! Build configuration is correct.The CMake configuration properly declares the ZLIB dependency (required for gzip compression) and follows the standard plugin registration pattern used throughout Fluent Bit.
plugins/CMakeLists.txt (1)
366-367: LGTM! Plugin registration is correct.The
out_logrotateplugin is properly registered following the same pattern as other output plugins.cmake/plugins_options.cmake (1)
120-120: LGTM! Build option properly defined.The plugin option is correctly defined with an appropriate description and default value (ON), consistent with other output plugins in the codebase.
CMakeLists.txt (1)
301-301: LGTM! Plugin enabled in all-features build.The
FLB_OUT_LOGROTATEflag is correctly set within theFLB_ALLblock, ensuring the plugin is included when building with all features enabled.cmake/windows-setup.cmake (1)
103-103: LGTM! Windows support enabled.The plugin is enabled for Windows builds following the standard pattern used for other output plugins in this file.
Note: Runtime tests for this plugin are currently only registered for non-Windows systems (see
tests/runtime/CMakeLists.txtline 237). If Windows support is intended, consider adding Windows-specific tests or verifying the existing tests work on Windows.plugins/out_logrotate/logrotate.h (1)
1-32: LGTM! Header structure is clean and correct.The header properly defines output format constants for the logrotate plugin with appropriate include guards and licensing. The enum provides clear format options (JSON, CSV, LTSV, PLAIN, MSGPACK, TEMPLATE) that align with the PR objectives for multi-format support.
tests/runtime/CMakeLists.txt (1)
235-238: Discrepancy confirmed: out_logrotate is enabled on Windows in cmake/windows-setup.cmake but excluded from runtime tests.Verification shows the review comment's concern is valid. The out_logrotate plugin:
- Is set to enabled (
Yes) on Windows in cmake/windows-setup.cmake line 103- Contains extensive Windows-specific code: Windows headers (Shlobj.h, Shlwapi.h), Windows API calls (strncpy_s, localtime_s, PathRemoveFileSpecA), and platform-specific macros
- Has no Windows-specific build exclusions in its own CMakeLists.txt
Yet the runtime test is excluded on Windows (tests/runtime/CMakeLists.txt line 236-238) under a comment stating "These plugins work only on Linux"—contradicting the Windows code present in the source.
The same pattern applies to out_file, which also has Windows-specific code but excluded tests.
Action needed: Manually verify whether Windows support is production-ready and either extend tests to cover Windows or disable the plugin on Windows if support is incomplete.
|
Once the coderabbit comments are addressed we should trigger a full package build but @SagiROosto can you confirm it builds for all targets? |
|
@patrick-stephens UT failed for typo issue I Fixed it along with coderabbit suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/out_logrotate.c (1)
294-354: Address the past review concern and clarify expected file count.The addition of
TEST_ASSERT(file_count >= 0)at lines 340 and 349 correctly addresses the past review concern about directory listing failures being silently ignored. Good fix!However, there's an inconsistency in the comment at line 350: it states "Current file + 2 rotated files" but checks
file_count <= 4. Withmax_files=3configured at line 324, the expected count should be 1 current file + 3 rotated files = 4 total, not "2 rotated files" as the comment suggests.Consider updating the comment to match the actual logic:
- TEST_CHECK(file_count <= 4); /* Current file + 2 rotated files */ + TEST_CHECK(file_count <= 4); /* Current file + 3 rotated files (max_files=3) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🔇 Additional comments (11)
plugins/CMakeLists.txt (1)
366-366: LGTM!The plugin registration follows the established pattern and is correctly positioned alphabetically between
out_fileandout_forward.tests/runtime/out_logrotate.c (3)
36-154: LGTM!The helper functions are well-structured with appropriate error handling and resource cleanup. The recursive directory deletion, file counting, and pattern matching logic are all correct.
156-229: LGTM!The basic rotation test correctly configures the plugin, triggers rotation by writing sufficient data, and validates that both the original file and rotated files exist.
231-292: LGTM!The gzip compression test properly validates that rotated files are compressed by waiting for
.gzfiles to appear, accounting for potential async compression.plugins/out_logrotate/logrotate.c (7)
106-206: LGTM!The initialization function correctly sets up the plugin context, creates the hash table for per-file size tracking, parses configuration options, and includes proper error handling with resource cleanup.
531-576: LGTM!The file size tracking correctly uses a hash table to maintain per-file size state, addressing the previous concern about a shared global counter. The
should_rotate_filefunction retrieves size by filename, andupdate_file_size_counterusesfstatto read actual file size before updating the hash table.
625-745: LGTM!The streaming gzip compression implementation is memory-efficient and correct:
- Processes files in 64KB chunks to avoid loading entire files into memory
- Properly initializes deflate with raw deflate mode (negative window bits)
- Writes RFC 1952 compliant gzip header and footer with CRC32
- Verifies all input is consumed and compression completes with
Z_STREAM_END- Includes proper error handling and resource cleanup
860-944: LGTM!The cleanup logic correctly:
- Counts rotated files matching the timestamp pattern
- Returns early if within the
max_fileslimit- Sorts files by modification time (oldest first) using a simple bubble sort
- Removes excess files to maintain the
max_fileslimitThe approach ensures that rotated files are cleaned up while preserving the current active file.
1067-1081: LGTM!The metrics handling path now correctly calls
update_file_size_counter(ctx, out_file, fp)at line 1078 before closing the file, which addresses the previous review concern about stale file size counters preventing rotation for metrics-only streams.
946-1182: LGTM!The flush callback implements the complete workflow correctly:
- Checks rotation condition using per-file size from hash table
- Performs rotation and cleanup when needed
- Removes rotated file's entry from hash table to reset tracking
- Initializes file size from disk for new files using
fstat- Updates file size counter after all write operations (logs, metrics, msgpack)
The per-file size tracking ensures correct behavior when multiple files are managed by a single plugin instance.
1184-1274: LGTM!The exit function properly cleans up resources by destroying the hash table and freeing the context. The configuration map is comprehensive with sensible defaults: 100MB max size, 7 max files retained, and gzip compression enabled by default.
|
You'll have to fix the commits that don't match the guidelines btw, i.e. you need a plugin/module prefix like |
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
0dd42f9 to
99917b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
plugins/out_logrotate/logrotate.c (8)
129-134: Consider a larger hash table size for multi-file scenarios.The hash table is created with only 64 buckets. If the plugin writes to many different files (when
fileis not set and tags are used as filenames), this could lead to excessive hash collisions and degraded performance. Consider using a larger size (e.g., 256 or 512) or making it configurable for high-throughput, multi-file deployments.
605-623: Document the 4GB limitation for gzip compression.The gzip footer stores the original file size in 32 bits (4 bytes), which limits support to files under 4GB. While this is a standard gzip limitation, files larger than 4GB will have their size truncated modulo 2^32, which could cause decompression tools to report incorrect sizes or fail validation. Consider logging a warning when compressing files approaching or exceeding this limit, or documenting this limitation in the plugin configuration.
783-795: Check unlink() return values for error logging.Both
unlink()calls (lines 789 and 793) don't check return values. If deletion fails (e.g., permission denied), the files remain on disk but the function returns success or failure based solely on compression status. While the gzip failure path returns -1, the success path (line 790) doesn't verify that cleanup succeeded. Consider logging warnings ifunlink()fails.if (ret == 0) { /* Remove the uncompressed file */ - unlink(rotated_filename); + if (unlink(rotated_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove uncompressed file %s", rotated_filename); + } flb_plg_debug(ctx->ins, "rotated and compressed file: %s", gzip_filename); } else { /* Remove the failed gzip file */ - unlink(gzip_filename); + if (unlink(gzip_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove incomplete gzip file %s", gzip_filename); + } return -1; }
911-923: Bubble sort may be inefficient for many rotated files.The bubble sort has O(n²) complexity. If
max_filesis high and many files accumulate (e.g., hundreds of rotated logs), this could introduce noticeable latency during cleanup. For production environments with large retention counts, consider usingqsort()with a comparison function for O(n log n) performance.
216-242: No validation for non-map objects in CSV output.The function checks
obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0(line 216) but doesn't handle the case where the object is not a map or is empty. If a non-map object is passed, the function returns 0 (success) without writing anything. While this may be intentional, it could lead to silent data loss. Consider logging a warning when non-map objects are encountered.static int csv_output(FILE *fp, int column_names, struct flb_time *tm, msgpack_object *obj, struct flb_logrotate_conf *ctx) { int i; int map_size; msgpack_object_kv *kv = NULL; - if (obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0) { + if (obj->type != MSGPACK_OBJECT_MAP) { + flb_plg_warn(ctx->ins, "csv_output: expected map, got type %d", obj->type); + return -1; + } + + if (obj->via.map.size > 0) { kv = obj->via.map.ptr; map_size = obj->via.map.size; ... + } else { + flb_plg_debug(ctx->ins, "csv_output: empty map, skipping"); } return 0; }
252-272: No validation for non-map objects in LTSV output.Similar to
csv_output, this function silently returns success if the object is not a map or is empty (line 252). Consider adding validation and logging for consistency with error handling elsewhere in the plugin.
370-381: Silent failure in plain_output when JSON conversion fails.If
flb_msgpack_to_json_str()returns NULL, the function silently skips writing and returns 0 (success). This could lead to data loss without any indication to the caller. Consider returning -1 or logging a warning when conversion fails.static int plain_output(FILE *fp, msgpack_object *obj, size_t alloc_size, int escape_unicode) { char *buf; buf = flb_msgpack_to_json_str(alloc_size, obj, escape_unicode); if (buf) { fprintf(fp, "%s" NEWLINE, buf); flb_free(buf); + } else { + return -1; } return 0; }
383-407: print_metrics_text doesn't return error status.The function is
voidand doesn't return error status to the caller. Ifcmt_decode_msgpack_create()fails (line 394), an error is logged but the caller (line 1075) has no way to know the write failed. Consider changing the return type tointand propagating the error so the flush operation can retry or handle the failure appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runtime/out_logrotate.c
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_add(401-494)flb_hash_table_del(605-674)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (1)
plugins/out_logrotate/logrotate.c (1)
1073-1081: Metrics file size update now included - excellent!The past review noted that
update_file_size_counter()was missing after writing metrics, causing rotation to never trigger for metrics-only streams. This has been correctly addressed at line 1078.
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
2a77707 to
329cba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- conf/fluent-bit-logrotate.conf
- plugins/out_logrotate/CMakeLists.txt
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txttests/runtime/CMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
SagiROosto
force-pushed
the
out_logrotate
branch
from
329cba4 to
9d1e142
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/out_logrotate/logrotate.c (2)
578-582: Optional: Remove redundant hash table delete.Lines 579–581 delete the existing key before re-adding it, but
flb_hash_table_add()already handles replacement internally (seesrc/flb_hash_table.clines 430–442). The delete-then-add pattern adds unnecessary overhead—though it's safe, it doubles the hash lookups.Apply this diff to streamline the update:
- /* Delete existing entry if it exists to ensure we can update the value */ - flb_hash_table_del(ctx->file_sizes, filename); /* Store or update file size in hash table */ ret = flb_hash_table_add(ctx->file_sizes, filename, strlen(filename), &file_size, sizeof(size_t));The
flb_hash_table_addfunction already performs an in-place update for existing keys, so the explicit delete is not required.
783-795: Optional: Add path-length validation before constructing rotated filenames.Lines 784 and 795 use
snprintfwithPATH_MAX - 1, which will silently truncate iffilenameis too long. WhilePATH_MAXis typically large (4096 on Linux), a truncated filename would cause rotated files to be unreachable byis_valid_rotation_filename()andcleanup_old_files(), breaking rotation cleanup.Add a length check before constructing the rotated filename:
+ /* Validate that we have room for timestamp suffix */ + if (strlen(filename) + 16 >= PATH_MAX) { /* ".YYYYMMDD_HHMMSS" = 16 chars */ + flb_plg_error(ctx->ins, "filename too long for rotation: %s", filename); + return -1; + } + /* Create rotated filename with timestamp */ snprintf(rotated_filename, PATH_MAX - 1, "%s.%s", filename, timestamp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_logrotate/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_del(605-674)flb_hash_table_add(401-494)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
366-366: LGTM: Plugin registration is correct.The
out_logrotateplugin is registered following the same pattern as other output plugins, and the placement maintains alphabetical order.tests/runtime/out_logrotate.c (3)
157-235: LGTM: Basic rotation test is well-structured.The test properly:
- Sets up and tears down the test directory
- Configures rotation with
max_size=5Kandgzip=false- Writes enough data (~16KB) to trigger rotation
- Verifies both the current file and at least one rotated file exist
The sleep intervals (lines 210, 218) are necessary to allow async flush operations to complete.
237-300: LGTM: Gzip compression test validates the compression feature.The test correctly:
- Enables gzip compression with
gzip=true- Triggers rotation by writing sufficient data
- Uses
wait_for_file_pattern()to poll for rotated files with.gzsuffix- Verifies that gzip compression was applied to rotated files
302-366: LGTM: Max files cleanup test validates the retention policy.The test properly:
- Configures
max_files=3to limit retained rotated files- Triggers multiple rotations by writing data in 5 iterations
- Validates that file count never exceeds 4 (current file + 3 rotated files)
- Uses
TEST_ASSERT(file_count >= 0)to catch directory listing failures (addressing the past review comment)- Verifies the final count after all rotations complete
The iterative checking (lines 349–351) ensures cleanup happens progressively during rotation, not just at the end.
* add logrotate out plugin * fix gzip * fix test * update max_size * update gzip * add logs Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
…d fix UT Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
…ad of hash table This change replaces the hash table with a linked list for storing file sizes, improving memory management and simplifying the code. Additionally, helper functions for managing file size entries have been added, enhancing the overall structure and readability of the logrotate plugin. Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
SagiROosto
force-pushed
the
out_logrotate
branch
from
9d1e142 to
277aea3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
751-796: Critical: Z_FINISH still not sent for certain file sizes, causing corrupt gzip output.The compression loop still has the logic error flagged in previous reviews. When the input file size is an exact multiple of
GZIP_CHUNK_SIZE,feof(src_fp)returns false immediately after the last successful read (line 764), soflush = Z_NO_FLUSH. On the next iteration,freadreturns 0, but theif (bytes_read > 0)guard on line 754 prevents the compression block from executing, soZ_FINISHis never sent. The loop exits withstatus != Z_STREAM_END, and rotated gzip files are corrupted.Apply this fix to ensure the final chunk always sends
Z_FINISH:/* Process file in chunks (ensure Z_FINISH is always issued) */ do { bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); + /* Check EOF status after read to determine if this is the last chunk */ + int is_eof = feof(src_fp) || bytes_read == 0; + if (bytes_read > 0) { /* Update CRC and total size */ crc = mz_crc32(crc, (const unsigned char *)input_buffer, bytes_read); total_input_size += bytes_read; /* Set up deflate input */ strm.next_in = (Bytef *)input_buffer; strm.avail_in = bytes_read; - /* Determine flush mode based on EOF after this read */ - flush = feof(src_fp) ? Z_FINISH : Z_NO_FLUSH; + /* Use the EOF status captured after the read */ + flush = is_eof ? Z_FINISH : Z_NO_FLUSH; /* Compress chunk */ do {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/out_logrotate/CMakeLists.txt
- plugins/out_logrotate/logrotate.h
- CMakeLists.txt
- plugins/CMakeLists.txt
- conf/fluent-bit-logrotate.conf
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- cmake/windows-setup.cmake
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_output.c (2)
flb_output_get_property(1108-1111)flb_output_set_context(1546-1549)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (7)
tests/runtime/CMakeLists.txt (1)
237-237: LGTM!The runtime test registration for
out_logrotateis correctly placed within the Linux-only block, consistent without_fileon the previous line.plugins/out_logrotate/logrotate.c (6)
527-648: Per-file size tracking correctly implemented.The file size tracking functions properly maintain a separate size counter for each output file using a linked list, resolving the previous issue where a single global counter caused incorrect rotation decisions. The linear search through the list is acceptable for typical workloads with a small number of output files.
1141-1148: LGTM! File size counter now updated for metrics.The metrics handling path now correctly updates the file size counter (line 1145) before closing the file, ensuring rotation works properly for metrics-only streams. This addresses the previous critical issue.
1179-1188: LGTM! Error path now updates file size counter.The decoder initialization error path now correctly updates the file size counter (line 1185) before closing the file, keeping internal state consistent even when errors occur. This addresses the previous issue.
1280-1365: Configuration defaults are sensible.The default values (100M max_size, 7 max_files, gzip enabled) provide reasonable defaults for typical log rotation scenarios. The
workers = 1setting correctly prevents concurrent flushes within a single instance.
873-927: LGTM! Rotation filename validation is thorough.The validation function correctly identifies rotated files by checking the timestamp pattern (YYYYMMDD_HHMMSS) and optional
.gzsuffix, preventing false positives during cleanup.
929-1014: Cleanup logic correctly retains newest files.The cleanup function properly sorts rotated files by modification time and removes the oldest entries beyond
max_files. The bubble sort (lines 982-993) is O(n²) but acceptable for the typical small number of rotated files.
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
…ng support This commit introduces several new test functions for various log formats including CSV, LTSV, plain text, MsgPack, and template formats. Additionally, it enhances the multithreading capabilities of the logrotate tests, ensuring that data is correctly handled and written in a concurrent environment. The changes also include helper functions for reading file content and verifying the output format. Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
817-872: Verify Z_FINISH is always sent at end of compression.The compression loop has a potential issue where Z_FINISH may not be sent for files whose size is an exact multiple of
GZIP_CHUNK_SIZE(64KB). When the lastfreadconsumes exactlyGZIP_CHUNK_SIZEbytes,feof()returns false (line 830). On the next iteration,freadreturns 0,feof()is now true, but theif (bytes_read > 0)guard (line 820) prevents the compression block from executing, soZ_FINISHis never sent.This was flagged in a past review comment and marked as addressed. However, examining the current code, the issue still appears to be present. The fix would require either:
- Moving the flush mode determination outside the
bytes_read > 0check, or- Always performing a final deflate call with
Z_FINISHafter the loop whenfeof()is true.Please verify this issue was actually fixed or if it needs to be addressed.
#!/bin/bash # Verify gzip compression correctness by checking if Z_FINISH logic handles exact multiples rg -n -A5 -B5 'Z_FINISH.*feof|feof.*Z_FINISH' plugins/out_logrotate/logrotate.c
🧹 Nitpick comments (3)
plugins/out_logrotate/logrotate.c (3)
167-179: Consider validating the template parameter when format is template.When
formatis set to"template", the code doesn't verify that thetemplateparameter is configured. If a user sets format to template but doesn't provide a template string,ctx->templatewill be NULL (line 134) or the default value from config_map (line 1539:"{time} {message}"), which may not be the intended behavior. Consider adding validation to ensure the template parameter is set when using template format, or document the default template behavior.
1148-1474: Verify all error paths properly release resources.The flush callback has complex lock handling with multiple acquire/release points. While tracing through the code, all error paths appear to properly:
- Release locks if acquired
- Update file size counters before closing
- Close file pointers before returning
However, the complexity makes this error-prone for future maintenance. Consider extracting helper functions or using RAII-style patterns (if possible in C with cleanup attributes) to reduce the chance of resource leaks during future modifications.
1557-1572: Consider documenting size format for max_size parameter.The
max_sizeparameter usesFLB_CONFIG_MAP_SIZEwhich likely supports human-readable formats like "100M", "1G", etc. The default value "100000000" (line 1557) is 100MB but would be more readable as "100M" or "100MB". Consider updating the default to use the human-readable format for consistency with the description.Apply this diff to improve readability:
{ FLB_CONFIG_MAP_SIZE, "max_size", "100000000", + FLB_CONFIG_MAP_SIZE, "max_size", "100M", 0, FLB_TRUE, offsetof(struct flb_logrotate_conf, max_size), "Maximum size of file before rotation (default: 100M)" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (8)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_output.c (2)
flb_output_get_property(1108-1111)flb_output_set_context(1546-1549)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_destroy(19-30)flb_lock_acquire(32-65)flb_lock_release(67-100)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 ast-grep (0.40.0)
tests/runtime/out_logrotate.c
[warning] 462-462: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (6)
tests/runtime/out_logrotate.c (2)
182-215: TOCTOU in test helper is acceptable for test code.The static analysis tool flags a TOCTOU race between
statandfopenat line 194. In test code running in isolation, this is not a security concern and the risk window is negligible. The pattern is idiomatic for checking file size before allocation.
1035-1244: Rotation tests comprehensively validate core plugin functionality.The three rotation tests verify size-based rotation, gzip compression, and max_files cleanup. The past review issue regarding directory listing failures has been properly addressed with
TEST_ASSERT(file_count >= 0)checks before the upper-bound validations (lines 1228, 1239).plugins/out_logrotate/logrotate.c (4)
529-669: File size tracking with per-file locking is well-designed.The use of a linked list with per-file locks allows concurrent operations on different files while protecting per-file state. Lock acquisition uses retry logic with appropriate defaults, and all lock acquisitions are paired with releases.
1065-1076: Bubble sort acceptable for small file counts.The bubble sort used here is O(n²) but acceptable since the number of rotated files is typically small (default max_files is 7). For much larger max_files values, a more efficient sort could be considered, but this is unlikely to be a bottleneck.
1207-1243: Rotation logic temporarily releases lock to avoid holding during slow operations.The code releases the lock before calling
rotate_file(lines 1210-1214), then re-acquires it afterward (lines 1228-1242). This is intentional to avoid holding the lock during the potentially slow rotation and compression operations. The plugin setsworkers = 1(line 1585) which prevents concurrent flushes within a single instance, reducing the risk of races during this window. However, if multiple plugin instances target the same file (which is documented as unsupported), races could still occur.The past review comments note this limitation is documented, so this is acceptable.
1476-1501: Exit callback properly cleans up all resources.The cleanup uses
mk_list_foreach_safefor safe deletion during iteration and properly destroys mutexes before freeing memory. Resource cleanup order is correct.
|
@patrick-stephens I made a couple of changes hopefully it will pass CI this time. can you run it? |
2 participants
This PR present a new out plugin
out_logrotate. This Will answer #7572 question and solve #7541 suggestion.This feature is highly required in bare-metal or physical machines where you have finite and limited storage.
The plugin is basically a copy of
out_filewith a rotation feature. The reason to do that in additional plugin and not on theout_fileitself is to avoid accidentally turning this feature on and having redundant info/allocs/processing where it is not needed (so normal users can still use the regular file plugin without issue).(this replaces closed #10824 PR)
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests