8000 remove pid file on exit by geyslan · Pull Request #3075 · aquasecurity/tracee · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

remove pid file on exit #3075

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 9, 2023
Merged

remove pid file on exit #3075

merged 1 commit into from
May 9, 2023

Conversation

geyslan
Copy link
Member
@geyslan geyslan commented May 8, 2023

1. Explain what the PR does

tracee: remove pid file on exit

This also does some cosmetic changes to the code.

2. Explain how to test it

$ sudo ./dist/tracee --filter event=ptrace_code_injection
TIME             UID    COMM             PID     TID     RET              EVENT                     ARGS

then

ctrl+c

and watch:

$ find /tmp/tracee
/tmp/tracee
/tmp/tracee/out

3. Other comments

Fix: #3074

@geyslan geyslan added this to the v0.14.1 milestone May 8, 2023
@geyslan geyslan requested a review from rafaeldtinoco May 8, 2023 17:15
@geyslan geyslan self-assigned this May 8, 2023
@geyslan geyslan force-pushed the 3074-del-pid-file branch from 868c9f4 to ddf9f1d Compare May 8, 2023 17:18
@geyslan geyslan marked this pull request as ready for review May 8, 2023 17:18
geyslan
geyslan commented May 8, 2023
View reviewed changes
josedonizetti
josedonizetti suggested changes May 8, 2023
View reviewed changes
Copy link
Contributor
@josedonizetti josedonizetti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There is a problem of having the PID file internal to Tracee{} library, instead of part of the main.go, which should create the pid file as the first thing, and clean it up at the end of it.

For anything using the pid file as a metric to know if tracee is alive or not (tracee-action does it, and k8s files might do it in the future), cleaning inside the lib, means those services will think tracee is not live and either stop docker or restart the container, even though there is logic to be execute after t.Run() like drain the chan of events, for example.

So if possible, maybe we could fix this problem here? Ideally we should remove the pid creation from inside the lib, and have it under cmd/tracee.goas the first and last thing before the process exits.

@geyslan geyslan force-pushed the 3074-del-pid-file branch from d32d6c6 to 768b20d Compare May 8, 2023 18:57
@geyslan
Copy link
Member Author
geyslan commented May 8, 2023 •
edited
Loading

@josedonizetti this e5c1cea is a proposal based on your concerns. I couldn't extract it to main as the pidfile uses capture output path logic which is only set in Runner.Run(). But with that I was able to extract the creation and destruction of the pidfile to the highest execution path. WDYT?

@geyslan geyslan force-pushed the 3074-del-pid-file branch from 768b20d to e5c1cea Compare May 8, 2023 19:08
@yanivagman
Copy link
Collaborator

There is a problem of having the PID file internal to Tracee{} library, instead of part of the main.go, which should create the pid file as the first thing, and clean it up at the end of it.

For anything using the pid file as a metric to know if tracee is alive or not (tracee-action does it, and k8s files might do it in the future), cleaning inside the lib, means those services will think tracee is not live and either stop docker or restart the container, even though there is logic to be execute after t.Run() like drain the chan of events, for example.

So if possible, maybe we could fix this problem here? Ideally we should remove the pid creation from inside the lib, and have it under cmd/tracee.goas the first and last thing before the process exits.

I think this problem is bigger than what this PR comes to solve (#3074 (comment)) and we should probably tackle it later. In the future, I believe the liveness/readiness probes can be implemented using our API server, and not through this pid file.

@josedonizetti
Copy link
Contributor

@yanivagman Liveness/readiness will use the server, that is why we introduced the --healthz flag, tracee-action uses the pid, for starting for now, so removing is not a problem, though others could be using the pid, for the ideas mentioned here, or others ideas, and having its removal before the full stop of the flow might lead to misbehavior. The pid should not be tied to the internals of the lib, but to the start/exit of the process.

@rafaeldtinoco
Copy link
Contributor

Issue is being mitigated by: 6aee729 at #3076 so tests won't fail because of tracee.pid leftovers.

@yanivagman yanivagman removed this from the v0.14.1 milestone May 9, 2023
@geyslan geyslan requested a review from josedonizetti May 9, 2023 13:37
@geyslan
Copy link
Member Author
geyslan commented May 9, 2023

@josedonizetti if you agree with this solution let's merge it.

@geyslan geyslan force-pushed the 3074-del-pid-file branch 2 times, most recently from 70193b7 to eb0ed15 Compare May 9, 2023 14:12
geyslan
geyslan commented May 9, 2023
View reviewed changes
@geyslan
tracee: remove pid file on exit
ef59bb0 
This also does some cosmetic changes to the code and adds
utils.RemoveAt() wrapper.
@geyslan geyslan force-pushed the 3074-del-pid-file branch from eb0ed15 to ef59bb0 Compare May 9, 2023 16:02
@geyslan geyslan merged commit a8369af into aquasecurity:main May 9, 2023
@geyslan geyslan deleted the 3074-del-pid-file branch May 29, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josedonizetti josedonizetti josedonizetti approved these changes

@rafaeldtinoco rafaeldtinoco Awaiting requested review from rafaeldtinoco

Assignees

@geyslan geyslan

Labels
area/ebpf area/UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tracee.pid isn't being cleared from /tmp/tracee/out after ctrl+c (kernel v6.1)
4 participants
@geyslan @yanivagman @josedonizetti @rafaeldtinoco
0