feat(clp-package): Add support for clp-json to ingest logs from S3. by haiqi96 · Pull Request #651 · y-scope/clp

haiqi96 · 2025-01-03T19:32:32Z

Description

This PR adds s3 ingestion support for clp-s package. Currently, we support compressing with one s3_url(with prefix matching) at a time. User can either specify the credentials through commandline, or via a credentials file. If no credentials are provided, the package will attempt to use the associated IAM if there is any.

The PR contains the following changes:

Introduces S3InputConfig and FsInputConfig for different ingestion path. Updated compression execution script to handle different input.
Updated compression scheduler to handle scheduling for s3 objects. Note to avoid excessive change, we let s3 objects reuse the existing metadata class for FilePath.
Updated compress.sh interface to allow specifying if the input is from local filesystem (fs) or s3(s3). Simplified native/compress.sh scripts' interface to always accept a target_list.
Update execution container to install necessary dependency for curl lib

The design decisions involved in this PR is documented in this doc

Due to time limitation, we leave the following items as todo for future:

If users provide invalid S3 information, such as incorrect credentials or a non-existent bucket, the issue is only detected when the package attempts to schedule the job. Additionally, if the provided S3 credentials lack sufficient privileges (e.g., list_object is allowed, but get_object is not), the package will return unhelpful error messages to the web UI ("see error log.txt blabla"), making it confusing for user.

Ideally, invalid S3 configurations should be detected at package startup to provide immediate and actionable feedback.

Validation performed

Manually verified that both fs compression and s3 compression works from the commandline.

Validations done after code review:
Launched compression successfully with

FS input, with --path-list that contains 2 files.
FS input, with PATH positional argument
Valid S3 URL with aws_key and id specified through argument
Valid S3 URL with aws_key and id specified through credential file

Launched compression with the following configuration and confirmed that errors is properly reported:

Valid S3 URL with invalid credential file.
Invalid S3 URL that doesn't resolve to any objects
Invalid S3 URL with unexisting bucket
S3 URL with bucket that can't be accessed.
S3 URL with no permission.

Also sanity checked the error for writing to S3

Setting the destination to a path without write permission. verified the error message makes sense.
Setting the archive_output with invalid s3 credentials. verified the error message makes sense.

Summary by CodeRabbit

Release Notes

New Features
- Added support for compressing files from both filesystem and Amazon S3 sources.
- Introduced new functions for generating log lists for filesystem and S3 inputs.
- Enhanced configuration management with new input types for compression tasks.
Improvements
- Refined input validation and error handling for compression processes.
- Improved task routing and execution for compression jobs.
- Streamlined command generation for S3 operations and enhanced metadata handling.
Infrastructure
- Updated Docker base images with additional certificate packages.
Bug Fixes
- Resolved issues with path resolution and input type handling.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

…ss/fs_compression_task.py

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

kirkrodrigues

Minor comments.

For the PR title, how about:

feat(clp-package): Add support for clp-json to ingest logs from S3.

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

components/clp-py-utils/clp_py_utils/s3_utils.py (3)
1-14: Organize imports by standard library, third-party, and local packages.

Consider reorganizing the imports into three groups separated by a blank line:

Standard library imports (configparser, re, pathlib, typing)

Third-party imports (boto3, botocore)

Local imports (job_orchestration, clp_py_utils)
 import configparser
 import re
 from pathlib import Path
 from typing import List, Tuple
+
 import boto3
 from botocore.config import Config
+
 from job_orchestration.scheduler.job_config import S3InputConfig
 from clp_py_utils.clp_config import S3Config
 from clp_py_utils.compression import FileMetadata
20-27: Enhance docstring with specific return value types.

The docstring should specify the exact types being returned and clarify that the values are strings.
     """
     Parses the `aws_access_key_id` and `aws_secret_access_key` of `user` from the given
     credentials_file_path.
     :param credentials_file_path:
     :param user:
-    :return: A tuple of (aws_access_key_id, aws_secret_access_key)
+    :return: A tuple of (str, str) containing (aws_access_key_id, aws_secret_access_key)
     :raises: ValueError if the file doesn't exist, or doesn't contain valid aws credentials.
     """
74-75: Improve error message with URL format examples.

The error message should include examples of valid URL formats to help users understand the expected format.
     if match is None:
-        raise ValueError(f"Unsupported URL format: {s3_url}")
+        raise ValueError(
+            f"Unsupported URL format: {s3_url}. Expected formats:\n"
+            "- Host-style: https://<bucket>.s3.<region>.amazonaws.com/<key>\n"
+            "- Path-style: https://s3.<region>.amazonaws.com/<bucket>/<key>"
+        )

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 996692e and 351f239.

📒 Files selected for processing (4)

components/clp-package-utils/clp_package_utils/scripts/compress.py (4 hunks)
components/clp-package-utils/clp_package_utils/scripts/native/compress.py (4 hunks)
components/clp-py-utils/clp_py_utils/s3_utils.py (2 hunks)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (11 hunks)

👮 Files not reviewed due to content moderation or server errors (3)

components/clp-package-utils/clp_package_utils/scripts/native/compress.py
components/clp-package-utils/clp_package_utils/scripts/compress.py
components/job-orchestration/job_orchestration/executor/compress/compression_task.py

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: lint-check (ubuntu-latest)
GitHub Check: lint-check (macos-latest)

🔇 Additional comments (3)

components/clp-py-utils/clp_py_utils/s3_utils.py (3)

88-98: LGTM! Input validation is thorough.

The function properly validates all required parameters before generating the URL.

101-137: LGTM! Pagination and metadata handling are well implemented.

The function correctly handles pagination and object metadata retrieval, with appropriate filtering for directory-like paths.

Line range hint 140-176: LGTM! Error handling and retry configuration are well implemented.

The function includes appropriate validation, retry configuration, and clear error messages.

…-scope#651) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

haiqi96 and others added 30 commits December 11, 2024 12:45

First version backup

ca46dca

Small refactor

b763e8b

First trial for new config

4e9529c

Further refactor and polishing

e9cdea4

Another small refactor

9ba0a38

small refactor again

58befef

Combine s3 utils

35ec0c3

Support handling S3 error message

5d57b10

Slight logging modification

9991307

Linter

5d23790

Add extra verification

b4bb2af

Update components/clp-py-utils/clp_py_utils/clp_config.py

f41c558

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

do nothing for now

ce5a667

backup changes for worker config

f05dc88

More support

abf5dde

Remove unnecssary change

7d34456

Linter

a7afd0d

Handle mount for fs & S3

99d3094

Linter

1afed1a

Remove unused functions

1de661a

Update components/job-orchestration/job_orchestration/executor/compre…

ce3de98

…ss/fs_compression_task.py

simplify worker config

f49664f

polishing

046cdcb

linter

242dec2

Apply suggestions from code review

ed280cb

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

Fix easier ones

0788e59

Backup changes

c198f27

Small fixes

4819f76

fixes

e5f43fb

add safeguard for archive update failure

1246062

kirkrodrigues reviewed Jan 16, 2025

View reviewed changes

haiqi96 commented Jan 16, 2025

View reviewed changes

Comment thread components/clp-package-utils/clp_package_utils/scripts/compress.py Outdated

Apply suggestions from code review

351f239

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

coderabbitai Bot reviewed Jan 16, 2025

View reviewed changes

haiqi96 changed the title ~~feat(clp-package): Add support for clp-s s3 ingestion~~ feat(clp-package): Add support for clp-json to ingest logs from S3. Jan 16, 2025

haiqi96 requested a review from kirkrodrigues January 16, 2025 17:43

kirkrodrigues approved these changes Jan 16, 2025

View reviewed changes

haiqi96 merged commit fae7fcc into y-scope:main Jan 16, 2025

coderabbitai Bot mentioned this pull request Mar 26, 2025

feat(clp-package)!: Refactor input log config so that S3 source locations are specified in the config file rather than through the CLI. #788

Merged

3 tasks

This was referenced Apr 21, 2025

feat(clp-package): Add support for more AWS authentication methods. #743

Merged

feat(clp-package)!: Refactor S3 log-ingestion configuration to use S3 URL as compression script argument (fixes #842). #852

Merged

haiqi96 deleted the s3_scheduler branch June 16, 2025 20:05

This was referenced Jul 24, 2025

feat(integration-tests): Add infrastructure for running integration tests; Add basic integration tests for clp & clp-s compression. #1100

Merged

refactor(clp): Prepare parsing & search code for deduplication with clp-s: #1138

Merged

This was referenced Aug 21, 2025

feat(package): Mark running jobs as failed when schedulers start. #1208

Merged

feat(package): Add --verbose option to package_utils scripts and improve error diagnostics (fixes #1081). #1212

Merged

coderabbitai Bot mentioned this pull request Sep 2, 2025

fix(package): Fail job and report failure to user for compression jobs that encounter at least one invalid input path (fixes #308). #1125

Merged

3 tasks

junhaoliao pushed a commit to junhaoliao/clp that referenced this pull request May 17, 2026

feat(clp-package): Add support for clp-json to ingest logs from S3. (y…

5ba1d27

…-scope#651) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clp-package): Add support for clp-json to ingest logs from S3.#651

feat(clp-package): Add support for clp-json to ingest logs from S3.#651
haiqi96 merged 79 commits into
y-scope:mainfrom
haiqi96:s3_scheduler

haiqi96 commented Jan 3, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

kirkrodrigues left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

haiqi96 commented Jan 3, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Validation performed

Summary by CodeRabbit

Release Notes

Uh oh!

kirkrodrigues left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

haiqi96 commented Jan 3, 2025 •

edited by coderabbitai Bot

Loading