Support ads bpf map lookup all #827

Okabe-Rintarou-0 · 2024-09-08T14:45:25Z

What type of PR is this?
feature

What this PR does / why we need it:
support cluster lookup all for dump all the kv of bpf map.
It's for #820

i've met some problem in developing, seems to be related to the deserialization problem(#702 ) talked about before:

i update cluster 1 here, but the lookup and the newly written lookupall both got empty cluster name.

the output is:
lookup: cluster: api_status:UPDATE connect_timeout:1, api_status:UPDATE name:"ut-cluster-2" connect_timeout:1

ut-cluster-2 is shown, but ut-cluster-1 does not work.

If we only define Name field, the test will even panic.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

hzxuzhonghu · 2024-09-09T01:24:41Z

i've met some problem in developing, seems to be related to the deserialization problem talked about before:

How to reproduce? And the proto gen seems outdated, you can re run make gen

Okabe-Rintarou-0 · 2024-09-09T02:02:12Z

i've met some problem in developing, seems to be related to the deserialization problem talked about before:

How to reproduce? And the proto gen seems outdated, you can re run make gen

For the 1st question: you can checkout to my branch and run the newly added unit test TestClusterLookupAll.
For the 2nd question: It's because i did not format the c code.

Okabe-Rintarou-0 · 2024-09-09T02:12:45Z

FYI:
In this pr, i mainly add a new function to lookup all elements. it will return a linked list(since the length can be dynamic)

struct element_list_node* deserial_lookup_all_elems(const void* msg_desciptor) {
    int ret, err;
    struct element_list_node* value_list_head = NULL;
    const char* map_name = NULL;
    struct op_context context = {.inner_map_object = NULL};
    const ProtobufCMessageDescriptor* desc;
    struct bpf_map_info outter_info = {0}, inner_info = {0}, info = {0};
    int map_fd, outter_fd = 0, inner_fd = 0;
    unsigned int id, outter_id = 0, inner_id = 0;

    if (msg_desciptor == NULL)
        return NULL;

    desc = (ProtobufCMessageDescriptor*)msg_desciptor;
    if (desc->magic != PROTOBUF_C__MESSAGE_DESCRIPTOR_MAGIC)
        return NULL;

    map_name = desc->short_name;
    ret = get_map_ids(map_name, &id, &outter_id, &inner_id);
    if (ret)
        return NULL;

    ret = get_map_fd_info(id, &map_fd, &info);
    if (ret < 0) {
        LOG_ERR("invalid MAP_ID: %d\n", id);
        return NULL;
    }

    ret = get_map_fd_info(inner_id, &inner_fd, &inner_info);
    ret |= get_map_fd_info(outter_id, &outter_fd, &outter_info);
    if (ret < 0 || map_info_check(&outter_info, &inner_info))
        goto end;

    init_op_context(context, NULL, NULL, desc, outter_fd, map_fd, &outter_info,
                    &inner_info, &info);

    value_list_head = create_struct_list(&context, &err);
    if (err != 0) {
        LOG_ERR("create_struct_list failed, err = %d", err);
        deserial_free_elem_list(value_list_head);
        value_list_head = NULL;
    }

end:
    if (context.key != NULL)
        free(context.key);
    if (map_fd > 0)
        close(map_fd);
    if (outter_fd > 0)
        close(outter_fd);
    if (inner_fd > 0)
        close(inner_fd);
    return value_list_head;
}

LiZhenCheng9527 · 2024-09-09T08:21:22Z

pkg/cache/v2/maps/cluster.go

+
+	cMsg := C.deserial_lookup_all_elems(unsafe.Pointer(&C.cluster__cluster__descriptor))
+	if cMsg == nil {
+		return nil, fmt.Errorf("ClusterLookupAll deserial_lookup_all_elems failed")


Suggested change

return nil, fmt.Errorf("ClusterLookupAll deserial_lookup_all_elems failed")

return nil, fmt.Error("ClusterLookupAll deserial_lookup_all_elems failed")

LiZhenCheng9527 · 2024-09-09T08:23:15Z

pkg/cache/v2/maps/cluster.go

@@ -76,6 +76,35 @@ func ClusterLookup(key string, value *cluster_v2.Cluster) error {
 	return err
 }

+func ClusterLookupAll() ([]*cluster_v2.Cluster, error) {
+	var (
+		err      error


nit:No need to define variables in advance

codecov · 2024-09-09T13:03:44Z

Codecov Report

Attention: Patch coverage is 38.29787% with 58 lines in your changes missing coverage. Please review.

Project coverage is 53.67%. Comparing base (d5a4c00) to head (e3c37f8).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/cache/v2/maps/authz.go	0.00%	20 Missing ⚠️
pkg/cache/v2/maps/route.go	0.00%	20 Missing ⚠️
pkg/status/status_server.go	33.33%	5 Missing and 3 partials ⚠️
pkg/cache/v2/maps/cluster.go	80.00%	2 Missing and 2 partials ⚠️
pkg/cache/v2/maps/listener.go	80.00%	2 Missing and 2 partials ⚠️
pkg/bpf/bpf_kmesh_l4.go	0.00%	2 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/cache/v2/cluster.go	`38.88% <ø> (+4.57%)`	⬆️
pkg/cache/v2/listener.go	`58.73% <ø> (+9.39%)`	⬆️
pkg/cache/v2/route.go	`61.11% <ø> (+11.11%)`	⬆️
pkg/bpf/bpf_kmesh_l4.go	`0.00% <0.00%> (ø)`
pkg/cache/v2/maps/cluster.go	`60.52% <80.00%> (+6.95%)`	⬆️
pkg/cache/v2/maps/listener.go	`45.65% <80.00%> (+9.54%)`	⬆️
pkg/status/status_server.go	`42.30% <33.33%> (+4.11%)`	⬆️
pkg/cache/v2/maps/authz.go	`0.00% <0.00%> (ø)`
pkg/cache/v2/maps/route.go	`30.26% <0.00%> (-10.81%)`	⬇️

... and 6 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

hzxuzhonghu · 2024-09-10T08:56:05Z

We can implement for the workload map first, which is simpler

Okabe-Rintarou-0 · 2024-09-10T09:20:51Z

We can implement for the workload map first, which is simpler

yes, that can be done by using the bpf go user space method.

for the ads map, i've implement a common method for looking up all keys of a given bpf map, which will return a linked list. We can then use it to implement LookupAll function for all ads bpf map.

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

Okabe-Rintarou-0 · 2024-09-20T00:57:22Z

FInished all, cc @hzxuzhonghu @YaoZengzeng

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

hzxuzhonghu · 2024-09-20T04:11:27Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+#define PRINTF(fmt, args...)                                                                                           \
+    do {                                                                                                               \
+        printf(fmt, ##args);                                                                                           \
+        fflush(stdout);                                                                                                \


why do we need this

without fflush we cannot see the log.
the c printf uses a buffer.

I met this when i am debugging.

hzxuzhonghu

Generally lg, what does the output look like

Okabe-Rintarou-0 · 2024-09-20T09:29:57Z

Generally lg, what does the output look like

will add a ut

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

nlgwcy · 2024-09-22T03:43:12Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+    while (!bpf_map_get_next_key(ctx->curr_fd, prev_key, ctx->key)) {
+        prev_key = ctx->key;
+
+        value = create_struct(ctx, err);


When create_struct returns error, value may not be NULL. create_struct needs to be optimized. When an error is returned, the value is released in the function instead of depending on the caller. Currently, leakage may occur in create_struct_list.

nlgwcy · 2024-09-22T03:49:07Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+        value = create_struct(ctx, err);
+        if (*err != 0) {
+            LOG_ERR("create_struct failed, err = %d\n", err);
+            deserial_free_elem_list(elem_list_head);


When an error occurs, resources must be reclaimed and returned in a unified manner. The recommended format is as follows:

while (!bpf_map_get_next_key(ctx->curr_fd, prev_key, ctx->key)) { value = create_struct(ctx, err); if (*err != 0) break; ...... struct element_list_node *new_node = (struct element_list_node *)calloc(1, sizeof(struct element_list_node)); if (!new_node) { *err = -1; break; } ....... } if (*err) { deserial_free_elem_list(elem_list_head); return NULL; } return elem_list_head;

nlgwcy · 2024-09-22T03:52:58Z

bpf/deserialization_to_bpf_map/deserialization_to_bpf_map.c

+            deserial_free_elem_list(elem_list_head);
+            return NULL;
+        }
+        new_node->elem = value;


These are basic operations for updating list. It is recommended that you encapsulate them into independent functions or macros or call existing libraries.

thanks, will modify it.

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

nlgwcy · 2024-09-25T06:18:02Z

/lgtm

hzxuzhonghu

we should support dump this from kmeshctl now

kmesh-bot · 2024-09-25T06:58:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

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

kmesh-bot added the do-not-merge/work-in-progress label Sep 8, 2024

kmesh-bot requested review from nlgwcy and supercharge-xsy September 8, 2024 14:45

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

LiZhenCheng9527 reviewed Sep 9, 2024

View reviewed changes

kmesh-bot added size/L and removed size/XXL labels Sep 9, 2024

Okabe-Rintarou-0 changed the title ~~[WIP]support cluster lookup all~~ [WIP]support ads bpf map lookup all Sep 10, 2024

Okabe-Rintarou-0 added 2 commits September 19, 2024 21:48

support cluster lookup all

4247987

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

code format

e50a459

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

Okabe-Rintarou-0 force-pushed the feat/dump_bpf branch from 197164a to e50a459 Compare September 19, 2024 13:49

kmesh-bot added size/XXL and removed size/L labels Sep 19, 2024

Okabe-Rintarou-0 changed the title ~~[WIP]support ads bpf map lookup all~~ Support ads bpf map lookup all Sep 20, 2024

kmesh-bot added size/L and removed size/XXL do-not-merge/work-in-progress labels Sep 20, 2024

Okabe-Rintarou-0 force-pushed the feat/dump_bpf branch from a3db1a0 to e8c4aa2 Compare September 20, 2024 01:03

kmesh-bot added size/XXL and removed size/L labels Sep 20, 2024

feat: support all ads bpf lookupAll

2a341ae

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

Okabe-Rintarou-0 force-pushed the feat/dump_bpf branch from e8c4aa2 to 2a341ae Compare September 20, 2024 01:04

kmesh-bot removed the size/XXL label Sep 20, 2024

kmesh-bot added the size/L label Sep 20, 2024

hzxuzhonghu reviewed Sep 20, 2024

View reviewed changes

kmesh-bot added size/XL and removed size/L labels Sep 20, 2024

add ut

a531634

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

Okabe-Rintarou-0 force-pushed the feat/dump_bpf branch from 22db770 to a531634 Compare September 21, 2024 00:10

nlgwcy reviewed Sep 22, 2024

View reviewed changes

optimize create struct and create struct list

e3c37f8

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

kmesh-bot assigned nlgwcy Sep 25, 2024

kmesh-bot added the lgtm label Sep 25, 2024

hzxuzhonghu approved these changes Sep 25, 2024

View reviewed changes

kmesh-bot added the approved label Sep 25, 2024

kmesh-bot merged commit 48d3608 into kmesh-net:main Sep 25, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ads bpf map lookup all #827

Support ads bpf map lookup all #827

Okabe-Rintarou-0 commented Sep 8, 2024 •

edited

Loading

hzxuzhonghu commented Sep 9, 2024

Okabe-Rintarou-0 commented Sep 9, 2024 •

edited

Loading

Okabe-Rintarou-0 commented Sep 9, 2024 •

edited

Loading

LiZhenCheng9527 Sep 9, 2024

LiZhenCheng9527 Sep 9, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

hzxuzhonghu commented Sep 10, 2024

Okabe-Rintarou-0 commented Sep 10, 2024

Okabe-Rintarou-0 commented Sep 20, 2024

hzxuzhonghu Sep 20, 2024

Okabe-Rintarou-0 Sep 20, 2024

hzxuzhonghu left a comment

Okabe-Rintarou-0 commented Sep 20, 2024

nlgwcy Sep 22, 2024

nlgwcy Sep 22, 2024 •

edited

Loading

nlgwcy Sep 22, 2024

Okabe-Rintarou-0 Sep 22, 2024

nlgwcy commented Sep 25, 2024

hzxuzhonghu left a comment

kmesh-bot commented Sep 25, 2024

	return nil, fmt.Errorf("ClusterLookupAll deserial_lookup_all_elems failed")
	return nil, fmt.Error("ClusterLookupAll deserial_lookup_all_elems failed")

Support ads bpf map lookup all #827

Support ads bpf map lookup all #827

Conversation

Okabe-Rintarou-0 commented Sep 8, 2024 • edited Loading

hzxuzhonghu commented Sep 9, 2024

Okabe-Rintarou-0 commented Sep 9, 2024 • edited Loading

Okabe-Rintarou-0 commented Sep 9, 2024 • edited Loading

LiZhenCheng9527 Sep 9, 2024

Choose a reason for hiding this comment

LiZhenCheng9527 Sep 9, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

hzxuzhonghu commented Sep 10, 2024

Okabe-Rintarou-0 commented Sep 10, 2024

Okabe-Rintarou-0 commented Sep 20, 2024

hzxuzhonghu Sep 20, 2024

Choose a reason for hiding this comment

Okabe-Rintarou-0 Sep 20, 2024

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Okabe-Rintarou-0 commented Sep 20, 2024

nlgwcy Sep 22, 2024

Choose a reason for hiding this comment

nlgwcy Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

nlgwcy Sep 22, 2024

Choose a reason for hiding this comment

Okabe-Rintarou-0 Sep 22, 2024

Choose a reason for hiding this comment

nlgwcy commented Sep 25, 2024

hzxuzhonghu left a comment

Choose a reason for hiding this comment

kmesh-bot commented Sep 25, 2024

Okabe-Rintarou-0 commented Sep 8, 2024 •

edited

Loading

Okabe-Rintarou-0 commented Sep 9, 2024 •

edited

Loading

Okabe-Rintarou-0 commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

nlgwcy Sep 22, 2024 •

edited

Loading