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

cgroup: Optionally add process and task to a subsystems subset #203

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

sameo
Copy link
Contributor

@sameo sameo commented Jul 23, 2021

In some cases, one may want to add processes and task only to certain
subsystems in a cgroup. For example, when a cgroup is created to only
limit a given subsystem usage, but leaves all other subsystems
unconstrained, there may be a need to move a process to only the
contrained cgroup subsystem while leaving it in another cgroup for
other subsystems.

This commit makes the Control interface process and task addition
functions variadic, in order to pass an optional list of subsystems that
behave as s subsystems filter.

Signed-off-by: Samuel Ortiz samuel.e.ortiz@protonmail.com

@fuweid
Copy link
Member

fuweid commented Jul 23, 2021

Need to rebase

@sameo sameo force-pushed the topic/subsystem-filter branch from 48526f0 to 2ea5283 Compare July 26, 2021 06:22
@sameo
Copy link
Contributor Author

sameo commented Jul 26, 2021

@fuweid The PR is rebased now.

@dmcgowan dmcgowan requested a review from fuweid September 20, 2021 17:43
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comments..

Does Update need to be have filters too?

// Add moves the provided process into the new cgroup
func (c *cgroup) Add(process Process) error {
return c.add(process, cgroupProcs)
func (c *cgroup) Add(process Process, subsystems ...Name) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we update the inline function descriptions too..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

control.go Outdated
// the cgroup subsystems. When giving AddProc a list of subsystem names, the process
// id is only added to those subsystems, provided that they are active in the targeted
// cgroup.
AddProc(pid uint64, subsystems ...Name) error
Copy link
Member

Choose a reason for hiding this comment

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

declaration isn't consistent with the other two.. as it includes subsystems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original AddProc declarartion was not consistent with the other methods as it had a named parameter (which is why I had to name the added one...). I'm fixing that too.

@@ -101,6 +101,50 @@ func TestAdd(t *testing.T) {
}
}

func TestAddFilteredSubsystems(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

let's also test against nil filteredSubstems..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

In some cases, one may want to add processes and task only to certain
subsystems in a cgroup. For example, when a cgroup is created to only
limit a given subsystem usage, but leaves all other subsystems
unconstrained, there may be a need to move a process to only the
contrained cgroup subsystem while leaving it in another cgroup for
other subsystems.

This commit makes the Control interface process and task addition
functions variadic, in order to pass an optional list of subsystems that
behave as s subsystems filter.

Signed-off-by: Samuel Ortiz <samuel.e.ortiz@protonmail.com>
@estesp
Copy link
Member

estesp commented Oct 7, 2021

hey @sameo, if you have a chance can you respond to @mikebrow's comments? Sorry for the slow review on this but I assume we can get it in if you are still looking to add this

@sameo
Copy link
Contributor Author

sameo commented Oct 7, 2021

Hey @estesp @mikebrow I'll be looking at this PR shortly, yes.

@sameo sameo force-pushed the topic/subsystem-filter branch from 2ea5283 to 80a7821 Compare October 7, 2021 16:28
@dmcgowan dmcgowan merged commit 9ada639 into containerd:main Oct 8, 2021
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.

7 participants