[Pending] Add AWS v2 package for the agent sidecar #1769
Conversation
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
hlts2
changed the title
Signed-off-by: hlts2 <[email protected]>
Deploying vald with
|
| Latest commit: |
e01ec5b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a91a6091.vald.pages.dev |
| Branch Preview URL: | https://feature-internal-db-s3-aws-s.vald.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1769 +/- ##
===========================================
+ Coverage 18.91% 31.82% +12.91%
===========================================
Files 565 374 -191
Lines 65223 32027 -33196
===========================================
- Hits 12337 10194 -2143
+ Misses 52089 21443 -30646
+ Partials 797 390 -407 ☔ View full report in Codecov by Sentry. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| func (c *client) download(ctx context.Context, key string, w io.WriterAt) (err error) { | ||
| input := &s3.GetObjectInput{ |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
ChecksumMode, ExpectedBucketOwner, IfMatch, IfModifiedSince, IfNoneMatch, IfUnmodifiedSince, PartNumber, Range, RequestPayer, ResponseCacheControl, ResponseContentDisposition, ResponseContentEncoding, ResponseContentLanguage, ResponseContentType, ResponseExpires, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5, VersionId are missing in GetObjectInput (exhaustruct)
| } | ||
|
|
||
| func (c *client) upload(ctx context.Context, key string, body io.Reader) (err error) { | ||
| input := &s3.PutObjectInput{ |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
ACL, BucketKeyEnabled, CacheControl, ChecksumAlgorithm, ChecksumCRC32, ChecksumCRC32C, ChecksumSHA1, ChecksumSHA256, ContentDisposition, ContentEncoding, ContentLanguage, ContentLength, ContentMD5, ExpectedBucketOwner, Expires, GrantFullControl, GrantRead, GrantReadACP, GrantWriteACP, Metadata, ObjectLockLegalHoldStatus, ObjectLockMode, ObjectLockRetainUntilDate, RequestPayer, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5, SSEKMSEncryptionContext, SSEKMSKeyId, ServerSideEncryption, StorageClass, Tagging, WebsiteRedirectLocation are missing in PutObjectInput (exhaustruct)
| io.WriteCloser | ||
| } | ||
|
|
||
| type client struct { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
fieldalignment: struct with 104 pointer bytes could be 80 (govet)
| "github.com/vdaas/vald/internal/log" | ||
| ) | ||
|
|
||
| type client struct { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
fieldalignment: struct with 208 pointer bytes could be 168 (govet)
|
|
||
| type logger struct{} | ||
|
|
||
| func New() logging.Logger { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
New returns interface (github.com/aws/smithy-go/logging.Logger) (ireturn)
| } | ||
|
|
||
| func New(opts ...Option) (Client, error) { | ||
| c := new(client) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
variable name 'c' is too short for the scope of its usage (varnamelen)
|
|
||
| func (c *client) Open(ctx context.Context, key string) (err error) { | ||
| c.wg = new(sync.WaitGroup) | ||
| f, err := file.CreateTemp() |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
variable name 'f' is too short for the scope of its usage (varnamelen)
|
|
||
| // New returns blob.Bucket implementation if no error occurs. | ||
| func New(opts ...Option) (b blob.Bucket, err error) { | ||
| c := new(client) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
variable name 'c' is too short for the scope of its usage (varnamelen)
| return err | ||
| } | ||
|
|
||
| c.s3client = s3.NewFromConfig(cfg, func(o *s3.Options) { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
parameter name 'o' is too short for the scope of its usage (varnamelen)
| // The minimum allowed part size is 5MB, and if this value is set to zero, | ||
| // the DefaultUploadPartSize(DefaultDownloadPartSize) value will be used. | ||
| func WithMaxPartSize(size string) Option { | ||
| return func(c *client) error { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
parameter name 'c' is too short for the scope of its usage (varnamelen)
kpango
force-pushed
the
main
branch
2 times, most recently
from
9b3087a to
4a659f2
Compare
WalkthroughWalkthroughThe recent updates refine S3 storage interactions across the Vald Helm chart and internal handling of S3 operations. Key changes include the removal of deprecated S3 features, the introduction of concurrency controls for improved performance, and enhancements in error handling and logging. These modifications aim to streamline operations, enhance performance, and ensure robust data management with AWS S3. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- charts/vald-helm-operator/crds/valdrelease.yaml (1 hunks)
- charts/vald/values.yaml (1 hunks)
- internal/config/blob_test.go (6 hunks)
- internal/db/storage/blob/v3/s3/downloader/downloader.go (1 hunks)
- internal/db/storage/blob/v3/s3/downloader/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/file/file.go (1 hunks)
- internal/db/storage/blob/v3/s3/logger/logger.go (1 hunks)
- internal/db/storage/blob/v3/s3/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/s3.go (1 hunks)
- internal/db/storage/blob/v3/s3/uploader/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/uploader/uploader.go (1 hunks)
Files skipped from review due to trivial changes (1)
- internal/db/storage/blob/v3/s3/option.go
Additional comments not posted (29)
internal/db/storage/blob/v3/s3/logger/logger.go (2)
12-14: LGTM! TheNewfunction correctly implements a factory pattern for creatingloggerinstances.
16-25: LGTM! TheLogffunction correctly handles different logging classifications.internal/db/storage/blob/v3/s3/file/file.go (2)
14-24: LGTM! TheClosefunction correctly handles the file lifecycle by closing and removing the file, with appropriate error handling.
27-36: LGTM! TheCreateTempfunction correctly creates a temporary file and handles potential errors.internal/db/storage/blob/v3/s3/downloader/option.go (4)
17-26: LGTM! TheWithBucketoption function correctly sets and validates the bucket name.
28-37: LGTM! TheWithAPIClientoption function correctly sets the API client with appropriate nil checks.
39-48: LGTM! TheWithConcurrencyoption function correctly sets the concurrency level with a minimum requirement check.
50-59: LGTM! TheWithMaxPartSizeoption function correctly sets the maximum part size with a minimum size requirement.internal/db/storage/blob/v3/s3/uploader/option.go (6)
20-29: LGTM! TheWithErrGroupoption function correctly sets the error group with appropriate nil checks.
31-40: LGTM! TheWithBucketoption function correctly sets and validates the bucket name.
42-51: LGTM! TheWithAPIClientoption function correctly sets the API client with appropriate nil checks.
53-62: LGTM! TheWithContentTypeoption function correctly sets and validates the content type.
64-73: LGTM! TheWithConcurrencyoption function correctly sets the concurrency level with a minimum requirement check.
75-84: LGTM! TheWithMaxPartSizeoption function correctly sets the maximum part size with a minimum size requirement.internal/db/storage/blob/v3/s3/downloader/downloader.go (3)
36-44: LGTM! TheNewfunction correctly initializes a new downloader client with error handling.
46-74: LGTM! TheDownloadfunction correctly performs the download operation with comprehensive error handling and resource management.
77-91: LGTM! Thedownloadfunction correctly uses the AWS SDK to perform the download and handles errors appropriately.internal/db/storage/blob/v3/s3/uploader/uploader.go (5)
40-48: LGTM! TheNewfunction correctly initializes a new uploader client with error handling.
50-83: LGTM! TheOpenfunction correctly prepares the uploader for writing with comprehensive resource management and error handling.
85-94: LGTM! TheClosefunction correctly manages resource cleanup for the uploader.
97-102: LGTM! TheWritefunction correctly writes data with appropriate checks and error handling.
104-123: LGTM! Theuploadfunction correctly uses the AWS SDK to perform the upload and handles errors appropriately.internal/db/storage/blob/v3/s3/s3.go (5)
54-70: LGTM! TheNewfunction correctly initializes a new S3 client with error handling and logger setup.
73-148: LGTM! TheOpenfunction correctly configures the S3 client with comprehensive settings and error handling.
150-153: LGTM! TheClosefunction correctly implements a placeholder for resource cleanup.
155-159: LGTM! TheReaderfunction correctly creates a reader for S3 downloads.
161-169: LGTM! TheWriterfunction correctly creates a writer for S3 uploads with appropriate initialization and error handling.charts/vald/values.yaml (1)
2356-2358: Ensure the minimum concurrency setting aligns with system requirements.Verification successful
The verification process aimed to determine if the minimum concurrency setting of 3 for the S3 operations in the Vald system's Helm chart configuration aligns with the system's performance requirements. The script executed searched for issues and pull requests related to "concurrency" in their title or body.
Analysis of the Script Output:
- Issues: No issues were returned from the search, indicating there are no reported problems directly mentioning "concurrency" in the context of system performance or configuration.
- Pull Requests: A single draft pull request was found, titled "[Pending] Add AWS v2 package for the agent sidecar". This PR does not directly address the concurrency setting but seems related to an upgrade or change in the AWS SDK used by the agent sidecar. The relevance of this PR to the concurrency setting is not clear without further details.
Conclusion:
Given the absence of reported issues directly related to the concurrency setting, and the lack of clear connection between the found PR and the specific concurrency configuration, there is no evidence from the GitHub repository to suggest that the minimum concurrency setting of 3 is insufficient or problematic.
Final Response:
The review comment's concern about ensuring the minimum concurrency setting aligns with system requirements appears to be addressed adequately in the current configuration, as there are no reported issues suggesting otherwise.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the minimum concurrency setting of 3 is sufficient for the system's performance requirements. # Test: Check if there are any related performance issues reported in the issues or pull requests. gh issue list --search "concurrency in:title,body" gh pr list --search "concurrency in:title,body"Length of output: 228
charts/vald-helm-operator/crds/valdrelease.yaml (1)
1053-1055: Ensure theconcurrencyproperty's minimum value aligns with the new AWS SDK v2 requirements.
|
@coderabbitai review |
Actions PerformedReview triggered.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- charts/vald-helm-operator/crds/valdrelease.yaml (1 hunks)
- charts/vald/values.yaml (1 hunks)
- internal/config/blob_test.go (6 hunks)
- internal/db/storage/blob/v3/s3/downloader/downloader.go (1 hunks)
- internal/db/storage/blob/v3/s3/downloader/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/file/file.go (1 hunks)
- internal/db/storage/blob/v3/s3/logger/logger.go (1 hunks)
- internal/db/storage/blob/v3/s3/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/s3.go (1 hunks)
- internal/db/storage/blob/v3/s3/uploader/option.go (1 hunks)
- internal/db/storage/blob/v3/s3/uploader/uploader.go (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- charts/vald-helm-operator/crds/valdrelease.yaml
- charts/vald/values.yaml
- internal/config/blob_test.go
- internal/db/storage/blob/v3/s3/downloader/downloader.go
- internal/db/storage/blob/v3/s3/downloader/option.go
- internal/db/storage/blob/v3/s3/file/file.go
- internal/db/storage/blob/v3/s3/logger/logger.go
- internal/db/storage/blob/v3/s3/option.go
- internal/db/storage/blob/v3/s3/s3.go
- internal/db/storage/blob/v3/s3/uploader/option.go
- internal/db/storage/blob/v3/s3/uploader/uploader.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
charts/vald-helm-operator/crds/valdrelease.yaml (3)
1112-1114: Verify the impact of removingmax_chunk_size.The property
max_chunk_sizeis still referenced in multiple files. Ensure that these references are updated or removed to avoid inconsistencies:
internal/config/blob.gocharts/vald/values.gocharts/vald/values.schema.jsoncharts/vald/README.mdAnalysis chain
Verify the impact of removing
max_chunk_size.Ensure that the removal of this property does not impact any dependent functionality or configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `max_chunk_size` in the codebase. # Test: Search for the usage of `max_chunk_size`. Expect: No occurrences. rg --type yaml 'max_chunk_size'Length of output: 123
Script:
#!/bin/bash # Description: Verify the usage of `max_chunk_size` in the entire codebase. # Test: Search for the usage of `max_chunk_size` in all files. Expect: No critical dependencies. rg 'max_chunk_size'Length of output: 3033
1112-1114: The removal ofenable_100_continueimpacts multiple parts of the codebase.The
enable_100_continueproperty is used in several files, including configuration and schema files. Removing it fromcharts/vald-helm-operator/crds/valdrelease.yamlcould affect the following files:
internal/config/blob.gocharts/vald/values.gocharts/vald/values.schema.jsoncharts/vald/README.mdPlease review the implications of this removal carefully to ensure no dependent functionality is disrupted.
Analysis chain
Verify the impact of removing
enable_100_continue.Ensure that the removal of this property does not impact any dependent functionality or configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `enable_100_continue` in the codebase. # Test: Search for the usage of `enable_100_continue`. Expect: No occurrences. rg --type yaml 'enable_100_continue'Length of output: 133
Script:
#!/bin/bash # Description: Verify the usage of `enable_100_continue` in the entire codebase. # Test: Search for the usage of `enable_100_continue` in all files. Expect: No occurrences. rg 'enable_100_continue'Length of output: 3088
Script:
#!/bin/bash # Extract relevant lines from files where `enable_100_continue` is mentioned to understand its context and usage. # Extract lines from `internal/config/blob.go`. rg --context 5 'enable_100_continue' internal/config/blob.go # Extract lines from `charts/vald/values.go`. rg --context 5 'enable_100_continue' charts/vald/values.go # Extract lines from `charts/vald/values.schema.json`. rg --context 5 'enable_100_continue' charts/vald/values.schema.json # Extract lines from `charts/vald/README.md`. rg --context 5 'enable_100_continue' charts/vald/README.mdLength of output: 31426
1112-1114: Ensure Consistency Across Codebase forenable_content_md5_validationThe
enable_content_md5_validationproperty is referenced in multiple files. If you intend to remove this property from the CRD YAML file, please ensure to update or remove all related references to maintain consistency and avoid potential issues:
internal/config/blob.gocharts/vald/values.schema.jsoncharts/vald/values.gocharts/vald/README.mdPlease verify and update these references accordingly.
Analysis chain
Verify the impact of removing
enable_content_md5_validation.Ensure that the removal of this property does not impact any dependent functionality or configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `enable_content_md5_validation` in the codebase. # Test: Search for the usage of `enable_content_md5_validation`. Expect: No occurrences. rg --type yaml 'enable_content_md5_validation'Length of output: 153
Script:
#!/bin/bash # Description: Extract context around the usage of `enable_content_md5_validation` in the specified file. # Test: Extract 10 lines before and after the occurrence of `enable_content_md5_validation`. rg --type yaml -A 10 -B 10 'enable_content_md5_validation' k8s/operator/helm/crds/valdrelease.yamlLength of output: 1203
Script:
#!/bin/bash # Description: Search for references to `enable_content_md5_validation` across the entire codebase. # Test: Search for the usage of `enable_content_md5_validation`. Expect: No occurrences outside of the YAML file. rg 'enable_content_md5_validation'Length of output: 3147
Script:
#!/bin/bash # Description: Extract context around the usage of `enable_content_md5_validation` in the identified files. # Extract context from internal/config/blob.go rg --type go -A 10 -B 10 'enable_content_md5_validation' internal/config/blob.go # Extract context from charts/vald/values.schema.json rg --type json -A 10 -B 10 'enable_content_md5_validation' charts/vald/values.schema.json # Extract context from charts/vald/values.go rg --type go -A 10 -B 10 'enable_content_md5_validation' charts/vald/values.go # Extract context from charts/vald/README.md rg --type md -A 10 -B 10 'enable_content_md5_validation' charts/vald/README.mdLength of output: 59473
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- charts/vald-helm-operator/crds/valdrelease.yaml (1 hunks)
- charts/vald/values.yaml (1 hunks)
Additional comments not posted (6)
charts/vald/values.yaml (2)
2433-2435: Verify the impact of removing S3 configuration parameters.The removal of
enable_param_validation,enable_100_continue,enable_content_md5_validation,enable_endpoint_host_prefix, andmax_chunk_sizesimplifies the configuration. Ensure that these features are not required in your use case.
2433-2435: LGTM! Verify the concurrency level.The addition of the
concurrencyparameter is a good improvement for performance. Ensure that the value of 3 is optimal for your use case.charts/vald-helm-operator/crds/valdrelease.yaml (4)
1112-1114: Verify the impact of removingenable_param_validation.Ensure that the removal of this property does not impact any dependent functionality or configurations.
1112-1114: LGTM! But verify the usage ofconcurrency.The addition of the
concurrencyproperty looks good. Ensure that it is correctly implemented and does not conflict with existing configurations.Verification successful
LGTM! The addition of the
concurrencyproperty is correctly implemented and does not conflict with existing configurations.
- The property
concurrencyis used consistently across various sections of thecharts/vald-helm-operator/crds/valdrelease.yamlfile.- Each instance of
concurrencyhas appropriate type and minimum constraints.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `concurrency` in the codebase. # Test: Search for the usage of `concurrency`. Expect: Correct implementation and no conflicts. rg --type yaml 'concurrency'Length of output: 15103
Script:
#!/bin/bash # Extract and review the relevant sections of `charts/vald-helm-operator/crds/valdrelease.yaml` where `concurrency` is mentioned. rg --type yaml -A 5 -B 5 'concurrency' charts/vald-helm-operator/crds/valdrelease.yamlLength of output: 13417
1112-1114: LGTM! But verify the usage ofmax_part_size.The
max_part_sizeproperty remains unchanged. Ensure that it is correctly implemented and does not conflict with existing configurations.Verification successful
The
max_part_sizeproperty is correctly implemented and consistent across the codebase. No conflicts found.
charts/vald/values.yaml: The property is defined with a type ofstringand a pattern of^[0-9]+(kb|mb|gb)$.charts/vald-helm-operator/crds/valdrelease.yaml: The property is similarly defined with a type ofstringand the same pattern.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `max_part_size` in the codebase. # Test: Search for the usage of `max_part_size`. Expect: Correct implementation and no conflicts. rg --type yaml 'max_part_size'Length of output: 556
Script:
#!/bin/bash # Extract lines around the `max_part_size` property in `charts/vald/values.yaml` rg --type yaml -A 5 -B 5 'max_part_size' charts/vald/values.yaml # Extract lines around the `max_part_size` property in `charts/vald-helm-operator/crds/valdrelease.yaml` rg --type yaml -A 5 -B 5 'max_part_size' charts/vald-helm-operator/crds/valdrelease.yamlLength of output: 1719
1112-1114: Verify the impact of removingenable_endpoint_host_prefix.Ensure that the removal of this property does not impact any dependent functionality or configurations.
Description:
I implemented the AWS v2 package for the agent sidecar.
As discussed previously, The logic for uploading and downloading data from s3 is implemented in a file-based way.
NOTE: This PR contains the following new dependencies.
Impact
The results of the backup speed comparison are as follows. (Average of 5 times)
Related Issue:
How Has This Been Tested?:
Environment:
Types of changes:
Changes to Core Features:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor