snapd icon indicating copy to clipboard operation
snapd copied to clipboard

many: implementation file monitoring for snap cgroups

Open sergio-costas opened this issue 2 years ago • 6 comments

cgroup: Add Snap/Cgroup monitoring capabilities (alternative MR)

This is an alternative MR for adding a Snap/Cgroup monitor that allows other parts of the code to wait until all the instances of a Snap have ended and it can be safely refreshed. It is the first step for a bigger MR that will allow to automagically refresh a snap after the system has notified the user that there is a refresh available and that they must close it.

sergio-costas avatar Oct 18 '22 14:10 sergio-costas

@mvo5 Ok, this should be ready for another round...

sergio-costas avatar Oct 19 '22 09:10 sergio-costas

@jhenstridge Ok, after testing the complete snapd I found (and remembered) why I used InDelete instead of InDeleteSelf: for some reason, InDeleteSelf isn't triggered in /sys/fs, so we must monitor the parent folder.

sergio-costas avatar Oct 20 '22 17:10 sergio-costas

@jhenstridge Ok, after testing the complete snapd I found (and remembered) why I used InDelete instead of InDeleteSelf: for some reason, InDeleteSelf isn't triggered in /sys/fs, so we must monitor the parent folder.

Looking at the cgroups(7) man page, it seems to suggest monitoring the cgroup.events file. If the file system behaves weirdly, then maybe it'd be worth structuring the code to use that logic. Something like:

  1. For each cgroup $dir, add a watch for $dir/cgroup.event for IM_MODIFY | IM_DELETE_SELF.
  2. When a notification is received for the watch, the cgroup should be considered gone if (a) the "populated" line in the file rrrreads 0, or (b) the file is not present.

jhenstridge avatar Oct 21 '22 07:10 jhenstridge

Looking at the cgroups(7) man page, it seems to suggest monitoring the cgroup.events file. If the file system behaves weirdly, then maybe it'd be worth structuring the code to use that logic. Something like:

The problem is that the cgroup.events file seems to be exclusive of cgroups V2...

sergio-costas avatar Oct 21 '22 08:10 sergio-costas

@jhenstridge Can you check if all is fine? It seems that there is a pending change request from you, but I can't find it...

sergio-costas avatar Oct 31 '22 11:10 sergio-costas

@jhenstridge Ok, changes done to the inotify part, and added an extra test for them. Ready for another review round.

sergio-costas avatar Nov 02 '22 12:11 sergio-costas

Squashed all the commits, as requested.

sergio-costas avatar Nov 30 '22 12:11 sergio-costas

Thanks for this. I did an incomplete pass and have some questions. I'll go over the parts I didn't read once I re-review

miguelpires avatar Dec 01 '22 18:12 miguelpires

Hey @sergio-costas, is this ready for a re-review? I see a few unanswered comments so I'm just checking before going through it again

miguelpires avatar Dec 05 '22 16:12 miguelpires

@MiguelPires Ops! I clearly forgot some of them. Give me some minutes to end another thing, and I will fix them.

sergio-costas avatar Dec 05 '22 16:12 sergio-costas

@MiguelPires Ops! I clearly forgot some of them. Give me some minutes to end another thing, and I will fix them.

No worries, take your time :slightly_smiling_face: I'll re-review once you're ready

miguelpires avatar Dec 05 '22 16:12 miguelpires

@MiguelPires Fixed the missing error check in AddWatcher.

sergio-costas avatar Dec 09 '22 10:12 sergio-costas