SDK | Fix log replication sdk issue #9213
Conversation
WalkthroughReplace AWS SDK v2 S3 usage with NooBaa S3 v3 client, convert log retrieval/parsing to async string-based processing, update S3 client construction plumbing, and adjust NotFound detection in replication utilities and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor BG as BG Service
participant RLP as ReplicationLogParser
participant S3 as NooBaa S3 (SDK v3)
rect rgb(237,243,254)
note right of S3: Client created via noobaa_s3_client.get_s3_client_v3_params
end
BG->>RLP: get_aws_log_candidates()
RLP->>S3: listObjectsV2(params)
S3-->>RLP: object list
loop for each object
RLP->>S3: getObject(key)
S3-->>RLP: Body (stream)
RLP->>RLP: await Body.transformToString()
RLP->>RLP: await aws_parse_log_object(log_string, ...)
end
RLP-->>BG: candidates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/util/cloud_utils.js (1)
188-198: Switching to noobaa_s3_client v3 params here looks right; tls: false is correct for INTERNAL S3.
- Matches the repo-wide v3 migration and returns a compatible S3 client.
- Using get_requestHandler_with_suitable_agent(endpoint) is appropriate.
- Per prior learning, INTERNAL 'api: s3' endpoints are non-secure, so tls: false is expected.
🧹 Nitpick comments (3)
src/server/bg_services/replication_log_parser.js (3)
271-276: ResponseContentType is unnecessary for getObject here.It doesn’t affect the returned Body and can be dropped to avoid confusion.
- const res = await s3.getObject({ + const res = await s3.getObject({ Bucket: bucket, Key: key, - ResponseContentType: 'json' });
293-295: Add a safe fallback when Body.transformToString is unavailable; specify encoding.Some runtimes or polyfills may not expose transformToString; also make encoding explicit.
-async function aws_parse_log_object(logs, log_object, sync_deletions, obj_prefix_filter) { - const log_string = await log_object.Body.transformToString(); +async function aws_parse_log_object(logs, log_object, sync_deletions, obj_prefix_filter) { + if (!log_object?.Body) { + dbg.error('aws_parse_log_object: missing Body on log object'); + return; + } + const toStringFromBody = async body => { + if (typeof body.transformToString === 'function') return body.transformToString('utf-8'); + // Fallback: collect stream + return new Promise((resolve, reject) => { + const chunks = []; + body.on('data', c => chunks.push(Buffer.isBuffer(c) ? c : Buffer.from(c))); + body.on('end', () => resolve(Buffer.concat(chunks).toString('utf-8'))); + body.on('error', reject); + }); + }; + const log_string = await toStringFromBody(log_object.Body);
374-385: SignatureVersion correctly honored; no fixes needed
The wrapper’sshould_use_sdk_v2checksparams.signatureVersion === 'v2'and falls back to the AWS SDK v2 client as intended.
• Optional: add atls: endpoint.startsWith('https://')property for explicit clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/bg_services/replication_log_parser.js(8 hunks)src/server/utils/replication_utils.js(1 hunks)src/util/cloud_utils.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T12:59:51.543Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
Applied to files:
src/util/cloud_utils.js
🧬 Code graph analysis (2)
src/util/cloud_utils.js (3)
src/server/bg_services/replication_log_parser.js (1)
noobaa_s3_client(13-13)src/sdk/namespace_s3.js (1)
noobaa_s3_client(14-14)src/sdk/object_sdk.js (1)
noobaa_s3_client(28-28)
src/server/bg_services/replication_log_parser.js (3)
src/util/cloud_utils.js (5)
noobaa_s3_client(16-16)require(9-9)require(13-13)require(14-14)s3(71-87)src/server/system_services/account_server.js (10)
noobaa_s3_client(26-26)require(13-13)require(23-23)require(24-24)require(25-25)s3(1075-1075)params(472-472)params(873-873)params(1137-1137)cloud_utils(16-16)src/server/system_services/bucket_server.js (6)
noobaa_s3_client(41-41)require(19-19)require(40-40)s3(454-454)s3(1304-1304)cloud_utils(25-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (4)
src/server/bg_services/replication_log_parser.js (4)
13-14: Importing noobaa_s3_client for v3 S3 is consistent with the migration.No issues.
63-64: Awaiting aws_parse_log_object is necessary after making it async.Good catch to prevent races and partial parsing.
229-235: Doc update to v3 S3 type is accurate.Minor but correct.
252-253: Switch to await s3.listObjectsV2(params) aligns with v3 S3 API.
- Using StartAfter with a normalized prefix is fine when ContinuationToken is absent.
- MaxKeys: 1 matches the loop’s throttled scan logic.
naveenpaul1
force-pushed
the
replication_sdk_fix
branch
from
76640b4 to
5b5d651
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/unit_tests/internal/test_bucket_log_based_replication.test.js (2)
260-260: Fix concatenated log lines; they’re missing newlines and break parsing/counts.Two log entries are glued on one line. Insert explicit newlines so each entry is parsed independently.
- aaa test.bucket [12/Feb/2023:09:08:28 +0000] ... TLSv1.2 - - aaa test.bucket [13/Feb/2023:09:08:56 +0000] ... + aaa test.bucket [12/Feb/2023:09:08:28 +0000] ... TLSv1.2 - - + aaa test.bucket [13/Feb/2023:09:08:56 +0000] ... - aaa test.bucket [13/Feb/2023:16:08:56 +0000] ... TLSv1.2 - - aaa test.bucket [13/Feb/2023:15:08:28 +0000] ... - - - aaa test.bucket [13/Feb/2023:15:08:28 +0000] ... TLSv1.2 - - + aaa test.bucket [13/Feb/2023:16:08:56 +0000] ... TLSv1.2 - - + aaa test.bucket [13/Feb/2023:15:08:28 +0000] ... - - + aaa test.bucket [13/Feb/2023:15:08:28 +0000] ... TLSv1.2 - -After fixing, please re-evaluate the exact expected
logs.length. Consider asserting candidates instead of raw log count for stability.Also applies to: 263-263
273-279:create_candidatesreturns a log per key, not an array; indexing with [0] is wrong.This makes the assertion ineffective and can mask bugs.
- for (const key in candidates) { - if (candidates[key][0]) { - expect(candidates[key][0].action).toEqual(action_candidates[key]); - } - } + for (const key of Object.keys(candidates)) { + expect(candidates[key].action).toEqual(action_candidates[key]); + }
🧹 Nitpick comments (2)
src/server/bg_services/replication_log_parser.js (2)
289-292: JSDoc nit: parameter description doesn’t match type.It’s a string now, not an “AWS log object”.
- * @param {String} log_string - AWS log object + * @param {string} log_string - Raw AWS server access log text
271-276: Minor:ResponseContentType: 'json'on GetObject is unnecessary.We read a text body and parse it ourselves; setting a response content type isn’t needed and may confuse future readers.
- const res = await s3.getObject({ - Bucket: bucket, - Key: key, - ResponseContentType: 'json' - }); + const res = await s3.getObject({ Bucket: bucket, Key: key });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/bg_services/replication_log_parser.js(8 hunks)src/server/utils/replication_utils.js(1 hunks)src/test/unit_tests/internal/test_bucket_log_based_replication.test.js(4 hunks)src/util/cloud_utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util/cloud_utils.js
- src/server/utils/replication_utils.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/internal/test_bucket_log_based_replication.test.js
🧠 Learnings (1)
📚 Learning: 2025-06-17T12:59:51.543Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
Applied to files:
src/server/bg_services/replication_log_parser.js
🧬 Code graph analysis (1)
src/server/bg_services/replication_log_parser.js (2)
src/util/cloud_utils.js (5)
noobaa_s3_client(16-16)require(9-9)require(13-13)require(14-14)s3(71-87)src/server/system_services/account_server.js (10)
noobaa_s3_client(26-26)require(13-13)require(23-23)require(24-24)require(25-25)s3(1075-1075)params(472-472)params(873-873)params(1137-1137)cloud_utils(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/server/bg_services/replication_log_parser.js (1)
374-384: Confirm TLS handling for internal NooBaa S3 endpoints.Per our prior learning, internal NooBaa S3 endpoints are non-secure (tls: false). Ensure
get_s3_client_v3_params/get_requestHandler_with_suitable_agentenforce that for INTERNAL endpoints; otherwise explicitly set it here.We used your stored preference from “Learnings”: internal NooBaa S3 uses non-TLS by design.
src/test/unit_tests/internal/test_bucket_log_based_replication.test.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
replication_sdk_fix
branch
from
5b5d651 to
66a215e
Compare
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
replication_sdk_fix
branch
from
66a215e to
e8e5927
Compare
2 participants
Describe the Problem
Log based deletion sync fails between AWS Namespacestore buckets
Explain the Changes
err?.namenoobaa_s3_client.get_s3_client_v3_params()Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Bug Fixes
Refactor
Chores