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

Add an e2e test to verify the --comm option works fine #212

Closed
hengyoush opened this issue Dec 23, 2024 · 10 comments · Fixed by #222
Closed

Add an e2e test to verify the --comm option works fine #212

hengyoush opened this issue Dec 23, 2024 · 10 comments · Fixed by #222
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed test

Comments

@hengyoush
Copy link
Owner

hengyoush commented Dec 23, 2024

Is your feature request related to a problem? Please describe.
Add an e2e test to verify the --comm (filter by process name) option works fine

Describe the solution you'd like

  1. Add a bash script file in testdata/ which contains the main test logic. (like this one: test_docker_filter_by_pid.sh), including two cases:

    1. target process start , then kyanos start , expected kyanos can watch the target process's network.
    2. kyanos start, then target process start, expected kyanos can watch the target process's network.
  2. Add a step in workflow : .github/workflows/test.yml which executes the test script file in 1.

@hengyoush hengyoush added help wanted Extra attention is needed good first issue Good for newcomers test labels Dec 23, 2024
@hengyoush hengyoush added this to v1.5.0 Dec 23, 2024
@hengyoush hengyoush moved this to Todo in v1.5.0 Dec 23, 2024
@hengyoush hengyoush self-assigned this Dec 25, 2024
@spencercjh
Copy link
Contributor

spencercjh commented Dec 27, 2024

Can a simple Python HTTP server be started directly (not in a Docker container) in this e2e test?

image
image

sudo ./kyanos watch -d http --comm python3

image

I need to simulate my local operation in the test.

AM I RIGHT?

@spencercjh
Copy link
Contributor

You haven't started this work yet, have you?

@hengyoush
Copy link
Owner Author

hengyoush commented Dec 27, 2024

You haven't started this work yet, have you?

No, I haven't.

Can a simple Python HTTP server be started directly (not in a Docker container) in this e2e test?

ok.

@spencercjh Thank you very much, assigned this issue to you.

@spencercjh
Copy link
Contributor

spencercjh commented Dec 30, 2024

HTTPS may require sending more than two requests over a long connection to be correctly captured 😂. Can this test be simplified by using curl to request any website (via http), and then using --comm curl? Finally check, when grepping, filter for the domain name (e.g., if curl requests baidu, grep for baidu.com).

What do you think? @spencercjh

I looked at the source code and the filtering of processes is done in SetupAgent -> LoadBPF -> setAndValidateParameters, and this call link is only executed once when the program starts. The range of processes filtered is only a snapshot of what is running at the time and does not dynamically select emerging processes as the program runs. This makes it difficult to use --comm to filter curl processes for #212.

It looks like --comm is not designed for one-off processes like curl but server proc. So the second test sub-task is unrealizable now.

kyanos start, then target process start, expected kyanos can watch the target process's network.

I've tested all of the above locally.

Also, the current implementation of --comm is not very user-friendly. It only supports filtering something like the output of ps —o comm, which I could extend to support ps —o cmd.

@hengyoush

@hengyoush
Copy link
Owner Author

hengyoush commented Dec 30, 2024

I looked at the source code and the filtering of processes is done in SetupAgent -> LoadBPF -> setAndValidateParameters, and this call link is only executed once when the program starts. The range of processes filtered is only a snapshot of what is running at the time and does not dynamically select emerging processes as the program runs. This makes it difficult to use --comm to filter curl processes for #212.

Actually, Kyanos knows when a process starts via eBPF tracepoint(sched_process_exec). After a process starts, it checks if the comm parameter matches the process name. If they match, it updates the filterPidMap in the kernel, bpf progs use filterPidMap to filter event by pid.

Look source code at: setAndValidateParameters(line: 292 ~ 309)

@spencercjh
Copy link
Contributor

I looked at the source code and the filtering of processes is done in SetupAgent -> LoadBPF -> setAndValidateParameters, and this call link is only executed once when the program starts. The range of processes filtered is only a snapshot of what is running at the time and does not dynamically select emerging processes as the program runs. This makes it difficult to use --comm to filter curl processes for #212.

Actually, Kyanos knows when a process starts via eBPF tracepoint(sched_process_exec). After a process starts, it checks if the comm parameter matches the process name. If they match, it updates the filterPidMap in the kernel, bpf progs use filterPidMap to filter event by pid.

Look source code at: setAndValidateParameters(line: 292 ~ 309)

Looks like something went wrong with my local test. I'll carry on.

@hengyoush
Copy link
Owner Author

Also, the current implementation of --comm is not very user-friendly. It only supports filtering something like the output of ps —o comm, which I could extend to support ps —o cmd.

Nice.

@spencercjh
Copy link
Contributor

I looked at the source code and the filtering of processes is done in SetupAgent -> LoadBPF -> setAndValidateParameters, and this call link is only executed once when the program starts. The range of processes filtered is only a snapshot of what is running at the time and does not dynamically select emerging processes as the program runs. This makes it difficult to use --comm to filter curl processes for #212.

Actually, Kyanos knows when a process starts via eBPF tracepoint(sched_process_exec). After a process starts, it checks if the comm parameter matches the process name. If they match, it updates the filterPidMap in the kernel, bpf progs use filterPidMap to filter event by pid.
Look source code at: setAndValidateParameters(line: 292 ~ 309)

Looks like something went wrong with my local test. I'll carry on.

The HTTPS protocol cannot be used when testing with curl. When using HTTP, we can see HTTP responses with 301.

@hengyoush
Copy link
Owner Author

hengyoush commented Dec 30, 2024

The first case "target process start, then kyanos start, expected kyanos can watch the target process's network." , you may not be able to use curl as the 'curl' process will terminate very quickly. Therefore, a server process might be needed for testing. @spencercjh

@spencercjh
Copy link
Contributor

The first case "target process start, then kyanos start, expected kyanos can watch the target process's network." , you may not be able to use curl as it will terminate very quickly. Therefore, a server process might be needed for testing. @spencercjh

Thanks for the explanation. 😭 🙏

@github-project-automation github-project-automation bot moved this from In Progress to Done in v1.5.0 Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed test
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants