cgroup: add Snap/Cgroup monitoring capabilities#12231
cgroup: add Snap/Cgroup monitoring capabilities#12231sergio-costas wants to merge 32 commits intocanonical:masterfrom
Conversation
sergio-costas
changed the title
sergio-costas
force-pushed
the
DT-576-snapd-refresh-awareness-ux-implementation_add-file-monitoring-support
branch
2 times, most recently
from
5b4dd43 to
38c01ab
Compare
Meulengracht
left a comment
Meulengracht
left a comment
There was a problem hiding this comment.
Thank you, some comments
Meulengracht
added
the
Needs Samuele review
mardy
left a comment
mardy
left a comment
There was a problem hiding this comment.
I realized I commented on the inotify code, which you only imported. :-)
So I guess you can ignore these comments, or maybe address them in a separate commit, so we clearly distinguish what changes where made by us.
sandbox/cgroup/monitor.go
Outdated
There was a problem hiding this comment.
Maybe onFileAdded and onFileDeleted?
sergio-costas
requested review from
Meulengracht and
mardy
and removed request for
Meulengracht and
mardy
Meulengracht
left a comment
Meulengracht
left a comment
There was a problem hiding this comment.
Thank you for all the changes, I think you missed a few, and added some comments
|
@Meulengracht Done. About those two, I didn't miss them... I don't know why did they still appear to you. (maybe I fix them but forgot to do a push...?) |
|
@mardy All done. |
They still appear here in this PR because they are not changed here, if they appear different to you, you may have messed up your branch locally. Take a look at your branch online, it appears just as here (https://github.com/sergio-costas/snapd/blob/DT-576-snapd-refresh-awareness-ux-implementation_add-file-monitoring-support/sandbox/cgroup/monitor.go#L84) |
|
@Meulengracht This is what I see in your link, which is the same that I see in my local code...
You asked me to remove an "if newApp.npaths < 0" that was unneeded, and it isn't there... Or was there another change to do that I missed? |
Ah, I think we are talking past each other :-) I was talking about adding a return inside the |
pedronis
changed the title
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
did a pass, bunch of style comments and some questions, we value consistency and clarity quite a bit as snapd is a fairly large codebase
sergio-costas
force-pushed
the
DT-576-snapd-refresh-awareness-ux-implementation_add-file-monitoring-support
branch
2 times, most recently
from
e03a383 to
25ea530
Compare
This MR adds 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.
Set better names for the callbacks in the main loop
Co-authored-by: Alberto Mardegan <mardy@users.sourceforge.net>
Since it is a singleton, and the function is to get that singleton, it makes more sense to call it MonitorSingleton().
In the methods for the CGroupMonitor class, the object was being pased by copy instead of by reference. This MR fixes it.
The methods MonitorFiles() and NumberOfWaitingMonitors() are used only for tests, so it makes sense to have them in export_test.go instead of monitor.go.
As requested, removed the singleton from the monitor.
The "monitored" dictionary's key is the folder being monitored for the files/folders contained in each key.
Use a struct OPTIONS for the options passed to InstancePathsOfSnap. Also reverted the path->basepath change, since the path module is not needed anymore.
sergio-costas
force-pushed
the
DT-576-snapd-refresh-awareness-ux-implementation_add-file-monitoring-support
branch
from
549993e to
d7b189e
Compare
|
@mardy @pedronis @mvo5 @Meulengracht I wrote an alternative patch for the monitor itself, where I fully remove the singleton. It reuses the changes in cgroup/scanning.go, which were the most reviewed by you, and completely rewrite and simplify the cgroup/monitor.go file. Which one do you prefer? |
|
I marked this one as Draft because #12280 is a better implementation. |
|
Closing this too because all the work is being done in #12280 |
4 participants
This MR adds 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.
This MR and a future one overrides #12155 . The changes from that MR will be proposed in a step-by-step manner to simplify the revision process.
Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?