server: add limiter config and reload mechanism #4842

CabinfeverB · 2022-04-25T03:38:09Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: Ref #4666
This PR should be merged after #4839
**This PR should be merged after #4869 **
This PR is used to reload the limit config for limiter after leader resign or restart and do some integration tests

What is changed and how it works?

update limiter config when reload presist config

update limiter config when reload presist config

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2022-04-25T03:38:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AndreMouche
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.

codecov · 2022-04-25T03:45:42Z

Codecov Report

Merging #4842 (12df5b3) into master (958d687) will increase coverage by 0.13%.
The diff coverage is 87.75%.

@@            Coverage Diff             @@
##           master    #4842      +/-   ##
==========================================
+ Coverage   75.43%   75.56%   +0.13%     
==========================================
  Files         309      309              
  Lines       30507    30552      +45     
==========================================
+ Hits        23012    23086      +74     
+ Misses       5475     5459      -16     
+ Partials     2020     2007      -13

Flag	Coverage Δ
unittests	`75.56% <87.75%> (+0.13%)`	⬆️

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

Impacted Files	Coverage Δ
server/api/service_middleware.go	`68.96% <73.33%> (+0.78%)`	⬆️
server/server.go	`73.30% <90.00%> (+1.26%)`	⬆️
server/config/service_middleware_config.go	`100.00% <100.00%> (+25.00%)`	⬆️
...erver/config/service_middleware_persist_options.go	`91.66% <100.00%> (+2.77%)`	⬆️
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
server/schedule/operator_controller.go	`84.56% <0.00%> (-1.21%)`	⬇️
tools/pd-ctl/pdctl/command/operator.go	`66.66% <0.00%> (-1.15%)`	⬇️
server/encryptionkm/key_manager.go	`72.90% <0.00%> (-0.80%)`	⬇️
server/cluster/coordinator.go	`70.45% <0.00%> (-0.40%)`	⬇️
server/cluster/cluster.go	`83.08% <0.00%> (-0.19%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 958d687...12df5b3. Read the comment docs.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-02T04:37:49Z

server/server.go

@@ -1475,6 +1506,14 @@ func (s *Server) reloadConfigFromKV() error {
 	return nil
 }

+func (s *Server) loadRateLimitConfig() {


Why do we need it?

The configuration of the Limiter differs from that of EnableRateLimit. The value of EnableRateLimit is fetched from PersistOption on every request, but Limiter does not. So after resigning leader, we should reload the associated configuration.

rleungx · 2022-06-02T04:37:55Z

server/config/service_middleware_config.go

 }

 // NewServiceMiddlewareConfig returns a new service middleware config
 func NewServiceMiddlewareConfig() *ServiceMiddlewareConfig {
 	audit := AuditConfig{
 		EnableAudit: defaultEnableAuditMiddleware,
 	}
+	ratelimit := RateLimitConfig{
+		EnableRateLimit: defaultEnableRateLimitMiddleware,


AndreMouche · 2022-06-07T02:53:38Z

server/api/service_middleware_test.go

 	}
 	postData, err = json.Marshal(ms)
 	c.Assert(err, IsNil)
 	c.Assert(tu.CheckPostJSON(testDialClient, addr, postData, tu.StatusOK(c)), IsNil)
 	sc = &config.ServiceMiddlewareConfig{}
 	c.Assert(tu.ReadGetJSON(c, testDialClient, addr, sc), IsNil)
 	c.Assert(sc.EnableAudit, Equals, false)
+	c.Assert(sc.EnableRateLimit, Equals, false)


Could you please separate the test for rate-limit from audit?

Meanwhile, could you please add some tests of updating the item of limiter-config?

Meanwhile, could you please add some tests of updating the item of limiter-config?

Some tests will be added in #4843 after providering update API

Meanwhile, could you please add some tests of updating the item of limiter-config?

I add a simple test, more test will in #4843 by specific API

AndreMouche · 2022-06-07T02:54:07Z

server/api/service_middleware_test.go

 	}
 	postData, err = json.Marshal(ms)
 	c.Assert(err, IsNil)
 	c.Assert(tu.CheckPostJSON(testDialClient, addr, postData, tu.StatusOK(c)), IsNil)
 	sc = &config.ServiceMiddlewareConfig{}
 	c.Assert(tu.ReadGetJSON(c, testDialClient, addr, sc), IsNil)
 	c.Assert(sc.EnableAudit, Equals, false)
+	c.Assert(sc.EnableRateLimit, Equals, false)


Meanwhile, could you please add some tests of updating the item of limiter-config?

AndreMouche · 2022-06-07T03:10:53Z

server/server.go

+func (s *Server) SetRateLimitConfig(cfg config.RateLimitConfig) error {
+	old := s.serviceMiddlewarePersistOptions.GetRateLimitConfig()
+	s.serviceMiddlewarePersistOptions.SetRateLimitConfig(&cfg)
+	if err := s.serviceMiddlewarePersistOptions.Persist(s.storage); err != nil {


Oh, Should we make this function threadsafe? Was it possible that two goroutines run this function, for example, should we avoid the following case:

go1 finished L1001 and update the cfg to false

go2 finished 1001 and update the cfg to true in cache

go2 persist the value with true (from cache) and return success

go1 persist the value with true(from cache) while it expect the value is false.
and more different cases may meet in the Persist with it not protect while read and write..

This happens to all config that needs to Persist, and if optimization is needed, I think we can fix it together in another PR

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-13T03:34:59Z

tests/server/config/config_test.go

+
+func (s *testConfigPresistSuite) SetUpSuite(c *C) {
+	ctx, cancel := context.WithCancel(context.Background())
+	server.EnableZap = true


This config is no longer used.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-13T04:06:15Z

server/api/service_middleware_test.go

 	s.cleanup()
 }

-func (s *testServiceMiddlewareSuite) TestConfigAudit(c *C) {
+func (s *testAuditMiddlewareSuite) TestConfigAuditSwitch(c *C) {


Please add a test for the default value.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche

LGTM

nolouch · 2022-06-13T06:33:51Z

/merge

ti-chi-bot · 2022-06-13T06:33:52Z

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

/run-all-tests

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 · 2022-06-13T06:33:54Z

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

Commit hash: ed0bbf4

ti-chi-bot · 2022-06-13T07:24:44Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

close #4666, ref #4839, ref #4840, ref #4842, ref #4843 Add rate limit middleware for HTTP API. Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ref tikv#4666, ref tikv#4839, ref tikv#4869 update limiter config when reload presist config Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

close tikv#4666, ref tikv#4839, ref tikv#4840, ref tikv#4842, ref tikv#4843 Add rate limit middleware for HTTP API. Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> --wip-- [skip ci]

CabinfeverB added 5 commits April 24, 2022 16:45

add limit config

e8102b4

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add rate-limit config

b9bcfbf

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix test

1a93604

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

remove future code

cefebc9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add rate-limit config presist reload

826ef7f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 25, 2022

ti-chi-bot requested review from JmPotato and nolouch April 25, 2022 03:38

CabinfeverB added 3 commits April 25, 2022 15:18

add test

3318100

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix comment

4e2a840

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'limiter_config' into server_rate_limit

6d1c289

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 25, 2022

CabinfeverB changed the title ~~Server rate limit~~ server: update limiter config when reload presist config Apr 25, 2022

CabinfeverB marked this pull request as ready for review April 25, 2022 08:02

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2022

Merge branch 'master' into limiter_config

713c8a8

CabinfeverB mentioned this pull request Apr 25, 2022

server, api: add rate-limit middleware #4844

Merged

CabinfeverB added 2 commits April 26, 2022 01:28

fix typo

049c4d6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix typo

1b183a0

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2022

CabinfeverB added 4 commits April 28, 2022 14:14

add service config

900df00

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add service config

133ab15

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge service config

899e1f9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into service_config

71bbf8c

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2022

rleungx reviewed Jun 2, 2022

View reviewed changes

AndreMouche reviewed Jun 7, 2022

View reviewed changes

CabinfeverB added 2 commits June 13, 2022 11:25

fix test

a7612eb

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into server_rate_limit

a436333

rleungx reviewed Jun 13, 2022

View reviewed changes

CabinfeverB added 2 commits June 13, 2022 11:35

fix test

3366026

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix test

7b96ab5

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jun 13, 2022

View reviewed changes

CabinfeverB added 2 commits June 13, 2022 12:52

fix test

11680a2

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix test

ed0bbf4

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx approved these changes Jun 13, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 13, 2022

AndreMouche approved these changes Jun 13, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 13, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 13, 2022

ti-chi-bot added 3 commits June 13, 2022 14:34

Merge branch 'master' into server_rate_limit

f5288aa

Merge branch 'master' into server_rate_limit

8b341e1

Merge branch 'master' into server_rate_limit

12df5b3

ti-chi-bot merged commit 5a64486 into tikv:master Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add limiter config and reload mechanism #4842

server: add limiter config and reload mechanism #4842

CabinfeverB commented Apr 25, 2022 •

edited

Loading

ti-chi-bot commented Apr 25, 2022 •

edited

Loading

codecov bot commented Apr 25, 2022 •

edited

Loading

rleungx Jun 2, 2022

CabinfeverB Jun 2, 2022

rleungx Jun 2, 2022

CabinfeverB Jun 13, 2022

AndreMouche Jun 7, 2022

AndreMouche Jun 7, 2022

CabinfeverB Jun 13, 2022

CabinfeverB Jun 13, 2022

AndreMouche Jun 7, 2022

AndreMouche Jun 7, 2022

CabinfeverB Jun 13, 2022

rleungx Jun 13, 2022

rleungx Jun 13, 2022

CabinfeverB Jun 13, 2022

AndreMouche left a comment

nolouch commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

server: add limiter config and reload mechanism #4842

server: add limiter config and reload mechanism #4842

Conversation

CabinfeverB commented Apr 25, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 25, 2022 • edited Loading

codecov bot commented Apr 25, 2022 • 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

AndreMouche left a comment

Choose a reason for hiding this comment

nolouch commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

ti-chi-bot commented Jun 13, 2022

CabinfeverB commented Apr 25, 2022 •

edited

Loading

ti-chi-bot commented Apr 25, 2022 •

edited

Loading

codecov bot commented Apr 25, 2022 •

edited

Loading