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

cg2: Don't read cgroup.procs when deleting threaded cg #318

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

dcantah
Copy link
Member

@dcantah dcantah commented Dec 31, 2023

Reading cgroup.procs seems to return ENOTSUPP when threaded, so check the type of the cg when going to delete and read the relevant file.

Reading cgroup.procs seems to return ENOTSUPP when threaded, so check
the type of the cg when going to delete and read the relevant file.

Signed-off-by: Danny Canter <danny@dcantah.dev>
if len(processes) > 0 {
return fmt.Errorf("cgroups: unable to remove path %q: still contains running processes", c.path)
if len(tasks) > 0 {
return fmt.Errorf("cgroups: unable to remove path %q: still contains running tasks", c.path)
Copy link
Member

Choose a reason for hiding this comment

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

Curious why we don't try to remove and check for EBSUY instead?

Copy link
Member Author

@dcantah dcantah Jan 16, 2024

Choose a reason for hiding this comment

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

I think, and this is for what was here today, it's mainly for a nice error so I tried to keep in line with what was here already. I suppose we could just immediately remove(c.path) (which tries to handle a reasonable amount of ebusy looping) although afterwards we'd still need to know what file to read to surface some decent error if the remove didn't go through.

@dcantah
Copy link
Member Author

dcantah commented Jan 24, 2024

@dmcgowan This should be ready to check in, didn't realize I got two 😆

@fuweid fuweid merged commit d131035 into containerd:main Jan 24, 2024
8 checks passed
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.

4 participants