external_storage: implement locking #56597

YuJuncen · 2024-10-12T03:14:27Z

What problem does this PR solve?

Issue Number: close #56523

Problem Summary:

See the issue. We need a safer method to lock the external storage.

What changed and how does it work?

This PR implemented the "put and verify" txn, also a document was provided.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Release note

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

None

Signed-off-by: hillium <yujuncen@pingcap.com>

tiprow · 2024-10-12T03:14:42Z

Hi @YuJuncen. 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.

codecov · 2024-10-12T03:28:50Z

Codecov Report

Attention: Patch coverage is 75.17241% with 36 lines in your changes missing coverage. Please review.

Project coverage is 60.0809%. Comparing base (dfd6cf2) to head (94ec644).
Report is 229 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56597         +/-   ##
=================================================
- Coverage   73.3217%   60.0809%   -13.2409%     
=================================================
  Files          1631       1878        +247     
  Lines        451083     713403     +262320     
=================================================
+ Hits         330742     428619      +97877     
- Misses       100025     259249     +159224     
- Partials      20316      25535       +5219

Flag	Coverage Δ
integration	`41.3402% <33.7931%> (?)`
unit	`74.5158% <73.7931%> (+2.0707%)`	⬆️

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

Components	Coverage Δ
dumpling	`55.2602% <ø> (+2.3124%)`	⬆️
parser	`∅ <ø> (∅)`
br	`63.2261% <75.1724%> (+17.1935%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

lance6716 · 2024-10-12T04:39:42Z

docs/design/2024-10-11-put-and-verify-transactions-for-external-storages.md

+| Verify() → **OK**                        |                                            |
+|                                          | Write("LOCK_Bob", "") → **OK**             |
+| Write("LOCK_Alice", "") → **OK**         |                                            |
+|                                          | Verify() → **Failed! "LOCK_Alice" exists** |


When contention is high no one can commit? It doesn't scale.

Perhaps a randomized back off algorithm can help. Anyway this is like some sort of optimistic lock, hence not pretty effective in the scenario that tons of clients racing for one lock...

OK if there will not be too many clients in the use case

…rage-locking

Tristan1900

Just a bunch of nits, thanks for the change!

Tristan1900 · 2024-10-21T15:16:43Z

docs/design/2024-10-11-put-and-verify-transactions-for-external-storages.md

+
+- When compacting / restoring, we want to block migrating to a new version. 
+- When migrating the backup storage to a new version, we want to forbid reading.
+- When truncating the storage, we don't want another trancating operation happen.


typo trancating

Tristan1900 · 2024-10-21T15:32:39Z

docs/design/2024-10-11-put-and-verify-transactions-for-external-storages.md

+- When truncating the storage, we don't want another trancating operation happen.
+- When backing up, we don't want another backup uses the same storage.
+
+But external storage locking isn't trivial. Simply putting a lock file isn't safe enough: because after checking there isn't such a lock file, another one may write it immediately. Object locks provide stronger consistency, but also require extra configuration and permissions.


hmm, I thought the new feature conditional put from S3 can solve this race condition, other vendors also have it. I remember you mentioned this feature can provide RW support, maybe that's where it differs from simply using conditional put on a lock file? I feel like it could be better to mention the comparison between the two.

Tristan1900 · 2024-10-21T15:40:28Z

br/pkg/storage/locking.go

+// over the same file was success.
+//
+// For more details, check docs/design/2024-10-11-put-and-verify-transactions-for-external-storages.md.
+type ExclusiveWrite struct {


I feel like CondtionalWrite or ConditionalPut is more descriptive. And VerifyWriteContext could be ConditionCheck or Precondtion, something like that. What do you think?

Tristan1900 · 2024-10-21T15:41:04Z

br/pkg/storage/locking.go

 	"go.uber.org/zap"
 )

+// ExclusiveWrite is a write that in a strong consistency storage.
+//
+// It is pretty like a "write if not exist", but it is atomic:


not sure this description is accurate cuz the VerifyWriteContext can be arbitrary

Tristan1900 · 2024-10-21T15:44:54Z

br/pkg/storage/locking.go

+	lock.path = target
+	lock.txnID, err = writer.CommitTo(ctx, storage)
+	if err != nil {
+		err = errors.Annotatef(err, "there is something about the lock: %s", tryFetchRemoteLock(ctx, storage, writeLock))


nit, maybe failed to commit the lock? I feel like error message should describe what the action was for better debugging.

Tristan1900 · 2024-10-21T15:46:57Z

br/pkg/storage/locking.go

+func TryLockRemoteRead(ctx context.Context, storage ExternalStorage, path, hint string) (lock RemoteLock, err error) {
+	readID := rand.Int63()
+	target := fmt.Sprintf("%s.READ.%016x", path, readID)
+	writeLock := fmt.Sprintf("%s.WRIT", target)


Is this a typo WRIT? maybe put the READ, WRITE, INTENT into a constant cuz multiple methods are using it, changing one place and forgot to change other will cause problem

The suffix WRIT is just for aligning it with READ.

Tristan1900 · 2024-10-21T15:47:50Z

br/pkg/storage/locking.go

+			res, err := json.Marshal(meta)
+			if err != nil {
+				log.Panic(
+					"Unreachable: a trivial object cannot be marshaled to JSON.",


nit, what does trivial at here mean?

Some how like Trivial types in C++. Anyway it can be more clear.

ti-chi-bot · 2024-10-21T15:49:11Z

@Tristan1900: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Just a bunch of nits, thanks for the change!

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/test-infra repository.

Signed-off-by: hillium <yujuncen@pingcap.com>

3pointer

rest LGTM

3pointer · 2024-11-11T03:06:20Z

br/pkg/storage/locking.go

-			return err
-		}
-		return ErrLocked{Meta: meta}
+func TryLockRemote(ctx context.Context, storage ExternalStorage, path, hint string) (lock RemoteLock, err error) {


why not use TryLockRemoteWrite instead of TryLockRemote?

Just for compatibility. Write locks requires an extra suffix.

ti-chi-bot · 2024-11-12T11:32:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, Leavrth, Tristan1900

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:

~~OWNERS~~ [3pointer,Leavrth]
~~br/OWNERS~~ [3pointer,Leavrth]
~~br/pkg/storage/OWNERS~~ [3pointer,Leavrth]

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 · 2024-11-12T11:32:25Z

[LGTM Timeline notifier]

Timeline:

2024-11-11 03:07:45.742005531 +0000 UTC m=+239227.932874524: ☑️ agreed by 3pointer.
2024-11-12 11:32:24.466341105 +0000 UTC m=+355906.657210087: ☑️ agreed by Leavrth.

ti-chi-bot · 2024-11-12T15:10:40Z

/test all

purelind · 2024-11-12T15:11:28Z

/test all

tiprow · 2024-11-12T15:11:52Z

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

In response to this:

/test all

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.

purelind · 2024-11-13T02:10:23Z

/retest

tiprow · 2024-11-13T02:10:44Z

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

In response to this:

/retest

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.

YuJuncen · 2024-11-19T09:51:09Z

/retest

tiprow · 2024-11-19T09:51:31Z

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

In response to this:

/retest

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.

YuJuncen · 2024-11-19T09:58:12Z

/ok-to-test

YuJuncen · 2024-11-19T10:12:36Z

/retest-required

YuJuncen · 2024-11-20T02:59:42Z

/test unit-test
/test tidb_parser_test
/test fast_test_tiprow

ti-chi-bot · 2024-11-20T02:59:44Z

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

/test build

/test check-dev

/test check-dev2

/test mysql-test

/test pull-br-integration-test

/test pull-integration-ddl-test

/test pull-integration-e2e-test

/test pull-lightning-integration-test

/test pull-mysql-client-test

/test pull-unit-test-ddlv1

/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test

/test pull-common-test

/test pull-e2e-test

/test pull-integration-common-test

/test pull-integration-copr-test

/test pull-integration-jdbc-test

/test pull-integration-mysql-test

/test pull-integration-nodejs-test

/test pull-integration-python-orm-test

/test pull-sqllogic-test

/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build

pingcap/tidb/ghpr_check

pingcap/tidb/ghpr_check2

pingcap/tidb/ghpr_mysql_test

pingcap/tidb/ghpr_unit_test

pingcap/tidb/pull_br_integration_test

pingcap/tidb/pull_integration_ddl_test

pingcap/tidb/pull_integration_e2e_test

pingcap/tidb/pull_lightning_integration_test

pingcap/tidb/pull_mysql_client_test

In response to this:

/test unit-test
/test tidb_parser_test
/test fast_test_tiprow

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.

tiprow · 2024-11-20T03:00:02Z

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

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test unit-test
/test tidb_parser_test
/test fast_test_tiprow

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.

tiprow · 2024-11-20T03:00:05Z

@ti-chi-bot[bot]: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

@YuJuncen: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:
/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-integration-e2e-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test pull-unit-test-ddlv1
/test unit-test
The following commands are available to trigger optional jobs:
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-integration-python-orm-test
/test pull-sqllogic-test
/test pull-tiflash-test
Use /test all to run the following jobs that were automatically triggered:
pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_integration_e2e_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test
In response to this:

/test unit-test
/test tidb_parser_test
/test fast_test_tiprow

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.

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.

YuJuncen · 2024-11-20T03:06:21Z

/retest-required

tiprow · 2024-11-20T03:08:08Z

@YuJuncen: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tidb_parser_test	`94ec644`	link	true	`/test tidb_parser_test`
fast_test_tiprow	`94ec644`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

YuJuncen · 2024-11-20T06:45:27Z

/test mysql-test

tiprow · 2024-11-20T06:45:48Z

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

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test mysql-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.

close pingcap#56523

YuJuncen added 4 commits August 8, 2024 14:11

initial implementation

e3a0379

Signed-off-by: hillium <yujuncen@pingcap.com>

added more test cases

8e4c167

Signed-off-by: hillium <yujuncen@pingcap.com>

added a design doc

9a8da94

Signed-off-by: hillium <yujuncen@pingcap.com>

fix building and doc format

dda8f11

Signed-off-by: hillium <yujuncen@pingcap.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 12, 2024

lance6716 reviewed Oct 12, 2024

View reviewed changes

YuJuncen mentioned this pull request Oct 12, 2024

added WriteAndVerify committing pingcap/tla-plus#44

Open

Merge branch 'master' of https://github.com/pingcap/tidb into ext-sto…

99a2f49

…rage-locking

Tristan1900 approved these changes Oct 21, 2024

View reviewed changes

YuJuncen added 2 commits October 28, 2024 11:41

addressed comments

160aeb8

Signed-off-by: hillium <yujuncen@pingcap.com>

make conditional put private

94ec644

Signed-off-by: hillium <yujuncen@pingcap.com>

3pointer approved these changes Nov 11, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 11, 2024

Leavrth approved these changes Nov 12, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 12, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 19, 2024

ti-chi-bot bot merged commit ea7ec59 into pingcap:master Nov 20, 2024
36 of 38 checks passed

lance6716 mentioned this pull request Nov 27, 2024

br: fix flaky test TestConcurrentLock #57591

Merged

12 tasks

YuJuncen added a commit to YuJuncen/tidb that referenced this pull request Jan 6, 2025

external_storage: implement locking (pingcap#56597)

230c21a

close pingcap#56523

external_storage: implement locking #56597

external_storage: implement locking #56597

Conversation

YuJuncen commented Oct 12, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 12, 2024

codecov bot commented Oct 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tristan1900 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

YuJuncen Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 21, 2024

3pointer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 12, 2024

ti-chi-bot bot commented Nov 12, 2024

[LGTM Timeline notifier]

ti-chi-bot commented Nov 12, 2024

purelind commented Nov 12, 2024

tiprow bot commented Nov 12, 2024

purelind commented Nov 13, 2024

tiprow bot commented Nov 13, 2024

YuJuncen commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

YuJuncen commented Nov 19, 2024

YuJuncen commented Nov 19, 2024

YuJuncen commented Nov 20, 2024

ti-chi-bot bot commented Nov 20, 2024

tiprow bot commented Nov 20, 2024

tiprow bot commented Nov 20, 2024

YuJuncen commented Nov 20, 2024

tiprow bot commented Nov 20, 2024

YuJuncen commented Nov 20, 2024

tiprow bot commented Nov 20, 2024

codecov bot commented Oct 12, 2024 •

edited

Loading

YuJuncen Oct 28, 2024 •

edited

Loading