-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kmesh ads mode cluster add consistent hash lb #888
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: kangmingfa <[email protected]>
Signed-off-by: kangmingfa <[email protected]>
Signed-off-by: kangmingfa <[email protected]>
Signed-off-by: kangmingfa <[email protected]>
Signed-off-by: kangmingfa <[email protected]>
Signed-off-by: kangmingfa <[email protected]>
@@ -0,0 +1,204 @@ | |||
package maglev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add copyright
@@ -0,0 +1,176 @@ | |||
package maglev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
"unsafe" | ||
|
||
"github.com/cilium/ebpf" | ||
"github.com/stretchr/testify/suite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a blank line to separate Kmesh pkgs from other pkgs.
@@ -11,6 +11,8 @@ | |||
#include "endpoint/endpoint.pb-c.h" | |||
|
|||
#define CLUSTER_NAME_MAX_LEN BPF_DATA_MAX_LEN | |||
#define MAGLEV_TABLE_SIZE 16381 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is table size set to this?
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will index
is greater than MAGLEV_TABLE_SIZE
after the %
operation?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this log is unnecessary.
|
||
##### maglev lookup table | ||
Use a compact table, table entry is bit width of endpoint count. | ||
For the table size default is 16381. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference here?
} | ||
|
||
func InitMaglevMap() error { | ||
log.Println("InitMaglevMap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove
__uint(map_flags, BPF_F_NO_PREALLOC); | ||
__array(values, struct inner_of_maglev); | ||
} outer_of_maglev SEC(".maps"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use the outer-inner map structure here?
if (!msg_header_len) | ||
return hash; | ||
|
||
BPF_LOG(INFO, ROUTER_CONFIG, "Got a header value len:%u\n", msg_header_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove or change to debug level
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5318 mean ?
MaglevOuterMapName = "outer_of_maglev" | ||
MaglevInnerMapName = "inner_of_maglev" | ||
MaglevMapMaxEntries = 65536 | ||
ClusterNameMaxLen = 192 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep consistent with ebpf c code
return (b.offset + (b.skip * b.next)) % maglevTableSize | ||
} | ||
|
||
func getLookupTable(cluster *cluster_v2.Cluster, tableSize uint64) ([]int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a source ref.
// sk->protocol, HASH_INIT4_SEED); | ||
// } | ||
|
||
static __always_inline __u32 map_array_get_32(__u32 *array, __u32 index, const __u32 limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a source ref.
__uint(value_size, sizeof(__u32)); | ||
__uint(max_entries, 1); | ||
} map_of_lb_hash SEC(".maps"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a describe comment to this map.
|
||
static inline int map_add_lb_hash(__u32 hash) | ||
{ | ||
int location = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that key conflicts exist. different sockets use the same key, and records may be overwritten.
@@ -12,6 +12,7 @@ message Cluster { | |||
ROUND_ROBIN = 0; | |||
LEAST_REQUEST = 1; | |||
RANDOM = 3; | |||
MAGLEV = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why enum selects 5 can be commented here.
#### Tc level | ||
Do lb algorithm based on network packet or message, to change the daddr and dport of packet. | ||
<br> | ||
This way can get full ip address info to do lb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tc is defective. TC is packet-by-packet. At the tc level, each packet must be opened for modification. Whether the performance is acceptable and whether the modified records and source tracing are designed? Defects in the tc protocol should be documented here
{ | ||
__u32 datum = 0; | ||
|
||
// if (__builtin_constant_p(index) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code is not required, delete it.
* for this util function, so we never fail here, and returned datum is | ||
* always valid. | ||
*/ | ||
asm volatile("%[index] <<= 2\n\t" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of moving two left?
// sk->protocol, HASH_INIT4_SEED); | ||
// } | ||
|
||
static __always_inline __u32 map_array_get_32(__u32 *array, __u32 index, const __u32 limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used to obtain a u32 data from an array. The function name can be optimized.
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?: