client: implement qos based on leakybucket algorithm #268

wu-hanqing · 2021-02-26T07:09:12Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

wu-hanqing · 2021-02-26T07:57:06Z

recheck

wu-hanqing · 2021-02-28T04:36:51Z

recheck

wu-hanqing · 2021-02-28T05:09:12Z

recheck

wu-hanqing · 2021-02-28T05:12:31Z

recheck

wu-hanqing · 2021-02-28T08:49:06Z

recheck

wu-hanqing · 2021-02-28T09:29:47Z

recheck

wu-hanqing · 2021-02-28T12:36:12Z

recheck

wu-hanqing · 2021-03-01T02:09:21Z

recheck

wu-hanqing · 2021-03-01T08:29:51Z

recheck

wu-hanqing · 2021-03-02T01:46:10Z

recheck

wu-hanqing · 2021-03-02T08:31:16Z

recheck

ilixiaocui · 2021-03-03T02:51:15Z

conf/mds.conf

+#### throttle options ####
+#
+# iops
+mds.throttle.iopsMin=1800


Basis for the default value？

refer to the configuration of similar products of Ali Cloud and Tencent Cloud.

also, these configurations exceed the parameters of cinder.

ilixiaocui · 2021-03-03T02:54:29Z

curve-ansible/server.ini

@@ -40,6 +40,12 @@ topo_file_path=/etc/curve/topo.json
 target_leader_range=3
 check_leader_range_times=100
 check_leader_range_interval=10
+throttle_iops_min=1800


The default value can be set here： curve-ansible/roles/generate_config/templates/main.yml

ilixiaocui · 2021-03-03T03:22:31Z

proto/nameserver2.proto

+    BPS_WRITE = 5;
+}
+
+message ThrottleParams {


Why is it not directly included ThrottleType

ilixiaocui · 2021-03-03T03:24:33Z

src/client/client_common.h

@@ -72,6 +72,32 @@ enum class FileStatus {
    BeingCloned,
 };

+struct ThrottleParams {
+    uint64_t limit;


Note the meaning of these parameters：
limit, burst, burstLength

ilixiaocui · 2021-03-03T06:34:59Z

src/client/throttle.h

+
+    FileThrottleParams throttleParams_;
+    uint64_t enabledThrottleFlag_;
+    std::vector<std::pair<uint64_t, common::TokenBucketThrottle*>> throttles_;


why not shared_ptr?

no need to use shared_ptr, because those TokenBucketThrottle's lifetime is simple

ilixiaocui · 2021-03-03T06:39:52Z

src/client/throttle.h

+    void UpdateThrottleParams(const FileThrottleParams& params);
+
+ private:
+    void ApplyThrottleLimit(uint64_t flag, uint64_t limit, uint64_t burst,


ilixiaocui · 2021-03-03T06:55:15Z

src/common/throttle.h

+
+    void Get(uint64_t tokens);
+    bool Get(uint64_t tokens, google::protobuf::Closure* done);
+    int SetLimit(uint64_t average, uint64_t burst, uint64_t burstLength);


Why the previous parameters is limit instead of average?

Limit in the previous parameter represents a value, abut for a token bucket, it needs to have an average generation rate to reach this limit. So, name this parameter and member value as average.

ilixiaocui · 2021-03-03T06:55:49Z

src/common/throttle.h

+        return TokensFilled(currentTick_) - TokensFilled(currentTick_ - 1);
+    }
+
+ private:


Appropriate notes

ilixiaocui · 2021-03-03T06:58:25Z

src/common/throttle.h

+    ~TokenBucketThrottle();
+
+    void Get(uint64_t tokens);
+    bool Get(uint64_t tokens, google::protobuf::Closure* done);


When are the two get called？add notes please

ilixiaocui · 2021-03-03T07:02:29Z

test/common/throttle_test.cpp

@@ -0,0 +1,122 @@
+/*


src/client/throttle.h not tested?

add some test cases

xu-chaojie · 2021-03-05T07:14:20Z

src/client/io_tracker.cpp

+void IOTracker::DoRead(MDSClient* mdsclient, const FInfo_t* fileInfo,
+                       Throttle* throttle) {
+    if (throttle) {
+        throttle->Get(true, length_);


Will blocking here cause a deadlock ？

If tokens is not enough, this call will block until tokens is available. And, when close a file, in throttle's destructor it will let a blocked requests pass.

xu-chaojie · 2021-03-05T07:16:57Z

src/client/io_tracker.cpp

@@ -184,6 +192,10 @@ void IOTracker::DoWrite(MDSClient* mdsclient, const FInfo_t* fileInfo) {
            break;
    }

+    if (throttle) {
+        throttle->Get(false, length_);


Have you tested performance after throttle here？

Tested.
For IOPS throttle, the Get operation can exceed 100,000 qps.
and if disable all throttle, the performance is basically unchanged

xu-chaojie · 2021-03-05T07:24:46Z

src/common/math_util.h

@@ -37,6 +39,18 @@ inline uint64_t MaxPowerTimesLessEqualValue(uint64_t value) {
    return pow;
 }

+template <typename T, typename Compare>


xu-chaojie · 2021-03-22T07:39:48Z

src/common/throttle.h

+    void Get(uint64_t tokens);
+
+    /**
+     * @brief Get tokens, return false if the available token is not enough,


return True if the available token is not enough ?

xu-chaojie · 2021-03-22T08:16:39Z

src/common/throttle.h

+     *        and when the requirment is met, it will call done->Run()
+     * @return return true if got enough tokens, otherwise return false
+     */
+    bool Get(uint64_t tokens, google::protobuf::Closure* done);


If this function is only called in the class 'TokenBucketThrottle', it can be 'private'.

wu-hanqing force-pushed the feature/tokenbucket branch from 19953dd to c236667 Compare February 28, 2021 05:04

wu-hanqing force-pushed the feature/tokenbucket branch from c236667 to 49f958d Compare March 2, 2021 06:10

ilixiaocui reviewed Mar 3, 2021

View reviewed changes

xu-chaojie reviewed Mar 5, 2021

View reviewed changes

wu-hanqing force-pushed the feature/tokenbucket branch 2 times, most recently from 192dc9e to 32d8cd5 Compare March 9, 2021 06:53

xu-chaojie reviewed Mar 22, 2021

View reviewed changes

xu-chaojie reviewed Mar 23, 2021

View reviewed changes

wu-hanqing force-pushed the feature/tokenbucket branch 3 times, most recently from 9d63087 to 5636d22 Compare March 24, 2021 08:57

wu-hanqing changed the title ~~client: implement qos based on tokenbucket algorithm~~ client: implement qos based on leakybucket algorithm Mar 24, 2021

client: implement qos based on leakybucket algorithm

c4efa39

wu-hanqing force-pushed the feature/tokenbucket branch from 5636d22 to c4efa39 Compare March 24, 2021 09:12

xu-chaojie approved these changes Mar 25, 2021

View reviewed changes

xu-chaojie self-requested a review March 25, 2021 12:00

xu-chaojie approved these changes Mar 25, 2021

View reviewed changes

ilixiaocui approved these changes Mar 26, 2021

View reviewed changes

ilixiaocui merged commit ff8d9da into opencurve:master Mar 26, 2021

wu-hanqing deleted the feature/tokenbucket branch March 29, 2021 08:52

client: implement qos based on leakybucket algorithm #268

client: implement qos based on leakybucket algorithm #268

Conversation

wu-hanqing commented Feb 26, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

wu-hanqing commented Feb 26, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Feb 28, 2021

wu-hanqing commented Mar 1, 2021

wu-hanqing commented Mar 1, 2021

wu-hanqing commented Mar 2, 2021

wu-hanqing commented Mar 2, 2021

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

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