Improve bpf log and bugs fix #364
Conversation
Signed-off-by: kangmingfa <1640528278@qq.com>
Codecov ReportAttention: Patch coverage is ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bfforever
force-pushed
the
main
branch
2 times, most recently
from
2ec65f5 to
28ab28f
Compare
| @@ -377,13 +377,13 @@ int route_config_manager(ctx_buff_t *ctx) | |||
|
|
|||
| virt_host = virtual_host_match(route_config, &addr, ctx); | |||
| if (!virt_host) { | |||
| BPF_LOG(ERR, ROUTER_CONFIG, "failed to match virtual host, addr=%u\n", addr.ipv4); | |||
| BPF_LOG(ERR, ROUTER_CONFIG, "failed to match virtual host, addr=%pI4h\n", &addr.ipv4); | |||
There was a problem hiding this comment.
Is it just easy to find the faulty object with address? Does need another identity?
There was a problem hiding this comment.
Maybe it could find which clusterIP service is failed when do virt host match. But this pr is just to change the ipaddr format of bpf log.
| @@ -72,7 +72,9 @@ func (c *Controller) Start() error { | |||
| } | |||
|
|
|||
| c.client = NewXdsClient(c.mode, c.bpfWorkloadObj) | |||
| c.client.workloadController.Processor.Sm = secertManager | |||
| if c.client.workloadController != nil { | |||
| c.client.workloadController.Processor.Sm = secertManager | |||
There was a problem hiding this comment.
The PR also contains a bug fix. Supplement the description in the PR title and description.
bfforever
changed the title
bfforever
changed the title
bpf/kmesh/ads/include/filter.h
Outdated
| @@ -199,7 +199,7 @@ int filter_chain_manager(ctx_buff_t *ctx) | |||
| /* filter match */ | |||
| ret = filter_chain_filter_match(filter_chain, &addr, ctx, &filter, &filter_idx); | |||
| if (ret != 0) { | |||
| BPF_LOG(ERR, FILTERCHAIN, "no match filter, addr=%u\n", addr.ipv4); | |||
| BPF_LOG(ERR, FILTERCHAIN, "no match filter, addr=%pI4\n", &addr.ipv4); | |||
bpf/include/bpf_log.h
Outdated
| reason is direct access would not be print ipaddr when pass `&ctx->remote_ipv4` to bpf_trace_printk */ | ||
| #define DECLARE_VAR_IPV4(t, ctx_ip, name) \ | ||
| __u32 name = 0; \ | ||
| if ((int)(BPF_LOGTYPE_##t) == BPF_DEBUG_ON) { \ |
There was a problem hiding this comment.
This macro seems not a must
There was a problem hiding this comment.
how to format ipv6 addr
It may need to define another macro to get ipv6, then pass ipv6 pointer to bpf_trace_printk using format%pI6c.
#https://www.kernel.org/doc/html/v4.20/core-api/printk-formats.html#ipv6-addresses
There was a problem hiding this comment.
This macro seems not a must
I consider using the macro to avoid defining ip variable when prog need to print the ipaddr info of bpf context, it could reduce code lines.
Also when one of ebpf prog's debug is off, it need not to get ip info from bpf context.
bpf/include/bpf_log.h
Outdated
| reason is direct access would not be print ipaddr when pass `&ctx->remote_ipv4` to bpf_trace_printk */ | ||
| #define DECLARE_VAR_IPV4(t, ctx_ip, name) \ | ||
| __u32 name = 0; \ | ||
| if ((int)(BPF_LOGTYPE_##t) == BPF_DEBUG_ON) { \ |
There was a problem hiding this comment.
can we remove this, imagine we may change the BPF_LOGTYPE_ from DEBUG to ERROR, what will happen if this is used by another caller for printing error
bfforever
force-pushed
the
main
branch
2 times, most recently
from
be69f03 to
11cef22
Compare
Signed-off-by: kangmingfa <1640528278@qq.com>
Signed-off-by: kangmingfa <1640528278@qq.com>
|
[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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Improve ipaddr format of bpf log.
And Fix xdsClient workloadController nil pointer bug when Kmesh mode is Ads.
Which issue(s) this PR fixes:
Fixes 358
Special notes for your reviewer:
In ads mode:
In workload mode:
Does this PR introduce a user-facing change?: