-
Notifications
You must be signed in to change notification settings - Fork 240
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
Cgroup2: Cleanup procs in TestKill #286
Conversation
Just a small comment: Should we kill all managers on cleanup using manager.Kill? |
@manugupt1 I did a Kill on anything that actually launched any processes, I'd say it's case by case BUT we could add a test helper in a follow up here to check the number of processes running -> Call Kill if there is any -> Always call delete |
Oh okay! I was just wondering. |
I think it's a good idea! It's easy to forget to cleanup so if future authors see we always just call 'CleanupCgroup' or something similar the situation would be better I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@manugupt1 Oh woops, I just realized your comment for calling |
If the test fails before we get to calling manager.Kill we'll leak the 5 sleep processes. Signed-off-by: Danny Canter <danny@dcantah.dev>
Resolved merge conflicts from 55c197e |
If the test fails before we get to calling manager.Kill we'll leak the 5 sleep processes, and its harmless to call this
at the end of the test also. This additionally removes the cmd.Process nil check as the comment in the stdlib states that
cmd.Process will be filled in if Start returns successfully.