cgroup1 delete: proceed to the next subsystem when a cgroup is not found#296
Merged
estesp merged 2 commits intocontainerd:mainfrom Jun 20, 2023
Merged
cgroup1 delete: proceed to the next subsystem when a cgroup is not found#296estesp merged 2 commits intocontainerd:mainfrom
estesp merged 2 commits intocontainerd:mainfrom
Conversation
singholt
force-pushed
the
main
branch
2 times, most recently
from
54ad06b to
9e56b3e
Compare
The Delete method checks for running processes within each subsystem, before deleting the cgroup from it. On certain systems, there are cases when removing a cgroup from one subsystem, also removes it from others. For example, removing a cgroup from net_cls subsystem also removes the cgroup from net_prio. This is because both net_cls and net_prio have a symlink to 'net_cls,net_prio'. This is also true for cpu and cpuacct. This change handles the above case, when a query to get cgroup processes within a subsystem returns a fs.ErrNotExist. In such a case, we move to the next subsystem, instead of erroring out of Delete() immediately. This ensures that the cgroup is deleted from all subsystems that have it. Signed-off-by: Anuj Singh <[email protected]>
Signed-off-by: Anuj Singh <[email protected]>
singholt
changed the title
Contributor
Author
|
thanks for reviewing Phil! It looks like I may need another review, perhaps @kzys ? I'd appreciate if you could take a look 😄 |
dcantah
approved these changes
Jun 19, 2023
This was referenced Jun 29, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Anuj Singh [email protected]
The cgroup1 Delete method checks for running processes within each subsystem, before deleting the cgroup from it.
I noticed that there are cases when removing a cgroup from one subsystem, also removes it from others.
For example, on Amazon Linux (kernel 4.14), removing a cgroup from
net_clssubsystem also removes the cgroup fromnet_prio. This is because bothnet_clsandnet_priohave a symlink to a common dirnet_cls, net_prio. Similarly, removing a cgroup fromcpualso removes it from thecpuacctsubsystem, because of a symlink tocpu,cpuacct.In such a case the Delete() errors out with an error like
Sample code: https://gist.github.com/singholt/2a600802c0f74c8acdacd247760dc291 that fails on the Delete() without this fix but passes with it.
This change handles the above case, when a query to get cgroup processes within a subsystem returns a
fs.ErrNotExist. In such a case, we move to the next subsystem, instead of erroring out of the Delete() immediately. This ensures that the cgroup is deleted from all subsystems that have it.As per cgroups v1 documentation, since files in a cgroup directory cannot and need not be removed, it is safe to assume that if
cgroup.procsdoesn't exist for a cgroup (within a subsystem), the cgroup has been removed from the subsystem.This PR has two commits - first addressing the problem above, with the unit test updated and second, just fixing a typo.