mcs: forward current http request to mcs #7078

lhy1024 · 2023-09-11T15:30:33Z

What problem does this PR solve?

Issue Number: Ref #5839

What is changed and how does it work?

It can be reviewed after #7072 merged.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-09-11T15:30:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: lhy1024 <[email protected]>

codecov · 2023-09-11T17:43:59Z

Codecov Report

Merging #7078 (2193aff) into master (d7d4756) will increase coverage by 0.13%.
The diff coverage is 98.07%.

❗ Current head 2193aff differs from pull request most recent head 338058d. Consider uploading reports for the commit 338058d to get more accurate results

@@            Coverage Diff             @@
##           master    #7078      +/-   ##
==========================================
+ Coverage   74.33%   74.47%   +0.13%     
==========================================
  Files         437      437              
  Lines       46821    46854      +33     
==========================================
+ Hits        34805    34894      +89     
+ Misses       8957     8904      -53     
+ Partials     3059     3056       -3

Flag	Coverage Δ
unittests	`74.47% <98.07%> (+0.13%)`	⬆️

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

Signed-off-by: lhy1024 <[email protected]>

pkg/utils/apiutil/serverapi/middleware.go

rleungx · 2023-09-12T03:26:21Z

tests/pdctl/scheduler/scheduler_test.go

@@ -530,3 +530,28 @@ func mightExec(re *require.Assertions, cmd *cobra.Command, args []string, v inte
 	}
 	json.Unmarshal(output, v)
 }
+
+func TestMicroservice(t *testing.T) {


How to know if the output comes from the scheduling server?

Because we have already disabled PD scheduling in api mode? How about adding info in the response header for further confirmation?

How about using a more concrete name? TestMicroservice is too general.

Signed-off-by: lhy1024 <[email protected]>

server/api/server.go

Signed-off-by: lhy1024 <[email protected]>

rleungx · 2023-09-13T06:21:06Z

server/api/server.go

@@ -45,19 +45,25 @@ func NewHandler(_ context.Context, svr *server.Server) (http.Handler, apiutil.AP
 			serverapi.MicroserviceRedirectRule(
 				prefix+"/admin/reset-ts",
 				tsoapi.APIPathPrefix+"/admin/reset-ts",
-				mcs.TSOServiceName),
+				mcs.TSOServiceName,
+				[]string{http.MethodPost}),
 			serverapi.MicroserviceRedirectRule(
 				prefix+"/operators",


How about the config or other paths?

I will resolve them in other PRs, this PR only forwards the current HTTP method in scheduling server.

rleungx · 2023-09-14T02:45:52Z

pkg/utils/apiutil/apiutil.go

 func copyHeader(dst, src http.Header) {
 	for k, vv := range src {
+		if k == "Content-Encoding" || k == "Content-Length" {


I have a concern about it and am not sure if only two keys will affect it.

It's possible, but for now gzip only affects these two, and we'll do more testing as we add interfaces.

rleungx · 2023-09-14T02:48:38Z

pkg/utils/apiutil/serverapi/middleware.go

+			// e.g. r.URL.Path = /pd/api/v1/operators/1 (before redirect)
+			//      matchPath  = /pd/api/v1/operators
+			//      targetPath = /scheduling/api/v1/operators
+			//      r.URL.Path = /scheduling/api/v1/operator/1 (after redirect)


How about using the custom way to do the transfer? Because we might change the previous path parameters to query parameters.

I will add TODO when meet other interfaces, which not support restful

rleungx · 2023-09-14T02:49:41Z

tests/pdctl/scheduler/scheduler_test.go

@@ -530,3 +530,28 @@ func mightExec(re *require.Assertions, cmd *cobra.Command, args []string, v inte
 	}
 	json.Unmarshal(output, v)
 }
+
+func TestMicroservice(t *testing.T) {


How about using a more concrete name? TestMicroservice is too general.

Signed-off-by: lhy1024 <[email protected]>

JmPotato · 2023-09-15T05:20:58Z

tests/pdctl/operator/operator_test.go

+	cmd := pdctlCmd.GetRootCmd()
+	args := []string{"-u", backendEndpoints, "operator", "show"}
+	var slice []string
+	output, err := pdctl.ExecuteCommand(cmd, args...)


How should we make sure this request is forwarded rather than being handled directly in this test?

#7078 (comment)

JmPotato · 2023-09-15T05:21:03Z

tests/pdctl/scheduler/scheduler_test.go

@@ -530,3 +530,28 @@ func mightExec(re *require.Assertions, cmd *cobra.Command, args []string, v inte
 	}
 	json.Unmarshal(output, v)
 }
+
+func TestForwardSchedulerRequest(t *testing.T) {


Signed-off-by: lhy1024 <[email protected]>

rleungx · 2023-09-15T09:25:12Z

pkg/utils/apiutil/serverapi/middleware.go

@@ -171,6 +171,7 @@ func (h *redirector) ServeHTTP(w http.ResponseWriter, r *http.Request, next http
 			return
 		}
 		clientUrls = append(clientUrls, targetAddr)
+		w.Header().Set(apiutil.ForwardToMicroServiceHeader, "true")


Is it only used for testing?

Yes, it is now.

Then, I think we shouldn't add it in the header, the testing code should not affect the normal situation.

If it is not added to resp, is there any other way to tell whether it is processed by the PD server or the scheduling server?

When debugging on a real cluster, it may also be useful to identify the origin of the server handling the request.

JmPotato · 2023-09-18T03:29:46Z

pkg/utils/apiutil/serverapi/middleware.go

 		if len(targetAddr) == 0 {
 			http.Error(w, apiutil.ErrRedirectFailed, http.StatusInternalServerError)
 			return
 		}
 		clientUrls = append(clientUrls, targetAddr)
+		w.Header().Set(apiutil.ForwardToMicroServiceHeader, "true")


If we keep this header filed, what about setting its value to the microservice name rather than a true?

e.g scheduling？tso？Is it necessary?

pkg/utils/apiutil/serverapi/middleware.go

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2023-09-18T07:55:11Z

/merge

ti-chi-bot · 2023-09-18T07:55:13Z

@lhy1024: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-09-18T07:55:14Z

This pull request has been accepted and is ready to merge.

Commit hash: f070884

ti-chi-bot · 2023-09-18T07:55:29Z

@lhy1024: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ref tikv#5839 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2023

ti-chi-bot bot requested review from JmPotato and rleungx September 11, 2023 15:30

lhy1024 force-pushed the sche-redirect branch from dca91b5 to eb64cd5 Compare September 11, 2023 17:28

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2023

lhy1024 added 3 commits September 12, 2023 01:31

mcs: watch scheduling service primary address

4e6f3b3

Signed-off-by: lhy1024 <[email protected]>

address comments

780736b

Signed-off-by: lhy1024 <[email protected]>

mcs: forward current http request to mcs

4012981

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the sche-redirect branch from eb64cd5 to 4012981 Compare September 11, 2023 17:32

lhy1024 added 2 commits September 12, 2023 03:22

avoid unexpected EOF error with gzip

e8a88b9

Signed-off-by: lhy1024 <[email protected]>

fix test

0f733dc

Signed-off-by: lhy1024 <[email protected]>

rleungx reviewed Sep 12, 2023

View reviewed changes

address comments

32a81e3

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the sche-redirect branch from 5449b2d to 32a81e3 Compare September 12, 2023 11:10

Merge branch 'master' of github.com:tikv/pd into sche-redirect

7eafe59

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the sche-redirect branch from b1b7542 to 7eafe59 Compare September 13, 2023 04:32

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 13, 2023

JmPotato reviewed Sep 13, 2023

View reviewed changes

server/api/server.go Outdated Show resolved Hide resolved

add methods check

40f129f

Signed-off-by: lhy1024 <[email protected]>

lhy1024 requested review from JmPotato and rleungx September 13, 2023 06:18

rleungx reviewed Sep 13, 2023

View reviewed changes

rleungx reviewed Sep 14, 2023

View reviewed changes

lhy1024 added 2 commits September 14, 2023 11:39

address comments

98a573b

Signed-off-by: lhy1024 <[email protected]>

rename

590d971

Signed-off-by: lhy1024 <[email protected]>

rleungx approved these changes Sep 15, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 15, 2023

JmPotato reviewed Sep 15, 2023

View reviewed changes

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2023

add test

2193aff

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the sche-redirect branch from 75ee330 to 2193aff Compare September 15, 2023 08:23

rleungx reviewed Sep 15, 2023

View reviewed changes

rleungx mentioned this pull request Sep 15, 2023

Separate scheduling into an independent service #5839

Closed

lhy1024 requested review from JmPotato and rleungx September 18, 2023 02:09

JmPotato reviewed Sep 18, 2023

View reviewed changes

lhy1024 force-pushed the sche-redirect branch from 12eab46 to 47eb529 Compare September 18, 2023 06:17

rleungx reviewed Sep 18, 2023

View reviewed changes

pkg/utils/apiutil/serverapi/middleware.go Outdated Show resolved Hide resolved

add test with failpoint

f070884

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the sche-redirect branch from 81e49f8 to f070884 Compare September 18, 2023 07:38

rleungx approved these changes Sep 18, 2023

View reviewed changes

JmPotato approved these changes Sep 18, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 18, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 18, 2023

Merge branch 'master' into sche-redirect

338058d

ti-chi-bot bot merged commit 0888ef6 into tikv:master Sep 18, 2023
22 checks passed

lhy1024 mentioned this pull request Oct 9, 2023

mcs: forward request to scheduling server #7178

Closed

25 tasks

rleungx pushed a commit to rleungx/pd that referenced this pull request Dec 1, 2023

mcs: forward current http request to mcs (tikv#7078)

c6fbf60

ref tikv#5839 Signed-off-by: lhy1024 <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

HuSharp mentioned this pull request Aug 21, 2024

Add middleware to PD microservice #8548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: forward current http request to mcs #7078

mcs: forward current http request to mcs #7078

lhy1024 commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

rleungx Sep 12, 2023

lhy1024 Sep 12, 2023

rleungx Sep 14, 2023

rleungx Sep 13, 2023

lhy1024 Sep 13, 2023

rleungx Sep 14, 2023

lhy1024 Sep 14, 2023

rleungx Sep 14, 2023

lhy1024 Sep 14, 2023

rleungx Sep 14, 2023

JmPotato Sep 15, 2023

lhy1024 Sep 15, 2023

JmPotato Sep 15, 2023

rleungx Sep 15, 2023

lhy1024 Sep 15, 2023

rleungx Sep 15, 2023

lhy1024 Sep 15, 2023

lhy1024 Sep 15, 2023

JmPotato Sep 18, 2023

lhy1024 Sep 18, 2023 •

edited

Loading

lhy1024 commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

mcs: forward current http request to mcs #7078

mcs: forward current http request to mcs #7078

Conversation

lhy1024 commented Sep 11, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Sep 11, 2023 • edited Loading

codecov bot commented Sep 11, 2023 • 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

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

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

lhy1024 Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

lhy1024 commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 18, 2023

ti-chi-bot bot commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

lhy1024 Sep 18, 2023 •

edited

Loading