-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Raw exec can use cgroups to manage PIDs #4350
Conversation
client/driver/executor/executor.go
Outdated
@@ -113,6 +113,11 @@ type ExecCommand struct { | |||
// ResourceLimits determines whether resource limits are enforced by the | |||
// executor. | |||
ResourceLimits bool | |||
|
|||
// Cgroup marks whether we put the process in a cgroup. Setting this field | |||
// doesn't enforce resource limits. To enforce limits, set ResoruceLimits. |
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.
typo - ResourceLimits
if err := manager.Apply(pid); err != nil { | ||
e.logger.Printf("[ERR] executor: error applying pid to cgroup subset %v: %v", e.resConCtx.cgPaths, err) | ||
return err | ||
} |
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.
This whole block looks super convoluted, is this the only way to do it though? not an expert here..
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.
Yeah it is! I did my best to comment every operation since it is confusing. The API of the library we use for cgroups kinda forces this complexity. Couldn't figure out another way to do it!
client/driver/raw_exec_test.go
Outdated
@@ -261,6 +265,92 @@ func TestRawExecDriver_Start_Kill_Wait(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestRawExecDriver_Start_Kill_Wait_Cgroup(t *testing.T) { |
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.
would add a comment that this test verifies that a raw exec task's process gets killed correctly when configured to use cgroup.
448ac9c
to
f47b78e
Compare
faf8c9f
to
fe18315
Compare
7f7a24d
to
103e867
Compare
103e867
to
657dd1e
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR makes it so that the raw exec driver uses cgroups to manage the
launched processes when on Linux and running as root. This can be disabled as
an option in the client map.
Fixes #4218 on Linux w/ cgroups and root