rm: add RM metadata handler with redirector scaffold#10246
rm: add RM metadata handler with redirector scaffold#10246ti-chi-bot[bot] merged 22 commits intotikv:masterfrom
Conversation
Signed-off-by: okjiang <819421878@qq.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
do-not-merge/needs-linked-issue
dco-signoff: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a PD metadata HTTP API handler in the redirector package for managing resource groups, controller configuration, and keyspace service limits. Includes public interfaces, adapter types, HTTP handlers with routing logic, and comprehensive test coverage with an in-memory test store. Changes
Sequence DiagramsequenceDiagram
participant Client
participant pdMetadataHandler
participant pdMetadataStore
participant Manager
Client->>pdMetadataHandler: HTTP POST /resource_groups
pdMetadataHandler->>pdMetadataHandler: ServeHTTP, parse request
pdMetadataHandler->>pdMetadataStore: AddResourceGroup(group)
pdMetadataStore->>Manager: Update internal state
Manager-->>pdMetadataStore: Success
pdMetadataStore-->>pdMetadataHandler: nil error
pdMetadataHandler-->>Client: HTTP 200 OK
Client->>pdMetadataHandler: HTTP GET /resource_groups/:id
pdMetadataHandler->>pdMetadataHandler: ServeHTTP, parse request
pdMetadataHandler->>pdMetadataStore: GetResourceGroup(id)
pdMetadataStore->>Manager: Lookup group
Manager-->>pdMetadataStore: ResourceGroup
pdMetadataStore-->>pdMetadataHandler: group, nil error
pdMetadataHandler-->>Client: HTTP 200 + JSON body
Client->>pdMetadataHandler: HTTP GET /service_limits/:keyspace_id
pdMetadataHandler->>pdMetadataHandler: ServeHTTP, parse request
pdMetadataHandler->>pdMetadataStore: lookupKeyspaceServiceLimit(id)
pdMetadataStore->>Manager: Lookup service limit
Manager-->>pdMetadataStore: limit, exists
pdMetadataStore-->>pdMetadataHandler: limit, true
pdMetadataHandler-->>Client: HTTP 200 + limit
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/mcs/resourcemanager/redirector/pd_metadata_handler.go`:
- Around line 218-238: setKeyspaceServiceLimit currently returns
http.StatusBadRequest for every error; change it to return 400 for
client/validation errors (e.g., invalid JSON from c.ShouldBindJSON, negative
req.ServiceLimit, or getKeyspaceIDByName failures that indicate bad input) but
return http.StatusInternalServerError (500) when h.store.SetKeyspaceServiceLimit
fails (indicating an internal/store error). Locate setKeyspaceServiceLimit and
replace the c.String(http.StatusBadRequest, err.Error()) after
h.store.SetKeyspaceServiceLimit(...) with
c.String(http.StatusInternalServerError, err.Error()), keeping other 400
responses as-is; ensure the error text is returned so callers see the failure
reason.
- Around line 198-216: The handler setControllerConfig currently applies updates
as it iterates the incoming map which can leave partial changes if one
UpdateControllerConfigItem fails; first validate all keys by iterating conf and
calling
reflectutil.FindJSONFullTagByChildTag(reflect.TypeOf(rmserver.ControllerConfig{}),
k) for every key and return 400 if any key is invalid, then in a second pass
apply h.store.UpdateControllerConfigItem for each validated key (or collect all
failures and abort before applying) so updates are atomic from the API's
perspective; reference setControllerConfig,
reflectutil.FindJSONFullTagByChildTag, rmserver.ControllerConfig, and
h.store.UpdateControllerConfigItem when making the change.
🧹 Nitpick comments (4)
pkg/mcs/resourcemanager/redirector/pd_metadata_handler.go (2)
106-117: Consider usingerrcode+errorRespinstead ofc.String()for error responses.The coding guidelines state HTTP handlers should use
errcode+errorResprather than plain string responses (which is essentiallyhttp.Error). All handlers in this file (postResourceGroup,putResourceGroup,getResourceGroup, etc.) usec.String(statusCode, err.Error())for error paths. Since the handler isn't wired into routing yet, this can be aligned later, but it's worth tracking.As per coding guidelines: "HTTP handlers should use errcode +
errorRespinstead ofhttp.Error".
132-154: Redundant nil check after error-based not-found.
GetResourceGroupon the store interface returns an error when the group doesn't exist (the test store returnsErrResourceGroupNotExists). The nil check at Line 149 would only trigger if a store implementation returns(nil, nil)— an ambiguous contract. If the interface contract is that not-found returns an error, the nil guard is dead code; if it's intentional defensive coding, consider documenting the contract on the interface.pkg/mcs/resourcemanager/redirector/pd_metadata_handler_test.go (2)
81-125: Consider adding a test for the negativeservice_limitvalidation path.The handler at Line 229–231 rejects negative
service_limitvalues with 400. This validation branch isn't exercised by any test. A small addition would improve confidence:resp = doJSONRequest(re, handler, http.MethodPost, "/resource-manager/api/v1/config/keyspace/service-limit/path_keyspace", map[string]float64{"service_limit": -1}) re.Equal(http.StatusBadRequest, resp.Code)
185-187:GetResourceGroupListalways returns empty — consider exercising it.The test store stub returns an empty slice regardless of input, and the handler's
getResourceGroupListendpoint isn't tested. If this is intentionally deferred, that's fine, but it leaves the list endpoint and its error-mapping logic uncovered.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10246 +/- ##
==========================================
+ Coverage 78.88% 78.90% +0.02%
==========================================
Files 527 529 +2
Lines 70920 71097 +177
==========================================
+ Hits 55944 56101 +157
- Misses 10978 10988 +10
- Partials 3998 4008 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
release-note-none
…-pd-metadata-http-handler
Signed-off-by: okjiang <819421878@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/mcs/resourcemanager/redirector/pd_metadata_handler_test.go`:
- Around line 128-132: The test currently injects an error via
store.setServiceLimitErr and asserts http.StatusBadRequest; after fixing
setKeyspaceServiceLimit to return 500 on internal store errors, update the test
in pd_metadata_handler_test.go to expect http.StatusInternalServerError instead
of http.StatusBadRequest (locate the block that calls doJSONRequest with POST to
"/resource-manager/api/v1/config/keyspace/service-limit/path_keyspace" and using
store.setServiceLimitErr). Ensure the assertion compares resp.Code to
http.StatusInternalServerError and adjust any related test setup or comments if
present.
In `@pkg/mcs/resourcemanager/redirector/pd_metadata_handler.go`:
- Around line 119-130: The putResourceGroup handler currently returns 500 for
all ModifyResourceGroup errors; update putResourceGroup to check if
h.store.ModifyResourceGroup(&group) returns the sentinel
ErrResourceGroupNotExists and respond with c.String(http.StatusNotFound,
err.Error()) (matching getResourceGroup/deleteResourceGroup behavior), otherwise
keep returning 500 for other errors and 200 on success; reference the
ModifyResourceGroup call in putResourceGroup and the ErrResourceGroupNotExists
error constant to implement the conditional check.
🧹 Nitpick comments (2)
pkg/mcs/resourcemanager/redirector/pd_metadata_handler.go (1)
76-84: Consider settinggin.SetMode(gin.ReleaseMode)to suppress debug output.
gin.New()in default mode emits debug-level route registration logs to stdout. In production this is noisy. Most PD handlers set release mode or use a custom logger.func newPDMetadataHandlerWithStore(store pdMetadataStore) http.Handler { + gin.SetMode(gin.ReleaseMode) handler := &pdMetadataHandler{ store: store, engine: gin.New(), }pkg/mcs/resourcemanager/redirector/pd_metadata_handler_test.go (1)
135-142:TestNewPDMetadataHandleronly checksNotNil— consider a smoke request.The test verifies construction but doesn't exercise routing through the real adapter. A single GET to
/config/groupswould confirm wiring end-to-end.
Signed-off-by: okjiang <819421878@qq.com>
okJiang
changed the title
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
okJiang
changed the title
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
|
/retest |
|
|
||
| // TODO: Deduplicate metadata /config handlers with pkg/mcs/resourcemanager/server/apis/v1/api.go | ||
| // once PD and RM handler error mapping can share one abstraction safely. | ||
| func (h *pdMetadataHandler) registerRouter() { |
There was a problem hiding this comment.
When deploying RM microservices, PD uses a redirector for forwarding. Implementing a simple handler in the redirector is more lightweight than directly reusing the entire ResourceManager service.
There was a problem hiding this comment.
pdMetadataHandler is used to handle write operations in PD side
| if keyspaceName != "" { | ||
| return keyspaceName | ||
| } | ||
| return c.Query("keyspace_name") |
There was a problem hiding this comment.
Why support this query? And I think this function can be merged into the getKeyspaceIDByName.
| keyspaceName := getServiceLimitTargetKeyspace(c) | ||
| keyspaceID, err := h.getKeyspaceIDByName(c, keyspaceName) | ||
| if err != nil { | ||
| c.String(http.StatusBadRequest, err.Error()) |
There was a problem hiding this comment.
If the keyspace name does not exist, getKeyspaceIDByName will return an error. I don't think this is a bad request; it should return 404.
Signed-off-by: okjiang <819421878@qq.com>
ti-chi-bot
Bot
added
the
release-note
There was a problem hiding this comment.
The file name should also be changed.
There was a problem hiding this comment.
Fixed in cb775ac. The redirector file pair has been renamed to metadata_handler.go and metadata_handler_test.go, so the old pd_metadata_handler* naming is fully removed now.
| } | ||
|
|
||
| // Store abstracts metadata operations for config APIs. | ||
| type Store interface { |
There was a problem hiding this comment.
Is there a better name than store?
There was a problem hiding this comment.
Fixed in cb775ac. I renamed the generic Store interface in metadataapi to ConfigStore, and updated the corresponding field/parameter names (configStore) to make the scope clearer while keeping ManagerStore unchanged.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at the full interface definition and surrounding context
rg -n "interface|func\s" pkg/mcs/resourcemanager/metadataapi/config_service.go | head -60
echo "---"
# See the full file
cat pkg/mcs/resourcemanager/metadataapi/config_service.go
Length of output: 13135
@rleungx, looking at the full interface, ConfigStore has three distinct concerns: resource group CRUD, controller config read/write, and keyspace service limits. The "Config" prefix undersells its scope, and "Store" implies a persistence layer rather than a manager-wrapping abstraction.
A few options, in order of preference:
-
ResourceManagerStore— most accurate to what the interface actually covers (all resource manager metadata), and unambiguous for callers (metadataapi.ResourceManagerStore). The one downside is a slight asymmetry with the concreteManagerStore. -
Backend/ManagerBackend— idiomatic Go pattern for dependency-injection interfaces. Rename the concrete type toManagerBackendtoo for consistency:// Interface type Backend interface { ... } // Concrete adapter type ManagerBackend struct { ... } func NewManagerBackend(manager *rmserver.Manager) *ManagerBackend { ... }
This pairing is clean and self-documenting.
-
MetadataStore— fits the package theme, butmetadataapi.MetadataStoreis somewhat redundant.
My recommendation is Backend (with ManagerBackend for the concrete type) as it follows the common Go idiom for abstracting dependencies, gives a clean interface/impl pair, and avoids conflating persistence semantics with what is really a manager delegation layer.
Signed-off-by: okjiang <819421878@qq.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/ok-to-retest |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
|
/retest |
1 similar comment
|
/retest |
|
@niubell: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, niubell, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
ti-chi-bot
Bot
added
release-note-none
4 participants
What problem does this PR solve?
Issue Number: ref #9737
Tracking issue: #9737
Discussion baseline: #9737 (comment)
This PR removes metadata handler logic drift between PD redirector fallback and
RM service APIs. Before this change, both sides handled the same
/resource-manager/api/v1/config/**metadata operations with duplicatedimplementations and gradually diverging status-code semantics.
What is changed and how does it work?
pkg/mcs/resourcemanager/metadataapi.ConfigServiceas the single metadata business-logic implementation.Storeinterface andManagerStoreadapter over*rmserver.Manager.pkg/mcs/resourcemanager/redirector/pd_metadata_handler.go) is now a thin route shell delegating toConfigService.pkg/mcs/resourcemanager/server/apis/v1/api.go) keeps existing route definitions and swagger-commented wrapper methods, but wrappers now delegate toConfigService.shouldHandlePDMetadataLocallyremainsfalse.X-Forbidden-Forward-To-Microservice: true.Unified HTTP semantics in this PR:
400 Bad Requestservice_limit < 0404 Not FoundGET/PUT/DELETE)/config/keyspace/service-limit*)GET)403 ForbiddenIsMetadataWriteDisabledError)500 Internal Server ErrorKeyspace resolution rule in service-limit APIs:
:keyspace_namefirstkeyspace_namewhen path param is emptyIn scope:
Out of scope:
Behavior changes:
/config/keyspace/service-limit*: keyspace-not-found now returns404(was partly400before)PUT /config/group: non-existing resource group now returns404Risk:
Check List
Tests
go test ./pkg/mcs/resourcemanager/metadataapi ./pkg/mcs/resourcemanager/redirector ./pkg/mcs/resourcemanager/server/apis/v1 -count=1make failpoint-enable && (cd tests/integrations && go test ./mcs/resourcemanager -run 'TestResourceManagerAPI|TestResourceManagerRedirector' -count=1); rc=$?; make failpoint-disable; exit $rcCode changes
Side effects
400responses in changed casesRelated changes
pingcap/docs/pingcap/docs-cn: N/Apingcap/tiup: N/ARelease note