ctl: replace doRequest with HTTP client to get health status #8212

okJiang · 2024-05-23T08:22:44Z

What problem does this PR solve?

Issue Number: Ref #7300

What is changed and how does it work?

Check List

Tests

Unit test

Code changes

Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)

Release note

None.

ti-chi-bot · 2024-05-23T08:22:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

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.

client/http/interface.go

tools/pd-ctl/pdctl/command/health_command.go

Signed-off-by: okJiang <819421878@qq.com>

ref tikv#7969 add parallel parameter Signed-off-by: husharp <jinhao.hu@pingcap.com> Signed-off-by: okJiang <819421878@qq.com>

ref tikv#7897 metrics: fix the duplicate avg metrics Signed-off-by: nolouch <nolouch@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Signed-off-by: okJiang <819421878@qq.com>

Co-authored-by: JmPotato <github@ipotato.me> Signed-off-by: okJiang <819421878@qq.com>

JmPotato

The rest LGTM.

client/http/request_info.go

tools/pd-ctl/pdctl/command/health_command.go

Signed-off-by: okJiang <819421878@qq.com>

…tl-health

JmPotato · 2024-05-23T09:07:50Z

client/http/interface.go

@@ -337,6 +338,20 @@ func (c *client) SetStoreLabels(ctx context.Context, storeID int64, storeLabels
 		WithBody(jsonInput))
 }

+// GetHealthStatus gets the health status of the cluster.
+func (c *client) GetHealthStatus(ctx context.Context) ([]Health, error) {


BTW, could you please add tests for this new method in /tests/integrations/client/http_client_test.go?

Signed-off-by: okJiang <819421878@qq.com>

JmPotato · 2024-05-23T13:09:48Z

/merge

ti-chi-bot · 2024-05-23T13:09:49Z

@JmPotato: 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.

HuSharp · 2024-05-23T14:21:26Z

tools/pd-ctl/pdctl/command/health_command.go

-		Run:   showHealthCommandFunc,
+		Use:               "health",
+		Short:             "show all node's health information of the PD cluster",
+		PersistentPreRunE: requirePDClient,


TestSendAndGetComponent ci failed :(

This is because the requirePDClient command executes the function shouldInitPDClient, which requires querying pd/api/v1/cluster.

The handler is replaced in TestSendAndGetComponent, so you need to add this router to the handler in TestSendAndGetComponent.

mux.HandleFunc("/pd/api/v1/cluster", func(w http.ResponseWriter, r *http.Request) { cluster := &metapb.Cluster{ Id: 0, } var rd render.Render rd.JSON(w, http.StatusOK, cluster) })

You can check code here

pd/tools/pd-ctl/tests/global_test.go

Lines 37 to 52 in 7e18a69

handler := func(context.Context, *server.Server) (http.Handler, apiutil.APIServiceGroup, error) {

mux := http.NewServeMux()

mux.HandleFunc("/pd/api/v1/health", func(w http.ResponseWriter, r *http.Request) {

callerID := apiutil.GetCallerIDOnHTTP(r)

for k := range r.Header {

log.Info("header", zap.String("key", k))

}

log.Info("caller id", zap.String("caller-id", callerID))

re.Equal(pdControlCallerID, callerID)

fmt.Fprint(w, callerID)

})

info := apiutil.APIServiceGroup{

IsCore: true,

}

return mux, info, nil

}

Thanks for your reminder and suggestion. I will fix it tomorrow.

JmPotato · 2024-05-23T15:03:10Z

/merge cancel

Signed-off-by: okJiang <819421878@qq.com>

…tl-health

Signed-off-by: okJiang <819421878@qq.com>

codecov · 2024-05-24T06:24:57Z

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.41%. Comparing base (7cc3b4e) to head (132fe16).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8212      +/-   ##
==========================================
+ Coverage   77.30%   77.41%   +0.11%     
==========================================
  Files         471      471              
  Lines       61445    61456      +11     
==========================================
+ Hits        47498    47577      +79     
+ Misses      10386    10315      -71     
- Partials     3561     3564       +3

Flag	Coverage Δ
unittests	`77.41% <87.50%> (+0.11%)`	⬆️

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

Signed-off-by: okJiang <819421878@qq.com>

HuSharp · 2024-05-24T06:31:33Z

tools/pd-ctl/tests/global_test.go

+			healths := []api.Health{
+				{
+					Name: "test",
+				},
+			}
+			healthsBytes, err := json.Marshal(healths)
+			re.NoError(err)
+			w.Write(healthsBytes)


It seems like this test wants to focus on calledID, so I think it's no need to change it.

FYI https://github.com/tikv/pd/pull/4491/files#diff-05102750a57e22445f5760fb1545227c9130a3a093f3b879e1f6e48ce27ea890

change cluster API because it is a required path

L55 has tested it. If we keep returning calledID, json.UnMarshal will be failed.

HuSharp · 2024-05-24T06:35:14Z

tools/pd-ctl/tests/global_test.go

 	cmd "github.com/tikv/pd/tools/pd-ctl/pdctl"
 	"go.uber.org/zap"
 )

-const pdControlCallerID = "pd-ctl"
+const pdControlCallerID = "pd-http-client"


there need to be pd-ctl, maybe You can reuse the variables in the global.go file directly

pd/tools/pd-ctl/pdctl/command/global.go

Line 36 in 7e18a69

pdControlCallerID = "pd-ctl"

FYI

pd/tools/pd-ctl/pdctl/command/global.go

Lines 103 to 112 in 7e18a69

func initNewPDClient(cmd *cobra.Command, opts ...pd.ClientOption) error {

if should, err := shouldInitPDClient(cmd); !should || err != nil {

return err

}

if PDCli != nil {

PDCli.Close()

}

PDCli = pd.NewClient(pdControlCallerID, getEndpoints(cmd), opts...)

return nil

}

It's a little strange here. The unit test requires it to be pd-http-client. I'll take a look for it

pd/client/http/client.go

Line 310 in 4cd42b3

c := &client{inner: newClientInner(ctx, cancel, source), callerID: defaultCallerID}

The HTTP client created in the pd-ctl is assigned by defaultCallerID. Do we need to change it to source. source equals to "pd-ctl" string.

It is a bug :( Have been fixed in this pr. #8214

I think you don't need open a new pr to fix it. We even need to modify the conflict for it. If it is bug, I can fix it in this pr easily. How was my propose?

Create an issue, link this pr, and then I can fix it.

I think this is a different issue, maybe with a new pr will be a little clearer 0 0

Because some background information is needed

After replacing HTTP with SDK #7490, we need to add the calledID explicitly to keep consistent with #4491
And source ID can be consistent with caller ID, #7565

It is ok for me. Let's wait for #8214 merged

Signed-off-by: okJiang <819421878@qq.com>

HuSharp · 2024-05-27T02:48:30Z

tools/pd-ctl/tests/global_test.go

@@ -45,12 +45,6 @@ func TestSendAndGetComponent(t *testing.T) {
 			w.Write(clusterBytes)
 		})
 		// check http client api
-		// TODO: remove this comment after replacing dialClient with the PD HTTP client completely.


please retain this comment, rest LGTM!

Signed-off-by: okJiang <819421878@qq.com>

HuSharp · 2024-05-27T03:20:43Z

/merge

ti-chi-bot · 2024-05-27T03:20:45Z

@HuSharp: 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 · 2024-05-27T03:20:47Z

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

Commit hash: 132fe16

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 23, 2024

ti-chi-bot bot requested review from JmPotato and rleungx May 23, 2024 08:22

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2024

okJiang force-pushed the pdctl-health branch from 91e609e to ec6e1b5 Compare May 23, 2024 08:28

JmPotato reviewed May 23, 2024

View reviewed changes

client/http/interface.go Show resolved Hide resolved

tools/pd-ctl/pdctl/command/health_command.go Outdated Show resolved Hide resolved

okJiang and others added 5 commits May 23, 2024 16:35

command: replace doRequest of HTTP client to get health status

f1fb830

Signed-off-by: okJiang <819421878@qq.com>

add a note

ee483b0

Signed-off-by: okJiang <819421878@qq.com>

tools/ut: add a parallel parameter (tikv#8186)

9f60979

ref tikv#7969 add parallel parameter Signed-off-by: husharp <jinhao.hu@pingcap.com> Signed-off-by: okJiang <819421878@qq.com>

metrics: fix the duplicate avg metrics (tikv#8210)

27a74a5

ref tikv#7897 metrics: fix the duplicate avg metrics Signed-off-by: nolouch <nolouch@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Signed-off-by: okJiang <819421878@qq.com>

Apply suggestions from code review

09bc239

Co-authored-by: JmPotato <github@ipotato.me> Signed-off-by: okJiang <819421878@qq.com>

okJiang force-pushed the pdctl-health branch from caa5f7c to 09bc239 Compare May 23, 2024 08:35

Merge branch 'master' into pdctl-health

ec44793

JmPotato approved these changes May 23, 2024

View reviewed changes

client/http/request_info.go Outdated Show resolved Hide resolved

tools/pd-ctl/pdctl/command/health_command.go Outdated Show resolved Hide resolved

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 23, 2024

okJiang added 2 commits May 23, 2024 16:50

fix comment

fd7e738

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'pdctl-health' of https://github.com/okJiang/pd into pdc…

794c735

…tl-health

JmPotato reviewed May 23, 2024

View reviewed changes

okJiang added 2 commits May 23, 2024 17:24

fix comment: add ut

b123494

Signed-off-by: okJiang <819421878@qq.com>

fix lint

d31faf8

Signed-off-by: okJiang <819421878@qq.com>

nolouch approved these changes May 23, 2024

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 May 23, 2024

Merge branch 'master' into pdctl-health

70d567c

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 23, 2024

HuSharp reviewed May 23, 2024

View reviewed changes

HuSharp removed the status/can-merge Indicates a PR has been approved by a committer. label May 23, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 23, 2024

ti-chi-bot bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 23, 2024

okJiang added 2 commits May 24, 2024 14:15

fix ci

cef9427

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'pdctl-health' of https://github.com/okJiang/pd into pdc…

f6f0f28

…tl-health

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 24, 2024

make check

120c0d4

Signed-off-by: okJiang <819421878@qq.com>

okJiang added 2 commits May 24, 2024 14:25

make check

a425cba

Signed-off-by: okJiang <819421878@qq.com>

fix lint

eea4858

Signed-off-by: okJiang <819421878@qq.com>

HuSharp reviewed May 24, 2024

View reviewed changes

Merge branch 'master' of https://github.com/tikv/pd into pdctl-health

76a0902

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2024

HuSharp reviewed May 27, 2024

View reviewed changes

revert comment

132fe16

Signed-off-by: okJiang <819421878@qq.com>

HuSharp approved these changes May 27, 2024

View reviewed changes

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 27, 2024

ti-chi-bot bot merged commit dd7f2a7 into tikv:master May 27, 2024
19 checks passed

HuSharp mentioned this pull request May 27, 2024

Tracking issue for the HTTP SDK #7300

Open

7 tasks

okJiang deleted the pdctl-health branch June 7, 2024 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ctl: replace doRequest with HTTP client to get health status #8212

ctl: replace doRequest with HTTP client to get health status #8212

okJiang commented May 23, 2024

ti-chi-bot bot commented May 23, 2024 •

edited

Loading

JmPotato left a comment

JmPotato May 23, 2024

okJiang May 23, 2024

JmPotato commented May 23, 2024

ti-chi-bot bot commented May 23, 2024

HuSharp May 23, 2024

HuSharp May 23, 2024

okJiang May 23, 2024

JmPotato commented May 23, 2024

codecov bot commented May 24, 2024 •

edited

Loading

HuSharp May 24, 2024 •

edited

Loading

okJiang May 24, 2024

HuSharp May 24, 2024

okJiang May 24, 2024 •

edited

Loading

okJiang May 24, 2024

HuSharp May 24, 2024

okJiang May 24, 2024 •

edited

Loading

HuSharp May 24, 2024 •

edited

Loading

okJiang May 24, 2024 •

edited

Loading

HuSharp May 27, 2024

HuSharp commented May 27, 2024

ti-chi-bot bot commented May 27, 2024

ti-chi-bot bot commented May 27, 2024

	handler := func(context.Context, *server.Server) (http.Handler, apiutil.APIServiceGroup, error) {
	mux := http.NewServeMux()
	mux.HandleFunc("/pd/api/v1/health", func(w http.ResponseWriter, r *http.Request) {
	callerID := apiutil.GetCallerIDOnHTTP(r)
	for k := range r.Header {
	log.Info("header", zap.String("key", k))
	}
	log.Info("caller id", zap.String("caller-id", callerID))
	re.Equal(pdControlCallerID, callerID)
	fmt.Fprint(w, callerID)
	})
	info := apiutil.APIServiceGroup{
	IsCore: true,
	}
	return mux, info, nil
	}

	func initNewPDClient(cmd *cobra.Command, opts ...pd.ClientOption) error {
	if should, err := shouldInitPDClient(cmd); !should \|\| err != nil {
	return err
	}
	if PDCli != nil {
	PDCli.Close()
	}
	PDCli = pd.NewClient(pdControlCallerID, getEndpoints(cmd), opts...)
	return nil
	}

ctl: replace doRequest with HTTP client to get health status #8212

ctl: replace doRequest with HTTP client to get health status #8212

Conversation

okJiang commented May 23, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 23, 2024 • edited Loading

JmPotato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JmPotato commented May 23, 2024

ti-chi-bot bot commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JmPotato commented May 23, 2024

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

HuSharp May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang May 24, 2024 • edited Loading

Choose a reason for hiding this comment

HuSharp May 24, 2024 • edited Loading

Choose a reason for hiding this comment

okJiang May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp commented May 27, 2024

ti-chi-bot bot commented May 27, 2024

ti-chi-bot bot commented May 27, 2024

ti-chi-bot bot commented May 23, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

HuSharp May 24, 2024 •

edited

Loading

okJiang May 24, 2024 •

edited

Loading

okJiang May 24, 2024 •

edited

Loading

HuSharp May 24, 2024 •

edited

Loading

okJiang May 24, 2024 •

edited

Loading