Proposal UT_Init #1440
Proposal UT_Init #1440
Conversation
|
Welcome @wxnzb! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wxnzb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request initiates the process of adding comprehensive unit tests for key eBPF programs, specifically sendmsg.c, cgroup_skb.c, and cgroup_sock.c. It introduces a detailed testing proposal, integrates new test suites into the existing Go-based eBPF testing framework, and enhances the framework's capabilities by generalizing program loading and expanding support for various eBPF program types. The changes lay the groundwork for robust testing of these critical eBPF components.
Highlights
- Unit Test Framework Proposal: A new markdown document (
docs/proposal/ut_test.md) has been added, detailing a comprehensive plan for implementing unit tests forsendmsg.c,cgroup_skb.c, andcgroup_sock.cusing theunitTests_BUILD_CONTEXTframework. This proposal outlines specific testing methodologies, attachment points, and triggering mechanisms for each eBPF program. - Integration of New Test Suites: The
workload_test.gofile has been updated to include new test suites forsendmsg,cgroup_skb, andcgroup_sock, integrating them into the existing eBPF unit testing framework. - Generalized eBPF Program Loading: A new helper function,
load_bpf_prog_to_cgroup, has been introduced inworkload_test.go. This function generalizes the process of loading and attaching eBPF programs to cgroups by allowing the program name to be passed as a parameter, improving reusability and flexibility for cgroup-attached eBPF tests. - Expanded eBPF Program Type Support: The
loadAndPrepSpecfunction inbpf_test.gohas been modified to explicitly supportebpf.SkMsgandebpf.CGroupSockAddrprogram types, ensuring that eBPF programs of these types (like those insendmsg.candcgroup_sock.c) are not inadvertently skipped during test setup. - Initial Test Implementations: Basic test cases have been added for
sendmsg.c,cgroup_skb.c, andcgroup_sock.c, demonstrating the loading, attachment (for cgroup programs), and triggering mechanisms for these eBPF programs within the Go test environment.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a proposal and initial implementation for unit tests for sendmsg.c, cgroup_skb.c, and cgroup_sock.c. The changes include a design document, updates to the test framework to support new eBPF program types, and the test scaffolding itself.
My review focuses on improving the quality and reliability of the new tests. Key feedback includes:
- Replacing
time.Sleepwith more robust synchronization mechanisms to avoid flaky tests. - Adding assertions to the tests to actually verify BPF program behavior instead of just triggering them.
- Adhering to Go's idiomatic naming conventions for functions.
- Improving the clarity and correctness of the proposal document and test implementation details like providing descriptive test names.
Overall, this is a good start on adding important test coverage. Addressing the feedback will make the tests more robust and maintainable.
test/bpf_ut/bpftest/workload_test.go
Outdated
| t.Run(tt.objFilename, tt.run()) | ||
| } | ||
| } | ||
| func testCgroupSkb(t *testing.T) { |
There was a problem hiding this comment.
This test function, and testCgroupSock, currently only trigger the BPF programs but lack assertions to verify the behavior. Relying on manual log inspection via startLogReader is not sufficient for automated testing. Please add assertions that check BPF map states or other program outputs to validate correctness.
Additionally, the use of time.Sleep (line 847) can lead to flaky tests. It's better to use synchronization mechanisms like channels or wait groups to coordinate between the test's main thread and goroutines.
test/bpf_ut/bpftest/workload_test.go
Outdated
| t.Run(tt.objFilename, tt.run()) | ||
| } | ||
| } | ||
| func testCgroupSock(t *testing.T) { |
There was a problem hiding this comment.
This test function currently only triggers the BPF program but lacks assertions to verify the behavior. Relying on manual log inspection via startLogReader is not sufficient for automated testing. Please add assertions that check BPF map states or other program outputs to validate correctness.
Additionally, the use of time.Sleep (line 890) can lead to flaky tests. It's better to use more reliable synchronization mechanisms.
docs/proposal/ut_test.md
Outdated
| - 1 | ||
| - To achieve generality for mounting to cgroup, a proName parameter can be added to this function to make it more generic. | ||
| ``` | ||
| func load_bpf_2_cgroup(t *testing.T, objFilename string, cgroupPath string) (*ebpf.Collection, link.Link) { |
There was a problem hiding this comment.
The function name load_bpf_2_cgroup uses snake_case. The idiomatic way to name functions in Go is to use camelCase (e.g., loadBpfToCgroup). The same applies to the proposed new function load_bpf_prog_to_cgroup on line 157.
docs/proposal/ut_test.md
Outdated
| - Controls skipping of simulated connections. | ||
| - 4：is_managed_by_kmesh_skb(skb) == false | ||
| - Packets not managed by Kmesh should be skipped directly. | ||
| - 5：Packets not managed by Kmesh should be skipped directly. |
There was a problem hiding this comment.
This point appears to be a duplicate of the one on the preceding line. Please remove it to improve clarity.
docs/proposal/ut_test.md
Outdated
| t.Fatal("loading collection:", err) | ||
| } | ||
| } | ||
| spec.Programs["sockops_prog"].AttachType) |
There was a problem hiding this comment.
There appears to be a syntax error on this line with a misplaced closing parenthesis. Please correct the code snippet.
test/bpf_ut/bpftest/workload_test.go
Outdated
| objFilename: "workload_cgroup_sock_test.o", | ||
| uts: []unitTest_BUILD_CONTEXT{ | ||
| { | ||
| name: "", |
There was a problem hiding this comment.
The test case name is empty. Please provide a descriptive name that explains what this test case is supposed to verify.
| name: "", | |
| name: "load_and_trigger_cgroup_connect4_prog_success", |
test/bpf_ut/bpftest/workload_test.go
Outdated
| t.Run(tt.objFilename, tt.run()) | ||
| } | ||
| } | ||
| func load_bpf_prog_to_cgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { |
There was a problem hiding this comment.
In Go, function names are idiomatically written in camelCase (e.g., loadBpfProgToCgroup) rather than snake_case. Please rename this function to follow Go conventions.
Also, consider refactoring the existing load_bpf_2_cgroup function to use this more generic one, and then remove load_bpf_2_cgroup to avoid code duplication.
| func load_bpf_prog_to_cgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { | |
| func loadBpfProgToCgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { |
| @@ -0,0 +1 @@ | |||
| #include "workload/cgroup_skb.c" No newline at end of file | |||
There was a problem hiding this comment.
This file is missing a newline character at the end. It's a common convention and good practice to end files with a newline.
| #include "workload/cgroup_skb.c" | |
| #include "workload/cgroup_skb.c" | |
| @@ -0,0 +1 @@ | |||
| #include "workload/cgroup_sock.c" No newline at end of file | |||
There was a problem hiding this comment.
This file is missing a newline character at the end. It's a common convention and good practice to end files with a newline.
| #include "workload/cgroup_sock.c" | |
| #include "workload/cgroup_sock.c" | |
test/bpf_ut/workload_sendmsg_test.c
Outdated
| @@ -0,0 +1 @@ | |||
| #include "workload/sendmsg.c" No newline at end of file | |||
There was a problem hiding this comment.
This file is missing a newline character at the end. It's a common convention and good practice to end files with a newline.
| #include "workload/sendmsg.c" | |
| #include "workload/sendmsg.c" | |
docs/proposal/ut_test.md
Outdated
| @@ -0,0 +1,220 @@ | |||
| ### Proposed Implementation Plan | |||
There was a problem hiding this comment.
proposal refer to: https://github.com/kmesh-net/kmesh/blob/main/docs/proposal/template.md
There was a problem hiding this comment.
Okay, I will adjust this accordingly.
docs/proposal/ut_test.md
Outdated
| - Currently, we are still considering whether it is necessary to implement triggering only in one of these cases. | ||
| #### Triggering cgroup_sock.c | ||
| ``` | ||
| conn, err := net.Dial("tcp", "1.1.1.1:80") // 目标地址不重要 |
There was a problem hiding this comment.
Did you not adjust after copying and pasting?
docs/proposal/ut_test.md
Outdated
| } | ||
| ``` | ||
| - 2 | ||
| - Does this function need to be modified |
There was a problem hiding this comment.
Is this your question?
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Please update this PR and fix the errors. Then we can merge it |
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiZhenCheng9527 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 |
3 participants
What type of PR is this?
What this PR does / why we need it:
This PR adds unit tests for
sendmsg.c,cgroup_skb.c, andcgroup_sock.cusing theunitTests_BUILD_CONTEXTframework. It covers multiple test paths including map write validation and control path skipping.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?: