Skip to content

Conversation

@takaidohigasi
Copy link
Contributor

@takaidohigasi takaidohigasi commented Jul 3, 2025

What problem does this PR solve?

Issue Number: ref #46609

Problem Summary:

  • if don't support profile config, there are some problems
    • secret handling: Operation using SecretAccessKey carries risks of key leakage
      • we can use SSO and so on for authentication by profile
    • bandwidth limiting
      • we can limit bandwidth of file copy

What changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
taka-h@LCW9RLFMFX ~/git/tidb % ./bin/dumpling -h tidb.xfidkq1s0cfw.clusters.tidb-cloud.com -uroot -p$PASSWORD -P 3306 --filetype=csv -o s3://mercari-new-sql-poc/initial_data/20250704 -B test --s3.profile=mercari
Release version: v9.0.0-beta.2.pre-50-g1dc70c8c57-dev
Git commit hash: 1dc70c8c575455175dc8bca7ca41122754db882c
Git branch:      46609-dumpling-profiling-support-for-s3
Build timestamp: 2025-07-04 09:08:18Z
Go version:      go version go1.24.3 darwin/arm64

[2025/07/04 18:08:34.290 +09:00] [INFO] [versions.go:54] ["Welcome to dumpling"] ["Release Version"=v9.0.0-beta.2.pre-50-g1dc70c8c57-dev] ["Git Commit Hash"=1dc70c8c575455175dc8bca7ca41122754db882c] ["Git Branch"=46609-dumpling-profiling-support-for-s3] ["Build timestamp"="2025-07-04 09:08:18"] ["Go Version"="go version go1.24.3 darwin/arm64"]
[2025/07/04 18:08:35.913 +09:00] [INFO] [s3.go:559] ["succeed to get bucket region from s3"] ["bucket region"=ap-northeast-1]
[2025/07/04 18:08:36.023 +09:00] [INFO] [version.go:441] ["detect server version"] [type=TiDB] [version=8.1.2]
[2025/07/04 18:08:36.247 +09:00] [WARN] [dump.go:1518] ["If the amount of data to dump is large, criteria: (data more than 60GB or dumped time more than 10 minutes)\nyou'd better adjust the tikv_gc_life_time to avoid export failure due to TiDB GC during the dump process.\nBefore dumping: run sql `update mysql.tidb set VARIABLE_VALUE = '720h' where VARIABLE_NAME = 'tikv_gc_life_time';` in tidb.\nAfter dumping: run sql `update mysql.tidb set VARIABLE_VALUE = '10m' where VARIABLE_NAME = 'tikv_gc_life_time';` in tidb.\n"]
[2025/07/04 18:08:36.348 +09:00] [INFO] [dump.go:1586] ["cannot check whether TiDB has TiKV, will apply tidb_snapshot by default. This won't affect dump process"] [error="sql: SELECT COUNT(1) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point': Error 1142 (42000): SELECT command denied to user 'root'@'%' for table 'tidb'"]
[2025/07/04 18:08:36.474 +09:00] [INFO] [dump.go:153] ["begin to run Dump"] [conf="{\"s3\":{\"endpoint\":\"\",\"region\":\"\",\"storage-class\":\"\",\"sse\":\"\",\"sse-kms-key-id\":\"\",\"acl\":\"\",\"access-key\":\"\",\"secret-access-key\":\"\",\"session-token\":\"\",\"provider\":\"\",\"force-path-style\":true,\"use-accelerate-endpoint\":false,\"role-arn\":\"\",\"external-id\":\"\",\"profile\":\"mercari\",\"object-lock-enabled\":false},\"gcs\":{\"endpoint\":\"\",\"storage-class\":\"\",\"predefined-acl\":\"\",\"credentials-file\":\"\"},\"azblob\":{\"endpoint\":\"\",\"account-name\":\"\",\"account-key\":\"\",\"access-tier\":\"\",\"sas-token\":\"\",\"encryption-scope\":\"\",\"encryption-key\":\"\"},\"SpecifiedTables\":false,\"AllowCleartextPasswords\":false,\"SortByPk\":true,\"NoViews\":true,\"NoSequences\":true,\"NoHeader\":false,\"NoSchemas\":false,\"NoData\":false,\"CompleteInsert\":false,\"TransactionalConsistency\":true,\"EscapeBackslash\":true,\"DumpEmptyDatabase\":true,\"PosAfterConnect\":false,\"CompressType\":0,\"Host\":\"tidb.xfidkq1s0cfw.clusters.tidb-cloud.com\",\"Port\":3306,\"Threads\":4,\"User\":\"root\",\"Security\":{\"CAPath\":\"\",\"CertPath\":\"\",\"KeyPath\":\"\"},\"LogLevel\":\"info\",\"LogFile\":\"\",\"LogFormat\":\"text\",\"OutputDirPath\":\"s3://mercari-new-sql-poc/initial_data/20250704\",\"StatusAddr\":\":8281\",\"Snapshot\":\"459176703743229957\",\"Consistency\":\"snapshot\",\"CsvNullValue\":\"\\\\N\",\"SQL\":\"\",\"CsvSeparator\":\",\",\"CsvDelimiter\":\"\\\"\",\"CsvLineTerminator\":\"\\r\\n\",\"Databases\":[\"test\"],\"Where\":\"\",\"FileType\":\"csv\",\"ServerInfo\":{\"ServerType\":3,\"ServerVersion\":\"8.1.2\",\"HasTiKV\":true},\"Rows\":0,\"ReadTimeout\":900000000000,\"TiDBMemQuotaQuery\":0,\"FileSize\":0,\"StatementSize\":1000000,\"SessionParams\":{\"tidb_snapshot\":\"459176703743229957\"},\"Tables\":{},\"CollationCompatible\":\"loose\",\"CsvOutputDialect\":0,\"IOTotalBytes\":null,\"Net\":\"\"}"]
[2025/07/04 18:08:37.775 +09:00] [INFO] [collector.go:264] ["backup success summary"] [total-ranges=7] [ranges-succeed=7] [ranges-failed=0] [total-take=719.542125ms] [total-kv-size=1.344kB] [average-speed=1.868kB/s] [total-rows=34]
[2025/07/04 18:08:38.014 +09:00] [INFO] [main.go:82] ["dump data successfully, dumpling will exit now"]


taka-h@LCW9RLFMFX ~/git/tidb % aws s3 ls s3://mercari-new-sql-poc/initial_data/20250704/ --profile=mercari
2025-07-04 18:08:38        146 metadata
2025-07-04 18:08:38        132 test-schema-create.sql
2025-07-04 18:08:38        470 test.OrderReceipts-schema.sql
2025-07-04 18:08:38        633 test.OrderReceipts.000000000.csv
2025-07-04 18:08:38        352 test.Orders-schema.sql
2025-07-04 18:08:38        407 test.Orders.000000000.csv
2025-07-04 18:08:38        213 test.Roles-schema.sql
2025-07-04 18:08:38        304 test.Roles.000000000.csv
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

it requries some document update
https://docs.pingcap.com/tidb/stable/dumpling-overview/#export-data-to-amazon-s3-cloud-storage

around

Pass SecretKey and AccessKey of the account with the permission to access the Amazon S3 backend storage to the Dumpling node as environment variables.

export AWS_ACCESS_KEY_ID=${AccessKey}
export AWS_SECRET_ACCESS_KEY=${SecretKey}

=>
pingcap/docs#21456

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

br: support s3 profile config, allow you to use SSO, bandwidth limiting, and etc.

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/dumpling This is related to Dumpling of TiDB. labels Jul 3, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 3, 2025

Hi @takaidohigasi. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ti-chi-bot ti-chi-bot bot added needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Jul 3, 2025
@tiprow
Copy link

tiprow bot commented Jul 3, 2025

Hi @takaidohigasi. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@takaidohigasi takaidohigasi changed the title br: support s3 configuration [pingcap/tidb#46609] br: support s3 profile config [pingcap/tidb#46609] Jul 3, 2025
@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-linked-issue labels Jul 3, 2025
@takaidohigasi takaidohigasi changed the title br: support s3 profile config [pingcap/tidb#46609] br: support s3 profile config Jul 3, 2025
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 3, 2025
@takaidohigasi
Copy link
Contributor Author

/test

@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 3, 2025

@takaidohigasi: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

1 similar comment
@tiprow
Copy link

tiprow bot commented Jul 3, 2025

@takaidohigasi: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@takaidohigasi takaidohigasi marked this pull request as ready for review July 4, 2025 09:12
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2025
@lance6716
Copy link
Contributor

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jul 4, 2025
@codecov
Copy link

codecov bot commented Jul 4, 2025

Codecov Report

Attention: Patch coverage is 60.71429% with 11 lines in your changes missing coverage. Please review.

Project coverage is 74.8508%. Comparing base (c2c36cd) to head (5540811).
Report is 1 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #62199        +/-   ##
================================================
+ Coverage   72.8591%   74.8508%   +1.9917%     
================================================
  Files          1768       1814        +46     
  Lines        485994     494152      +8158     
================================================
+ Hits         354091     369877     +15786     
+ Misses       110224     101160      -9064     
- Partials      21679      23115      +1436     
Flag Coverage Δ
integration 48.7973% <39.2857%> (?)
unit 72.1883% <60.7142%> (+0.0507%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.8700% <ø> (ø)
parser ∅ <ø> (∅)
br 63.1253% <60.7142%> (+16.7918%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@takaidohigasi takaidohigasi force-pushed the 46609-dumpling-profiling-support-for-s3 branch from 5c4ae91 to ad3c4bd Compare July 4, 2025 10:37
@takaidohigasi
Copy link
Contributor Author

/test build

@tiprow
Copy link

tiprow bot commented Jul 4, 2025

@takaidohigasi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test fast_test_tiprow
  • /test tidb_parser_test

Use /test all to run all jobs.

In response to this:

/test build

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@takaidohigasi
Copy link
Contributor Author

/retest

@lance6716
Copy link
Contributor

Dumpling / integration-test (5.7.35) (pull_request)Failing after 7m

/tmp/dumpling_test_result/sql_res.s3/s3.minio/mybucket/dump/s3.t.000000000.sql/100e05a3-2515-4da7-b66a-52312cb39dc6:
part.1
part.10
part.11
part.12
part.13
part.14
part.15
part.16
part.2
part.3
part.4
part.5
part.6
part.7
part.8
part.9
+(dumpling/tests/s3/run.sh:67): bin/mc config host add minio http://127.0.0.1:5000/ testid testkey8

mc: <ERROR> `config` is not a recognized command. Get help using `--help` flag. 
+(dumpling/tests/s3/run.sh:1): cleanup
+(dumpling/tests/s3/run.sh:31): cleanup(): echo 'Stopping motoserver'
Stopping motoserver
+(dumpling/tests/s3/run.sh:32): cleanup(): kill -2 22715
INFO: Exiting on signal: INTERRUPT
dumpling/tests/_utils/run_services: line 7: 13102 Killed                  bin/tidb-server --config "$DUMPLING_TEST_DIR/tidb.toml"
make: *** [Makefile:588: dumpling_integration_test] Error 1
Error: The operation was canceled.

I tried several times but integration test resulted in error.

+(dumpling/tests/s3/run.sh:67): bin/mc config host add minio http://127.0.0.1:5000/ testid testkey8

mc: <ERROR> `config` is not a recognized command. Get help using `--help` flag. 

I think that's a known problem #61575 . Don't worry, reviewers will remind you if some critical CI failed. I'll take a look next week.

@takaidohigasi
Copy link
Contributor Author

thanks let me know!

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 7, 2025
@lance6716
Copy link
Contributor

/cc @Leavrth

@ti-chi-bot ti-chi-bot bot requested a review from Leavrth July 7, 2025 06:12
@takaidohigasi
Copy link
Contributor Author

🙏

@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, Leavrth

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 23, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 23, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-07-07 06:12:13.541467034 +0000 UTC m=+1893786.264646016: ☑️ agreed by lance6716.
  • 2025-07-23 09:10:10.195696669 +0000 UTC m=+155766.933122880: ☑️ agreed by Leavrth.

@takaidohigasi
Copy link
Contributor Author

thanks, I will create the PR for docs too.

@hawkingrei
Copy link
Member

/retest

1 similar comment
@lance6716
Copy link
Contributor

/retest

@takaidohigasi
Copy link
Contributor Author

https://github.com/pingcap/tidb/actions/runs/16466462588/job/46707983113?pr=62199

gcc_freebsd_amd64.c:7:10: fatal error: 'sys/signalvar.h' file not found
make: *** [Makefile:419: build_br] Error 1

@takaidohigasi
Copy link
Contributor Author

/retest

@takaidohigasi
Copy link
Contributor Author

bin/mc config host add minio http://127.0.0.1:5000/ testid testkey8
=>
bin/mc alias set minio http://127.0.0.1:5000/ testid testkey8
?

I will check the master branch

@takaidohigasi
Copy link
Contributor Author

already fixed on master.
https://github.com/pingcap/tidb/blob/master/dumpling/tests/s3/run.sh#L67

so I pull(merge) upstream/master

@lance6716
Copy link
Contributor

/retest

@takaidohigasi
Copy link
Contributor Author

I believe this will resolve integration test failure

@takaidohigasi
Copy link
Contributor Author

all the integration-tests were passed, and waiting other tests will succeed.

@ti-chi-bot ti-chi-bot bot merged commit f0fb312 into pingcap:master Jul 25, 2025
34 checks passed
@takaidohigasi
Copy link
Contributor Author

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved component/dumpling This is related to Dumpling of TiDB. lgtm ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants