keyspace: add preconditions for keyspace config update #10199
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
📝 WalkthroughWalkthroughAdds CAS-style preconditions to keyspace config updates: new error, new Manager API UpdateKeyspaceConfigWithPreconditions with transactional precondition checks and mutations, API/CLI plumbing to pass preconditions, and tests (including concurrent scenarios). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as API Handler
participant Manager as Keyspace Manager
participant Etcd as Etcd Store
Client->>Handler: PATCH /keyspaces/{name}/config (Config + Preconditions)
Handler->>Manager: UpdateKeyspaceConfigWithPreconditions(name, mutations, preconditions)
Manager->>Manager: checkKeyspaceConfigPreconditions(currentConfig, preconditions)
alt Preconditions match
Manager->>Manager: build txn with mutations
Manager->>Etcd: Commit transaction
Etcd-->>Manager: Success
Manager-->>Handler: Updated KeyspaceMeta
Handler-->>Client: 200 OK + new config
else Preconditions fail
Manager-->>Handler: ErrKeyspaceConfigPreconditionFailed
Handler-->>Client: 409 Conflict
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10199 +/- ##
==========================================
+ Coverage 78.59% 78.65% +0.05%
==========================================
Files 520 520
Lines 70014 70096 +82
==========================================
+ Hits 55028 55132 +104
+ Misses 11008 10993 -15
+ Partials 3978 3971 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Changes:
- Extend /pd/api/v2/keyspaces/{name}/config PATCH with "preconditions" to support CAS-like config updates (equal / absent).
- Add pd-ctl support: keyspace update-config --expect and --expect-absent.
Rationale:
We need PD to provide a simple CAS-style guard for keyspace config updates, so concurrent key-rotation workflows can coordinate safely and only the expected state transition is allowed.
Signed-off-by: Bisheng Huang <hbisheng@gmail.com>
hbisheng
force-pushed
the
keyspace-cas
branch
from
6897830 to
a4f2a67
Compare
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/apiv2/handlers/keyspace.go (1)
322-368:⚠️ Potential issue | 🟡 MinorReturn conflicts via
errorRespand document HTTP 409.
The new conflict paths should follow the handler convention (errcode+errorResp) and the Swagger annotations should include the 409 response so clients can rely on it.✏️ Swagger annotation update
// `@Failure` 400 {string} string "The input is invalid." +// `@Failure` 409 {string} string "Precondition failed or transaction conflicted." // `@Failure` 500 {string} string "PD server failed to proceed the request."Based on learnings: HTTP handlers in Go must use errcode and
errorResp; avoidhttp.Error.
As per coding guidelines: Swagger specification: runmake swagger-spec(with SWAGGER=1) to regenerate; keep annotations current.
|
/retest |
|
@ystaticy PTAL |
pkg/keyspace/keyspace.go
Outdated
| if err := checkKeyspaceConfigPreconditions(meta.GetConfig(), preconditions); err != nil { | ||
| return err | ||
| } | ||
| for _, mutation := range mutations { |
There was a problem hiding this comment.
Added an applyKeyspaceConfigMutations function
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Bisheng Huang <hbisheng@gmail.com>
hbisheng
force-pushed
the
keyspace-cas
branch
from
9f387f7 to
496b049
Compare
|
@hbisheng: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
What problem does this PR solve?
Issue Number: Close #10214
What is changed and how does it work?
In the CMEK / encryption key rotation workflow, PD keyspace config is planned to be used to store the current and target encryption config version. Although the workflow is designed to have a single active actor, unexpected concurrent executions (for example, retries or overlapping operations) can lead to config updates overwriting each other and breaking the intended state transition.
This PR adds CAS-style guards to keyspace config updates so that encryption key rotation can advance safely and only when the system is in the expected state. With this, callers can, for example, set
next_encryption_file_idonly if it is absent, or advancecurrent_encryption_file_idonly if it matchesnext_encryption_file_id.Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests