Kmesh ads mode cluster add consistent hash lb #888

bfforever · 2024-09-23T16:42:05Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
proposal

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kmesh-bot · 2024-09-23T16:42:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kevin-wangzefeng for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

YaoZengzeng · 2024-09-25T01:54:46Z

pkg/consistenthash/maglev/maglev.go

@@ -0,0 +1,204 @@
+package maglev


Add copyright

YaoZengzeng · 2024-09-25T01:54:57Z

pkg/consistenthash/maglev/maglev_test.go

@@ -0,0 +1,176 @@
+package maglev


YaoZengzeng · 2024-09-25T01:58:41Z

pkg/consistenthash/maglev/maglev_test.go

+	"unsafe"
+
+	"github.com/cilium/ebpf"
+	"github.com/stretchr/testify/suite"


Add a blank line to separate Kmesh pkgs from other pkgs.

LiZhenCheng9527 · 2024-09-29T06:13:23Z

bpf/kmesh/ads/include/cluster.h

@@ -11,6 +11,8 @@
 #include "endpoint/endpoint.pb-c.h"

 #define CLUSTER_NAME_MAX_LEN BPF_DATA_MAX_LEN
+#define MAGLEV_TABLE_SIZE    16381


Why is table size set to this?

LiZhenCheng9527 · 2024-09-29T06:27:24Z

bpf/kmesh/ads/include/cluster.h

+        BPF_LOG(INFO, CLUSTER, "lb_policy is maglev, got a hash value:%u\n", hash);
+    }
+    index = hash % MAGLEV_TABLE_SIZE;
+    if (index >= MAGLEV_TABLE_SIZE)


Will index is greater than MAGLEV_TABLE_SIZE after the % operation?

LiZhenCheng9527 · 2024-09-29T06:28:22Z

bpf/kmesh/ads/include/cluster.h

+    if (index >= MAGLEV_TABLE_SIZE)
+        return NULL;
+    id = map_array_get_32(backend_ids, index, MAGLEV_TABLE_SIZE);
+    BPF_LOG(INFO, CLUSTER, "lb_policy is maglev, select backend id:%u\n", id);


nit: I think this log is unnecessary.

LiZhenCheng9527 · 2024-09-29T07:11:22Z

docs/proposal/consistent_hash.md

+
+##### maglev lookup table
+Use a compact table, table entry is bit width of endpoint count.
+For the table size default is 16381.


Is there a reference here?

LiZhenCheng9527 · 2024-09-29T07:18:18Z

pkg/consistenthash/maglev/maglev.go

+}
+
+func InitMaglevMap() error {
+	log.Println("InitMaglevMap")


nit: remove

supercharge-xsy · 2024-10-12T02:19:13Z

bpf/kmesh/ads/include/cluster.h

+    __uint(map_flags, BPF_F_NO_PREALLOC);
+    __array(values, struct inner_of_maglev);
+} outer_of_maglev SEC(".maps");
+


Is it necessary to use the outer-inner map structure here?

supercharge-xsy · 2024-10-12T02:32:59Z

bpf/kmesh/ads/include/route_config.h

+    if (!msg_header_len)
+        return hash;
+
+    BPF_LOG(INFO, ROUTER_CONFIG, "Got a header value len:%u\n", msg_header_len);


remove or change to debug level

supercharge-xsy · 2024-10-12T02:34:06Z

bpf/kmesh/ads/include/route_config.h

+    BPF_LOG(INFO, ROUTER_CONFIG, "Got a header value len:%u\n", msg_header_len);
+    if (!bpf_strncpy(msg_header_cp, msg_header_len, msg_header_v))
+        return hash;
+    BPF_LOG(INFO, ROUTER_CONFIG, "Got a header value:%s\n", msg_header_cp);


supercharge-xsy · 2024-10-12T02:34:29Z

bpf/kmesh/ads/include/route_config.h

+    if (!bpf_strncpy(msg_header_cp, msg_header_len, msg_header_v))
+        return hash;
+    BPF_LOG(INFO, ROUTER_CONFIG, "Got a header value:%s\n", msg_header_cp);
+    hash = 5318;


5318 mean ?

bfforever · 2024-10-12T06:29:10Z

pkg/consistenthash/maglev/maglev.go

+	MaglevOuterMapName         = "outer_of_maglev"
+	MaglevInnerMapName         = "inner_of_maglev"
+	MaglevMapMaxEntries        = 65536
+	ClusterNameMaxLen          = 192


keep consistent with ebpf c code

bfforever · 2024-10-12T07:01:47Z

pkg/consistenthash/maglev/maglev.go

+	return (b.offset + (b.skip * b.next)) % maglevTableSize
+}
+
+func getLookupTable(cluster *cluster_v2.Cluster, tableSize uint64) ([]int, error) {


add a source ref.

bfforever · 2024-10-12T07:05:30Z

bpf/kmesh/ads/include/cluster.h

+// 			    sk->protocol, HASH_INIT4_SEED);
+// }
+
+static __always_inline __u32 map_array_get_32(__u32 *array, __u32 index, const __u32 limit)


add a source ref.

supercharge-xsy · 2024-10-12T07:24:09Z

bpf/kmesh/ads/include/kmesh_common.h

+    __uint(value_size, sizeof(__u32));
+    __uint(max_entries, 1);
+} map_of_lb_hash SEC(".maps");
+


Add a describe comment to this map.

supercharge-xsy · 2024-10-12T07:25:51Z

bpf/kmesh/ads/include/route_config.h

+
+static inline int map_add_lb_hash(__u32 hash)
+{
+    int location = 0;


It seems that key conflicts exist. different sockets use the same key, and records may be overwritten.

kmesh-bot added the kind/feature label Sep 23, 2024

kmesh-bot requested review from kwb0523, nlgwcy and Okabe-Rintarou-0 September 23, 2024 16:42

kmesh-bot added the size/XXL label Sep 23, 2024

bfforever added 5 commits September 24, 2024 00:47

add consistent hash proposal

e4890d8

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

update consistant_hash proposal

ee6b8d7

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

update consistant_hash proposal

228c285

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

update consistant_hash proposal

49d827b

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

ads mode support consistent hash

ff0646b

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

bfforever force-pushed the lb_ospp branch from c82edc5 to ff0646b Compare September 23, 2024 16:51

ads mode support consistent hash

ad8047c

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

kmesh-bot added size/XL and removed size/XXL labels Sep 23, 2024

YaoZengzeng reviewed Sep 25, 2024

View reviewed changes

pkg/consistenthash/maglev/maglev.go

@@ -0,0 +1,204 @@

package maglev

Copy link

Member

YaoZengzeng Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add copyright

YaoZengzeng reviewed Sep 25, 2024

View reviewed changes

pkg/consistenthash/maglev/maglev_test.go

@@ -0,0 +1,176 @@

package maglev

Copy link

Member

YaoZengzeng Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

YaoZengzeng reviewed Sep 25, 2024

View reviewed changes

LiZhenCheng9527 reviewed Sep 29, 2024

View reviewed changes

supercharge-xsy reviewed Oct 12, 2024

View reviewed changes

bfforever commented Oct 12, 2024

View reviewed changes

supercharge-xsy reviewed Oct 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kmesh ads mode cluster add consistent hash lb #888

Kmesh ads mode cluster add consistent hash lb #888

bfforever commented Sep 23, 2024 •

edited

Loading

kmesh-bot commented Sep 23, 2024

YaoZengzeng Sep 25, 2024

YaoZengzeng Sep 25, 2024

YaoZengzeng Sep 25, 2024

LiZhenCheng9527 Sep 29, 2024

LiZhenCheng9527 Sep 29, 2024

LiZhenCheng9527 Sep 29, 2024

LiZhenCheng9527 Sep 29, 2024

LiZhenCheng9527 Sep 29, 2024

supercharge-xsy Oct 12, 2024

supercharge-xsy Oct 12, 2024

supercharge-xsy Oct 12, 2024

supercharge-xsy Oct 12, 2024

bfforever Oct 12, 2024

bfforever Oct 12, 2024

bfforever Oct 12, 2024

supercharge-xsy Oct 12, 2024

supercharge-xsy Oct 12, 2024

Kmesh ads mode cluster add consistent hash lb #888

Are you sure you want to change the base?

Kmesh ads mode cluster add consistent hash lb #888

Conversation

bfforever commented Sep 23, 2024 • edited Loading

kmesh-bot commented Sep 23, 2024

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

bfforever commented Sep 23, 2024 •

edited

Loading