out_s3: added blob handling feature #9907

leonardo-albertovich · 2025-02-03T12:59:37Z

No description provided.

Signed-off-by: Leonardo Alminana <[email protected]>

swapneils

Added some comments that I'd prefer to clarify and/or change before merging this.

Curious why the ubuntu unit tests are failing, as well.

swapneils · 2025-03-18T20:04:37Z

src/aws/flb_aws_util.c

+
+    /* A match against "$TAG[" indicates an invalid or out of bounds tag part. */
+    if (strstr(s3_key, tmp)){
+        flb_warn("[s3_key] Invalid / Out of bounds tag part: At most 10 tag parts "


Nit: Is going above 10 tag parts the most likely cause of this issue? The "first part" of the error message implies that tags in the wrong place also trigger this check, in which case the "second part" should mention that in more detail as well to avoid confusing operators.

swapneils · 2025-03-18T20:06:18Z

src/aws/flb_aws_util.c

+
+    valid_blob_path = (char *) blob_path;
+
+    while (*valid_blob_path == '.' ||


Can this value ever use the ../ format? If so the semantics here would ignore that motion, which is incorrect.

swapneils · 2025-03-18T20:12:56Z

plugins/out_s3/s3.h

+                              struct multipart_upload *m_upload,
+                              char *pre_signed_url);
+
+int abort_multipart_upload(struct flb_s3 *ctx,


Why are we creating this function? I see we're using it to abort multipart blob uploads in s3.c, but why is that needed here?

swapneils · 2025-03-18T20:24:55Z

src/flb_blob_db.c

+
+int flb_blob_db_lock(struct flb_blob_db *context)
+{
+    return flb_lock_acquire(&context->global_lock,


As I understand every blob publish operation is dependent on manipulating the same sqlite table. Do we know how this affects performance when parallelizing publishes? Is there a way to get a narrower lock here?

swapneils · 2025-03-18T20:36:30Z

src/flb_blob_db.c

+
+int flb_blob_db_unlock(struct flb_blob_db *context)
+{
+    return flb_lock_release(&context->global_lock,


Nit: out of curiosity, why are we using global_lock here but db_lock in azure_blob_db.c?

swapneils · 2025-03-18T20:42:27Z

plugins/out_s3/s3_multipart.c

+
+    s3_client = ctx->s3_client;
+    if (s3_plugin_under_test() == FLB_TRUE) {
+        /* c = mock_s3_call("TEST_ABORT_MULTIPART_UPLOAD_ERROR", "AbortMultipartUpload"); */


Nit: Do we need this line?

swapneils · 2025-03-18T20:48:46Z

plugins/out_s3/s3.h

+    time_t upload_parts_freshness_threshold;
+    int file_delivery_attempt_limit;
+    int part_delivery_attempt_limit;
+    flb_sds_t authorization_endpoint_url;


How are we using this authorization endpoint? Isn't the process of publishing binary data to S3 equivalent to string data, +/- the declared body type in the HTTP requests?

swapneils · 2025-03-18T21:02:34Z

plugins/out_s3/s3.c

+    sched = flb_sched_ctx_get();
+
+    /* convert from seconds to milliseconds (scheduler needs ms) */
+    ms = ctx->upload_parts_timeout * 1000;


Is this the correct semantics?

From the description of upload_parts_timeout below it seems like a maximum age to retry blob publishing, past which we will drop a blob instead of retrying.
As I understand the code, here we're instead inserting blob parts into the database and then attempting to publish them every upload_parts_timeout seconds, dropping them on the first failure.

ShelbyZ · 2025-03-18T20:00:08Z

src/aws/flb_aws_util.c

@@ -732,6 +732,203 @@ char* strtok_concurrent(
 #endif
 }

+/* Constructs S3 object key as per the blob format. */
+flb_sds_t flb_get_s3_blob_key(const char *format,


A majority of this is duplicated in flb_get_s3_key(...), does it make sense to try and introduce two helpers:

https://github.com/fluent/fluent-bit/blob/leonardo-master-s3-blob/src/aws/flb_aws_util.c#L936-L1053 - initial validation/replacement

https://github.com/fluent/fluent-bit/blob/leonardo-master-s3-blob/src/aws/flb_aws_util.c#L1080-L1100 - random alpha replacement

This could avoid possible divergence between the two methods over time

ShelbyZ · 2025-03-18T20:26:33Z

include/fluent-bit/flb_blob_db.h

+                                char *tag,
+                                char *source,
+                                char *destination,
+                                char *path,


There are other instances where we are using cfl_sds_t in place of char*, should we be consistent and migrate all char* here to cfl_sds_t?

ShelbyZ · 2025-03-18T20:27:48Z

src/flb_blob_db.c

+
+/*  Fluent Bit
+ *  ==========
+ *  Copyright (C) 2015-2024 The Fluent Bit Authors


Should the copyright block include our current year? 🫣

ShelbyZ · 2025-03-18T20:30:56Z

src/flb_blob_db.c

+    }
+
+
+    /* file destination update  */


Should this comment include remote-id (similar to file part below)?

ShelbyZ · 2025-03-18T20:32:15Z

src/flb_blob_db.c

+        return FLB_BLOB_DB_ERROR_PREPARING_STATEMENT_GET_NEXT_FILE_PART;
+    }
+
+    result = sqlite3_prepare_v2(context->db->handler,


Should these next results contain a comment above (similar to prior prepared statement blocks)?

ShelbyZ · 2025-03-18T20:52:03Z

src/flb_blob_db.c

+
+static int flb_blob_db_file_reset_part_upload_states(struct flb_blob_db *context,
+                                                     uint64_t id,
+                                                     char *path)


Similar to above comments on unused parameters

ShelbyZ · 2025-03-18T20:52:28Z

src/flb_blob_db.c

+
+int flb_blob_db_file_reset_upload_states(struct flb_blob_db *context,
+                                         uint64_t id,
+                                         char *path)


Similar to above comments on unused parameters

ShelbyZ · 2025-03-18T20:53:11Z

src/flb_blob_db.c

+                                 uint64_t part_id,
+                                 size_t offset_start,
+                                 size_t offset_end,
+                                 int64_t *out_id)


Similar to above comments on unused parameters

ShelbyZ · 2025-03-18T21:35:02Z

plugins/out_s3/s3.c

+
+    flb_blob_db_lock(&ctx->blob_db);
+
+    while (1) {


A large portion of these while(1) loops are duplicating code:

https://github.com/fluent/fluent-bit/blob/leonardo-master-s3-blob/plugins/out_s3/s3.c#L2582-L2645

https://github.com/fluent/fluent-bit/blob/leonardo-master-s3-blob/plugins/out_s3/s3.c#L2674-L2736

Can we consider moving to a helper method to avoid the duplication?

ShelbyZ · 2025-03-18T21:43:17Z

plugins/out_s3/s3.c

+    return ret;
+}
+
+static int blob_fetch_multipart_complete_pre_signed_url(struct flb_s3 *context,


These fetch_*_pre_signed_url methods are near duplicates with minor changes to the tmp (path variable), can we consider using a helper method for getting the presigned url and have each method handle their own tmp/path generation to reduce code?

leonardo-albertovich added 6 commits February 3, 2025 13:55

flb_blob_db: initial commit of the blob database component

622d278

Signed-off-by: Leonardo Alminana <[email protected]>

signv4: added missing method support

14476ba

Signed-off-by: Leonardo Alminana <[email protected]>

sqldb: added locking mechanics

151a2cc

Signed-off-by: Leonardo Alminana <[email protected]>

aws_util: added blob specific s3 key generation

960f3e8

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: added blob support

e73d42a

Signed-off-by: Leonardo Alminana <[email protected]>

build: added blob database component

dda8050

Signed-off-by: Leonardo Alminana <[email protected]>

leonardo-albertovich requested review from PettitWesley, edsiper, fujimotos and koleini as code owners February 3, 2025 12:59

github-actions bot added the docs-required label Feb 3, 2025

leonardo-albertovich had a problem deploying to pr February 3, 2025 13:00 — with GitHub Actions Failure

out_s3: added missing windows header

285144c

Signed-off-by: Leonardo Alminana <[email protected]>

leonardo-albertovich temporarily deployed to pr February 3, 2025 14:18 — with GitHub Actions Inactive

out_s3: added missing TLS initializer

a768345

Signed-off-by: Leonardo Alminana <[email protected]>

leonardo-albertovich temporarily deployed to pr February 3, 2025 14:40 — with GitHub Actions Inactive

leonardo-albertovich temporarily deployed to pr February 3, 2025 15:03 — with GitHub Actions Inactive

leonardo-albertovich added 5 commits February 5, 2025 12:15

out_s3: removed erroneously included code (CID 532473)

d083add

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: fixed potential memory leak (CID 532470)

ca26268

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: fixed compiler warning (CID 532472)

bde4f84

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: fixed compiler warning (CID 532471)

ff2ab43

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: fixed NULL dereferences (CID 532469)

87436be

Signed-off-by: Leonardo Alminana <[email protected]>

out_s3: fixed compiler warning (CID 532468)

52c2339

Signed-off-by: Leonardo Alminana <[email protected]>

leonardo-albertovich temporarily deployed to pr February 5, 2025 12:02 — with GitHub Actions Inactive

leonardo-albertovich temporarily deployed to pr February 5, 2025 12:24 — with GitHub Actions Inactive

leonardo-albertovich temporarily deployed to pr February 5, 2025 12:25 — with GitHub Actions Inactive

out_s3: pre-signed url support addition

039bd43

Signed-off-by: Leonardo Alminana <[email protected]>

leonardo-albertovich temporarily deployed to pr February 18, 2025 14:49 — with GitHub Actions Inactive

leonardo-albertovich temporarily deployed to pr February 18, 2025 15:12 — with GitHub Actions Inactive

leonardo-albertovich temporarily deployed to pr February 18, 2025 15:17 — with GitHub Actions Inactive

swapneils suggested changes Mar 18, 2025

View reviewed changes

ShelbyZ suggested changes Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

out_s3: added blob handling feature #9907

out_s3: added blob handling feature #9907

leonardo-albertovich commented Feb 3, 2025

swapneils left a comment

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

swapneils Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025

ShelbyZ Mar 18, 2025


		valid_blob_path = (char *) blob_path;

		while (*valid_blob_path == '.' \|\|

out_s3: added blob handling feature #9907

Are you sure you want to change the base?

out_s3: added blob handling feature #9907

Conversation

leonardo-albertovich commented Feb 3, 2025

swapneils left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment