out_gcs: Implement Google Cloud Storage(gcs) plugin#11792
Open
cosmo0920 wants to merge 3 commits into
Open
Conversation
cosmo0920
requested review from
celalettin1286,
edsiper,
niedbalski and
patrick-stephens
as code owners
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Reviewed commit: ffe6fb0502
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@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: 6
🧹 Nitpick comments (2)
tests/runtime/out_gcs.c (2)
65-67: ⚡ Quick winClean up the temporary store directory after the test.
The test creates a temporary directory with
mkdtemp()but never removes it, leaving files in/tmp. Add cleanup code before returning.🧹 Proposed cleanup
After
flb_destroy(ctx)on line 98, add:/* Clean up temporary store directory */ char rm_cmd[256]; snprintf(rm_cmd, sizeof(rm_cmd), "rm -rf %s", store_dir); system(rm_cmd);Or use a more portable approach:
/* Clean up temporary store directory */ flb_utils_recursive_unlink(store_dir);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/runtime/out_gcs.c` around lines 65 - 67, The test creates a temp directory via store_dir and mkdtemp but never removes it; after flb_destroy(ctx) (the test teardown) add code to recursively remove store_dir — either by invoking flb_utils_recursive_unlink(store_dir) if available or by executing a safe platform call to remove the directory contents — ensuring the cleanup runs before the test returns so the /tmp directory is not left behind.
19-21: ⚡ Quick winClean up the temporary store directory after the test.
The test creates a temporary directory with
mkdtemp()but never removes it, leaving files in/tmp. Add cleanup code before returning.🧹 Proposed cleanup
After
flb_destroy(ctx)on line 51, add:/* Clean up temporary store directory */ char rm_cmd[256]; snprintf(rm_cmd, sizeof(rm_cmd), "rm -rf %s", store_dir); system(rm_cmd);Or use a more portable approach:
/* Clean up temporary store directory */ flb_utils_recursive_unlink(store_dir);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/runtime/out_gcs.c` around lines 19 - 21, The test creates a temporary directory (store_dir via mkdtemp) but never removes it; after flb_destroy(ctx) in the test teardown add cleanup to remove store_dir—either call flb_utils_recursive_unlink(store_dir) if available, or invoke a safe removal (e.g., build an rm -rf command with snprintf into a buffer and call system) to recursively delete the temporary directory; ensure you reference the same store_dir variable and perform the cleanup before the test returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_gcs/gcs_store.c`:
- Around line 55-68: The gcs_store_init currently creates a shared file store at
ctx->store_dir and always opens the same stream name "gcs_upload_buffer",
causing different out_gcs instances to share and drain each other's buffers;
update gcs_store_init to derive a unique stream namespace per instance (e.g.,
incorporate instance-specific fields such as ctx->name, ctx->bucket or an
internal instance id) and use that string when calling flb_fstore_stream_create
(ctx->fs_stream) so the sequence-index and buffered files live in an
instance-specific namespace; ensure the chosen identifier is stable across
restarts for that instance and still kept with ctx so cleanup and
flb_fstore_destroy behave correctly.
In `@plugins/out_gcs/gcs.c`:
- Around line 757-790: The generated key currently calls flb_get_s3_key(..., 0)
before ctx->seq_index is incremented, so $INDEX never appears in the object
name; move/perform the seq handling prior to key generation: if
ctx->key_fmt_has_seq_index, increment ctx->seq_index (or compute the next
sequence value) and pass that value as the final argument to flb_get_s3_key
instead of 0 so gcs_key/gcs_key_final includes the sequence, and keep the
existing write_seq_index(ctx->seq_index_file, ctx->seq_index) call to persist
the new value after successful use; update references to gcs_key, gcs_key_final,
ctx->seq_index, flb_get_s3_key and write_seq_index accordingly.
- Around line 56-82: The static header templates (content_type_header,
canned_acl_header, content_md5_header, storage_class_header) are mutated per
request and can race across out_gcs instances; move these into
gcs_upload_object() as local (stack) variables, initialize their
.key/.key_len/.val/.val_len per-request there, and use the local instances when
building the request instead of the global symbols; also remove or replace any
other references to the static globals so no code mutates shared header state.
- Around line 1084-1093: Check that flb_oauth2_create(...) and
flb_upstream_create(...) succeeded and fail init immediately if either returns
NULL: after assigning ctx->o = flb_oauth2_create(...) verify ctx->o != NULL (log
an error via flb_plg_error or process logger) and goto error on failure; do the
same after ctx->u = flb_upstream_create(...) to ensure ctx->u != NULL (and goto
error). Also ensure you clean up any partially initialized resources (e.g.,
destroy the mutex if token_mutex_initialized was set) when jumping to the error
path so there are no leaks.
- Around line 591-594: attach_recovered_chunk() backdates recovered entries but
add_to_queue() always overwrites entry->upload_time, causing recovered files to
be delayed; modify add_to_queue() so it only sets entry->upload_time when it is
not already initialized/backdated (e.g., if entry->upload_time == 0 or not in
the past), otherwise preserve the existing upload_time, then add the entry to
ctx->upload_queue; update the logic around upload_time in add_to_queue() (and
any callers) to ensure process_upload_queue() can pick up backdated entries
immediately.
- Around line 721-725: The code currently returns only the transport result from
flb_http_do (ret) which treats any completed HTTP response—including
401/403/5xx—as success; update the upload call path to inspect the HTTP response
status after flb_http_do (use the HTTP client/response available via the client
variable c or bytes/response fields) and treat only 2xx statuses as success
(return an error/non-zero for non-2xx so process_upload_queue() will retry),
while still calling flb_http_client_destroy(c) and
flb_upstream_conn_release(u_conn) to clean up; adjust the return value logic
around flb_http_do, the HTTP client 'c', and the response status check so that
non-2xx responses are not acknowledged as successful.
---
Nitpick comments:
In `@tests/runtime/out_gcs.c`:
- Around line 65-67: The test creates a temp directory via store_dir and mkdtemp
but never removes it; after flb_destroy(ctx) (the test teardown) add code to
recursively remove store_dir — either by invoking
flb_utils_recursive_unlink(store_dir) if available or by executing a safe
platform call to remove the directory contents — ensuring the cleanup runs
before the test returns so the /tmp directory is not left behind.
- Around line 19-21: The test creates a temporary directory (store_dir via
mkdtemp) but never removes it; after flb_destroy(ctx) in the test teardown add
cleanup to remove store_dir—either call flb_utils_recursive_unlink(store_dir) if
available, or invoke a safe removal (e.g., build an rm -rf command with snprintf
into a buffer and call system) to recursively delete the temporary directory;
ensure you reference the same store_dir variable and perform the cleanup before
the test returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b203c4a8-5359-459a-b781-d5c061e3be04
📒 Files selected for processing (10)
CMakeLists.txtcmake/plugins_options.cmakeplugins/CMakeLists.txtplugins/out_gcs/CMakeLists.txtplugins/out_gcs/gcs.cplugins/out_gcs/gcs.hplugins/out_gcs/gcs_store.cplugins/out_gcs/gcs_store.htests/runtime/CMakeLists.txttests/runtime/out_gcs.c
cosmo0920
force-pushed
the
cosmo0920-implement-out_gcs-plugin
branch
from
ffe6fb0 to
018a263
Compare
cosmo0920
force-pushed
the
cosmo0920-implement-out_gcs-plugin
branch
from
2fb74aa to
b2635b7
Compare
cosmo0920
force-pushed
the
cosmo0920-implement-out_gcs-plugin
branch
from
616c9d5 to
be98d51
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-implement-out_gcs-plugin
branch
from
77b1242 to
ec5e012
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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
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.
This PR adds Google Cloud Storage (gcs) plugin to send logs type of events into GCS buckets.
Previously, we have a PR for this purpose: #6984
But the PR seems to be stale so I had taken over that work.
The issue should be related #1032 and it's a long standing issue.
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:
With macOS's leaks command, there's no leaks reported:
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
Build
Tests