mcs: add affinity redirect and scheduling watcher #10042
Conversation
ti-chi-bot
bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
dco-signoff: yes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10042 +/- ##
==========================================
+ Coverage 78.62% 78.72% +0.10%
==========================================
Files 520 522 +2
Lines 70089 70270 +181
==========================================
+ Hits 55105 55318 +213
+ Misses 10998 10946 -52
- Partials 3986 4006 +20
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
lhy1024
force-pushed
the
affinity-pr6
branch
2 times, most recently
from
079ad98 to
4dc8ea5
Compare
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
lhy1024
force-pushed
the
affinity-pr6
branch
from
aad6e2f to
198a8dc
Compare
|
/retest |
1 similar comment
|
/retest |
lhy1024
force-pushed
the
affinity-pr6
branch
4 times, most recently
from
0720b5d to
64d7903
Compare
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
| } | ||
|
|
||
| // AffinityMicroserviceRedirector only forwards affinity GET requests to the scheduling service. | ||
| func AffinityMicroserviceRedirector() gin.HandlerFunc { |
There was a problem hiding this comment.
Why not in server/api/server.go?
There was a problem hiding this comment.
it is a middleware for v2
|
/retest |
pkg/schedule/affinity/manager.go
Outdated
| labelRule = m.labelRuleBuffer[group.ID] | ||
| } | ||
| // Once group created, the buffer must be deleted. | ||
| delete(m.labelRuleBuffer, group.ID) |
There was a problem hiding this comment.
Is it better to delete after updateGroupLabelRuleLockedWithCount?
| return | ||
| } | ||
| // Attach only non-empty label rules to avoid marking the group as having ranges when it does not. | ||
| if len(gkr.KeyRanges) > 0 { |
There was a problem hiding this comment.
If len(keyRanges) is a default limit, can we put it inside updateGroupLabelRuleLockedWithCount
There was a problem hiding this comment.
Moving the check inside would break the other call.
| // Store LabelRule information in the buffer when it is synchronized before the group. | ||
| m.labelRuleBuffer[groupID] = labelRule | ||
| return nil | ||
| } |
There was a problem hiding this comment.
How about returning direct when !exist && len(gkr.KeyRanges) ==0?
| _, err = client.Put(ctx, labelKey, string(labelValue)) | ||
| re.NoError(err) | ||
|
|
||
| time.Sleep(100 * time.Millisecond) |
|
@coderabbitai full review |
|
@bufferflies @okJiang Friendly ping |
pkg/schedule/labeler/labeler.go
Outdated
| var toDelete []string | ||
| err := l.storage.LoadRegionRules(func(k, v string) { | ||
| r, err := NewLabelRuleFromJSON([]byte(v)) | ||
| r, err := NewLabelRuleFromJSONWithoutCheck([]byte(v)) |
There was a problem hiding this comment.
Yes, NewLabelRuleFromJSONWithoutCheck is origin NewLabelRuleFromJSON
| ) | ||
|
|
||
| // MicroserviceRedirector creates a middleware to rewrite and forward requests to a microservice primary. | ||
| func MicroserviceRedirector(rules ...serverapi.RedirectRule) gin.HandlerFunc { |
There was a problem hiding this comment.
We have many middleware in different packages, is it possible to reorganize them?
There was a problem hiding this comment.
we can do it in another pr?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/mcs/scheduling/server/affinity/watcher.go`:
- Around line 45-56: Update the Watcher struct to use a pointer waitgroup and
remove the stored context: change wg from type sync.WaitGroup to *sync.WaitGroup
and remove the ctx field; initialize it as &sync.WaitGroup{} in NewWatcher. Add
a ctx context.Context parameter to initializeGroupWatcher and
initializeAffinityLabelWatcher and, inside those functions, pass the ctx
argument (not w.ctx) and w.wg (not &w.wg) when calling etcdutil.NewLoopWatcher.
Ensure all method signatures and call sites that used w.ctx are updated to
accept and forward the context parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/mcs/scheduling/server/affinity/watcher_test.go`:
- Around line 199-279: The test TestConcurrentLabelRuleUpdates uses a value
WaitGroup which can be copied; change the declaration of wg to a pointer and
initialize it (e.g. wg := &sync.WaitGroup{}) so all goroutines use the same
*sync.WaitGroup instance; keep all existing wg.Add, wg.Done and wg.Wait calls
but ensure wg is created before the first Add to avoid nil dereference.
🧹 Nitpick comments (1)
pkg/mcs/scheduling/server/apis/v1/api.go (1)
1617-1634: Use errcode/errorResp helpers for affinity API errors.
These handlers return raw error strings via gin aborts; align them with the standard error response helpers for consistent API contracts.
As per coding guidelines: HTTP handlers in Go must use errcode anderrorResp; avoidhttp.Error.
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
Signed-off-by: lhy1024 <admin@liudos.us>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/apiv2/middlewares/microservice_redirector.go`:
- Around line 32-63: The MicroserviceRedirector middleware is not preserving
client addressing headers before proxying; update MicroserviceRedirector to set
X-Forwarded-For and X-Forwarded-Port on c.Request.Header just before calling
apiutil.NewCustomReverseProxies(...).ServeHTTP: obtain the client IP from
c.Request.RemoteAddr (or RemoteAddr split by ':'), append it to any existing
X-Forwarded-For value (comma-separated) and set X-Forwarded-Port from the
request's port (use r.URL.Port() if present, otherwise extract port from
RemoteAddr or infer 80/443), then proceed to create the reverse proxy; reference
the MicroserviceRedirector function, c.Request, apiutil.NewCustomReverseProxies,
and headers X-Forwarded-For / X-Forwarded-Port.
🧹 Nitpick comments (6)
pkg/mcs/scheduling/server/affinity/watcher_test.go (1)
234-242: Silently ignoredputLabelRuleerror in concurrent goroutine.If
putLabelRulefails, the test won't detect it and may pass or hang on theEventuallycall with a confusing timeout. Consider collecting errors or usingrequirewithin the goroutine (withtcaptured safely).♻️ Proposed fix to capture errors
for i := range groupCount { wg.Add(1) go func(idx int) { defer wg.Done() labelRule := makeTestLabelRule(groups[idx].ID, "7480000000000000ff"+string(rune('0'+idx))+"000000000000000f8", "7480000000000000ff"+string(rune('0'+idx))+"100000000000000f8", ) - _ = putLabelRule(ctx, client, labelRule) + if err := putLabelRule(ctx, client, labelRule); err != nil { + t.Logf("putLabelRule failed for group %d: %v", idx, err) + } }(i) }pkg/mcs/scheduling/server/affinity/watcher.go (1)
84-116: Group watcherputFnalso returns errors on unmarshal failure — consider the same treatment.Same concern as with the label watcher: if an invalid group JSON value exists in etcd,
putFnreturns the unmarshal error which may block the watcher. This is less likely since the group path is more specific, but the same defensive approach (log + return nil) could be applied for consistency.pkg/utils/apiutil/serverapi/middleware.go (2)
143-145:r.URL.Pathis mutated (trailing-slash stripped) even when no rule matches.Line 145 unconditionally strips trailing slashes from the request URL before rule matching. If no rule matches, the caller's request is still mutated. This was pre-existing behavior in the private method, but now that
MatchMicroserviceRedirectis a public API consumed by multiple callers (including the new gin middleware), this side effect is more visible and could surprise callers who expect an unmatched request to be unmodified.Consider guarding the mutation so it only applies when a match is found, or documenting this side effect prominently in the function's doc comment.
164-182: Path rewriting withRawPath: prefix trimming uses the non-rawMatchPath.On Line 176, when
r.URL.RawPathis non-empty,pathis set to the raw (percent-encoded) value but theTrimPrefixstill usesrule.MatchPath(the non-encoded form). If the match path ever contains characters that appear differently in the raw form (e.g., spaces encoded as%20), the prefix won't be trimmed correctly.In practice, route prefixes like
/pd/api/v1/operatorsare plain ASCII, so this is unlikely to bite today — but it's worth keeping in mind if more exotic paths are introduced later.pkg/utils/apiutil/serverapi/middleware_test.go (1)
36-147: Good edge-case coverage, but the happy-path (successful redirect with path rewriting) is missing.All five tests exercise conditions that prevent or short-circuit a redirect. Consider adding at least one test that:
- Verifies a successful match returns
(true, expectedAddr).- Asserts that
r.URL.Pathis correctly rewritten (e.g.,/pd/api/v1/operators/123→/scheduling/api/v1/operators/123).- Covers multiple rules where the first doesn't match and the second does.
This would guard the core redirect + path-rewriting logic against regressions.
server/apiv2/middlewares/microservice_redirector.go (1)
49-52: Consider using a structured error response instead of a plain string.
c.AbortWithStatusJSONhere serializes the.Error()string directly as the JSON body, producing a bare JSON string (e.g.,"redirect failed"). If the rest of the v2 API uses a structured error envelope (e.g.,{"code": ..., "message": ...}), this would be inconsistent. The same applies to line 57.
|
/retest |
Signed-off-by: lhy1024 <admin@liudos.us>
ti-chi-bot
bot
added
needs-1-more-lgtm
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, okJiang 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 |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
1 similar comment
|
/retest |
ref tikv#9764 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
5 participants
What problem does this PR solve?
Issue Number: Ref #9764
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit
New Features
Improvements
Tests