Skip to content

[WIP] set process group ID when fork/exec so child processes terminate together#695

Closed
mars1024 wants to merge 2 commits intocontainernetworking:masterfrom
mars1024:feat/kill_plugin_tree
Closed

[WIP] set process group ID when fork/exec so child processes terminate together#695
mars1024 wants to merge 2 commits intocontainernetworking:masterfrom
mars1024:feat/kill_plugin_tree

Conversation

@mars1024
Copy link
Member

@mars1024 mars1024 commented Aug 2, 2019

Provide a experimental way to fix #687

We let sub process called by invoke.Exec has the process group id same as its parent process id, so when the parent process is killed, all the sub processes will be killed in tree recursively.

Fox example, if we use libcni in CRI to call plugin A, and plugin A call plugin B and C, and plugin B call another plugin D, then the pid/pgid maybe like,

process pid pgid
CRI 100 120
A 103 100
B 105 103
C 106 103
D 108 105

If CRI (pid==100) was killed, processes(A pid==103) with group id == 100 will be killed, and processes(B pid==105 C pid==106) with group id == 103 will be killed, and process D will be killed in the same way recursively.
For the same reason , if CRI try to kill process A, B&C&D will be killed recursively.

Signed-off-by: Bruce Ma brucema19901024@gmail.com

@mars1024 mars1024 force-pushed the feat/kill_plugin_tree branch from f2e3efb to 62f2c79 Compare August 5, 2019 07:39
@mars1024
Copy link
Member Author

mars1024 commented Aug 5, 2019

I think test has been broken by #535 , maybe we need sudo now, do you have some suggestions? @dcbw

@dcbw
Copy link
Member

dcbw commented Aug 7, 2019

@mars1024 can you add the sudo back to travis.yaml and see what happens?

@rosenhouse
Copy link
Contributor

Could you cover this change with a test?

@bboreham
Copy link
Contributor

bboreham commented Aug 7, 2019

Code looks fine; could I ask that you squash the commits and make the title more like "Set the process group so child processes terminate together"? The current title is focused on the side-effect rather than the actual change.

…ate together

Signed-off-by: Bruce Ma <brucema19901024@gmail.com>
@mars1024 mars1024 force-pushed the feat/kill_plugin_tree branch from 62f2c79 to a6d08e8 Compare August 9, 2019 03:06
@mars1024
Copy link
Member Author

mars1024 commented Aug 9, 2019

@mars1024 can you add the sudo back to travis.yaml and see what happens?

OK, I'll try ~

@mars1024
Copy link
Member Author

mars1024 commented Aug 9, 2019

Could you cover this change with a test?

Sure, after CI pass, I'll update some tests.

@mars1024
Copy link
Member Author

mars1024 commented Aug 9, 2019

Code looks fine; could I ask that you squash the commits and make the title more like "Set the process group so child processes terminate together"? The current title is focused on the side-effect rather than the actual change.

Squash done, thanks ~

@bboreham bboreham changed the title invoke: support to terminate plugins recursively set process group ID when fork/exec so child processes terminate together Aug 14, 2019
@squeed
Copy link
Member

squeed commented Aug 14, 2019

Hmm, something is wrong - I can't run the tests locally either.

@mars1024
Copy link
Member Author

Hmm, something is wrong - I can't run the tests locally either.

Yes, me too, I'm trying to find more info about the broken CI.

@squeed
Copy link
Member

squeed commented Aug 15, 2019

Hmm, something is wrong - I can't run the tests locally either.

Yes, me too, I'm trying to find more info about the broken CI.

I checked out your PR and ran ./test_linux.sh and it failed - should be easier to debug.

Signed-off-by: Bruce Ma <brucema19901024@gmail.com>
@mars1024 mars1024 force-pushed the feat/kill_plugin_tree branch from a6d08e8 to 7cf0e0c Compare August 21, 2019 13:03
@mars1024
Copy link
Member Author

I tried, but It seems not work if we set pgid to os.GetPid(), otherwise killing process is different from killing process group, we can not expect os/exec in golang to help us kill process group although process group id is same as process id.

So I will change to another implementation based on killing process group later.

@mars1024
Copy link
Member Author

/hold

@mars1024 mars1024 changed the title set process group ID when fork/exec so child processes terminate together [WIP] set process group ID when fork/exec so child processes terminate together Aug 21, 2019
@bboreham
Copy link
Contributor

bboreham commented Mar 3, 2021

Hi, are you likely to come back to this?

@mars1024
Copy link
Member Author

mars1024 commented Mar 4, 2021

Hi, are you likely to come back to this?

I'll try but need some time to catch up..

@mars1024
Copy link
Member Author

/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libcni: terminate plugins on context timeout

5 participants