in_calyptia_fleet: create fleet file if not using fleet. #10890
Merged
Conversation
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
ThomasBrady
requested review from
edsiper,
fujimotos and
koleini
as code owners
WalkthroughUpdates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Plugin as CalyptiaFleet Plugin
participant getFile as get_calyptia_file()
participant Check as check_timestamp_is_newer()
participant Config as Fluent Bit Context
participant FS as Filesystem
Plugin->>getFile: request config (dst == NULL)
getFile->>Check: compare last_modified vs existing
Check-->>getFile: is_newer (TRUE/FALSE)
getFile->>Config: inspect current config usage
Config-->>getFile: not_using_fleet (TRUE/FALSE)
alt is_newer == FLB_FALSE and not_using_fleet == FLB_FALSE
getFile-->>Plugin: abort creation (existing newer file in use)
else
getFile->>FS: create/write timestamped config
FS-->>getFile: write success
getFile-->>Plugin: return success (log notes if not_using_fleet)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
edsiper
requested changes
Sep 17, 2025
Member
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)
1201-1203: Default to “not using fleet” when context/config is unavailable (safer bootstrap).If flb_context_get() or flb_ctx->config is NULL, we currently treat this as “using fleet,” which can block first‑run in edge cases. Compute not_using_fleet via is_fleet_config with a NULL‑tolerant cfg pointer.
Apply this diff:
- flb_ctx_t *flb_ctx = flb_context_get(); - int is_newer = check_timestamp_is_newer(ctx, last_modified); - int not_using_fleet = (flb_ctx && flb_ctx->config && is_fleet_config(ctx, flb_ctx->config) == FLB_FALSE); + flb_ctx_t *flb_ctx = flb_context_get(); + struct flb_config *cur_cfg = flb_ctx ? flb_ctx->config : NULL; + int is_newer = check_timestamp_is_newer(ctx, last_modified); + int not_using_fleet = (is_fleet_config(ctx, cur_cfg) == FLB_FALSE);
1194-1196: Parse Last-Modified as GMT (avoid TZ/DST skew).mktime() interprets tm as local time. Last‑Modified is GMT; use timegm/_mkgmtime to avoid skew.
Apply this diff:
- flb_strptime(fbit_last_modified, "%a, %d %B %Y %H:%M:%S GMT", &tm_last_modified); - last_modified = mktime(&tm_last_modified.tm); + /* RFC 7231 uses abbreviated month (%b) and GMT; convert as UTC */ + if (flb_strptime(fbit_last_modified, "%a, %d %b %Y %H:%M:%S GMT", &tm_last_modified) == NULL) { + goto client_error; + } +#if defined(_WIN32) + last_modified = _mkgmtime(&tm_last_modified.tm); +#else + last_modified = timegm(&tm_last_modified.tm); +#endif
1197-1200: Tighten the comment to mirror the predicate.Make the intent crystal clear for future readers.
Apply this diff:
- /* Check if there are existing files with timestamps >= new timestamp - * OR if we're not currently using a fleet config (first run scenario) - */ + /* Proceed if (is_newer || not_using_fleet): allow bootstrap while preserving timestamp gating */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)
src/flb_lib.c (1)
flb_context_get(993-999)
🔇 Additional comments (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (1)
1197-1219: Good bootstrap gate: allow create when not currently using fleet config.Condition now correctly permits first‑run creation even on equal timestamps; logs are clear.
ThomasBrady
force-pushed
the
thomasbrady/fix-empty-fleet-dir-no-upgrade
branch
2 times, most recently
from
22bc3a6 to
583b75b
Compare
ThomasBrady
force-pushed
the
thomasbrady/fix-empty-fleet-dir-no-upgrade
branch
from
583b75b to
6921a62
Compare
Contributor
Author
Thanks for TAL, updated. |
ThomasBrady
force-pushed
the
thomasbrady/fix-empty-fleet-dir-no-upgrade
branch
from
6921a62 to
bb8136e
Compare
Signed-off-by: Thomas Brady <[email protected]>
ThomasBrady
force-pushed
the
thomasbrady/fix-empty-fleet-dir-no-upgrade
branch
from
bb8136e to
29050d6
Compare
pwhelan
changed the title
Member
|
I think we will need a backport of this one for branch 4.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Issue:
The Calyptia Fleet plugin failed to apply configurations on first run when starting with an empty fleet directory. The plugin would download a configuration but reject it because the timestamp comparison logic (check_timestamp_is_newer()) would find the just-downloaded file and determine its timestamp was not newer than itself.
Root Cause:
The timestamp comparison was too strict - it only allowed new configs if their timestamp was strictly newer than existing files. This prevented the initial fleet configuration from being applied when no fleet config was currently active.
Fix:
Modified the condition in get_calyptia_file() to also check if we're currently using a fleet configuration. The config is now applied if:
This allows the fleet plugin to properly bootstrap itself on Windows when starting with an empty directory while maintaining the existing timestamp-based update logic for subsequent configuration changes.
Excerpts from logs demonstrating the issue and fix:
Empty Fleet Directory Error Log (v4.0.9)
Initial startup with empty fleet directory:
First download succeeds:
Reload fails:
Subsequent attempts blocked:
This pattern repeats every 15 seconds:
Fixed logs:
Initial startup - same empty directory errors:
Fix applied - detects not using fleet config:
Successful reload:
Configuration successfully applied:
Subsequent checks correctly skip same timestamp:
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
Bug Fixes
New Features