Skip to content
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

Make use of global variables instead of bpf config map #1263

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hzxuzhonghu
Copy link
Member

What type of PR is this?

/kind enhancement

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@kmesh-bot
Copy link
Collaborator

[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 ask for approval from hzxuzhonghu. 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:

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

@hzxuzhonghu hzxuzhonghu requested a review from Copilot March 4, 2025 11:31
@kmesh-bot kmesh-bot added size/XXL and removed size/XL labels Mar 4, 2025

Choose a reason for hiding this comment

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

PR Overview

This PR implements an enhancement by replacing the use of a BPF config map with global variables for several configuration parameters. The changes involve removing the obsolete km_configmap and associated Kmesh*KmeshConfig structs across multiple files while adding new variable specifications for BpfLogLevel, EnableMonitoring, NodeIp, and PodGateway.

Reviewed Changes

File Description
bpf/kmesh/bpf2go/dualengine/kmeshsockopsworkload_bpfeb.go Removed km_configmap references and KmeshSockopsWorkloadKmeshConfig; added variable specs for global configuration.
bpf/kmesh/bpf2go/dualengine/kmeshsockopsworkload_bpfel.go Removed km_configmap references and KmeshSockopsWorkloadKmeshConfig; added global variables for configuration.
bpf/kmesh/bpf2go/dualengine/kmeshsockopsworkloadcompat_bpfeb.go Removed km_configmap from maps and KmeshSockopsWorkloadCompatKmeshConfig as part of the config map removal.
bpf/kmesh/bpf2go/dualengine/kmeshsockopsworkloadcompat_bpfel.go Removed km_configmap and updated variable specs accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshcgroupsockworkloadcompat_bpfeb.go Removed km_configmap from maps and KmeshCgroupSockWorkloadCompatKmeshConfig.
bpf/kmesh/bpf2go/dualengine/kmeshcgroupsockworkloadcompat_bpfel.go Removed km_configmap and updated variable specs accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshcgroupsockworkload_bpfeb.go Removed km_configmap and KmeshCgroupSockWorkloadKmeshConfig in favor of global variables.
bpf/kmesh/bpf2go/dualengine/kmeshcgroupsockworkload_bpfel.go Removed km_configmap and updated the close logic to match the new configuration style.
bpf/kmesh/bpf2go/dualengine/kmeshxdpauthcompat_bpfeb.go Removed km_configmap and updated variable specs for authentication configuration.
bpf/kmesh/bpf2go/dualengine/kmeshxdpauth_bpfeb.go Removed km_configmap from maps and updated variable specs accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshxdpauthcompat_bpfel.go Removed km_configmap and updated variable specs for consistency.
bpf/kmesh/bpf2go/dualengine/kmeshxdpauth_bpfel.go Removed km_configmap from maps and updated variable specs accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshsendmsgcompat_bpfeb.go Removed km_configmap from maps and updated close logic accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshsendmsg_bpfel.go Removed km_configmap from maps and updated close logic accordingly.
bpf/kmesh/bpf2go/general/kmeshtcmarkdecrypt_bpfel.go Removed km_configmap from maps and KmeshTcMarkDecryptKmeshConfig in favor of global variables.
bpf/kmesh/bpf2go/general/kmeshtcmarkdecrypt_bpfeb.go Removed km_configmap from maps and updated configuration accordingly.
bpf/kmesh/bpf2go/dualengine/kmeshsendmsg_bpfeb.go Removed km_configmap from maps and KmeshSendmsgKmeshConfig in favor of global variables.
bpf/kmesh/bpf2go/dualengine/kmeshsendmsgcompat_bpfel.go Removed km_configmap from maps and updated close logic accordingly.

Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.

@LiZhenCheng9527 LiZhenCheng9527 added this to the v1.1 milestone Mar 5, 2025
@hzxuzhonghu
Copy link
Member Author

Let me fix the TestServerAccesslogHandler panic

var BpfLogLevel uint32
if err := l.BpfLogLevel.Get(&BpfLogLevel); err != nil {
log.Errorf("get BpfLogLevel failed %v", err)
return 0
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to distinguish between disable and error?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, Variable.Get will not fail.

Whatever, will propagate the error back in a following pr

var AuthzOffload uint32
if err := l.AuthzOffload.Get(&AuthzOffload); err != nil {
log.Errorf("get AuthzOffload failed %v", err)
return 0
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 40.66667% with 89 lines in your changes missing coverage. Please review.

Project coverage is 44.88%. Comparing base (eeeb399) to head (65bd807).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
pkg/bpf/bpf.go 40.17% 52 Missing and 15 partials ⚠️
pkg/status/status_server.go 19.04% 16 Missing and 1 partial ⚠️
pkg/controller/controller.go 0.00% 5 Missing ⚠️

❌ Your patch check has failed because the patch coverage (40.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines Coverage Δ
pkg/bpf/ads/loader.go 32.95% <100.00%> (ø)
pkg/bpf/workload/loader.go 25.86% <100.00%> (+2.64%) ⬆️
pkg/utils/test/bpf_map.go 46.55% <ø> (-0.91%) ⬇️
pkg/controller/controller.go 0.00% <0.00%> (ø)
pkg/status/status_server.go 29.73% <19.04%> (+1.37%) ⬆️
pkg/bpf/bpf.go 43.66% <40.17%> (+1.61%) ⬆️

... and 5 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 f839eff...65bd807. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Zhonghu Xu <[email protected]>
Signed-off-by: Zhonghu Xu <[email protected]>
Signed-off-by: Zhonghu Xu <[email protected]>
Signed-off-by: Zhonghu Xu <[email protected]>
@hzxuzhonghu
Copy link
Member Author

@LiZhenCheng9527 @nlgwcy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants