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

snapstate: Check the status of refresh-pending snaps #12155

Conversation

sergio-costas
Copy link
Contributor

@sergio-costas sergio-costas commented Sep 19, 2022

Every time snapd tries to refresh a snap and fails, sending to the user a message to close it, will monitor that snap to detect when it is closed, and issue a new refresh command in that precise moment.

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch 3 times, most recently from 0c01599 to 0800477 Compare September 19, 2022 11:34
@sergio-costas sergio-costas changed the title DT-576 Check the status of refresh-pending snaps snapstate: Check the status of refresh-pending snaps Sep 19, 2022
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch 2 times, most recently from ed5006d to fce6679 Compare September 19, 2022 13:45
@sergio-costas
Copy link
Contributor Author

sergio-costas commented Sep 20, 2022

The current code uses polling to detect when the process has ended, but it is only used while there is a pending refresh; when there are no snap refreshes blocked by a running process, it doesn't do polling, but waits to receive data from a Golang Channel.

I tried to use "wait4" ("waitpid" seems to not be available) to avoid polling, but since the snap's processes aren't a child of snapd, the call returns directly.

I could use inotify to detect when the cgroup files disappear without using polling, but that requires an external library because it isn't supported in "naive go"... is that OK?

Finally, to re-launch the refresh process I'm currently calling the shell "snap refresh XXXXX". I preferred to do it that way because I'm still not very familiar with the code. Also, since it is being called from a thread, I'm afraid of race conditions. Anyway, I'm checking the "doSnapAction" call.

@sergio-costas
Copy link
Contributor Author

doSnapAction isn't exported...

@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch 2 times, most recently from 7390066 to d9d8c9c Compare September 23, 2022 09:16
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch from d9d8c9c to 5bdbaeb Compare September 26, 2022 08:38
@sergio-costas
Copy link
Contributor Author

@pedronis Can you check again this?

@sergio-costas sergio-costas marked this pull request as ready for review September 29, 2022 14:19
@sergio-costas
Copy link
Contributor Author

@mvo5 I did the migration to a no-polling code using fsnotify (https://pkg.go.dev/gopkg.in/fsnotify/fsnotify.v1); but if you dislike that module (it's quite big), it is possible to just copy the two files of "inotify" (https://pkg.go.dev/k8s.io/utils/inotify) and have everything integrated.

@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch from f148d62 to 999bb16 Compare September 29, 2022 15:30
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch from c1e786b to 7ccf8e8 Compare September 29, 2022 17:35
@pedronis pedronis requested a review from mvo5 September 29, 2022 17:35
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch from 7ccf8e8 to da9d8a7 Compare September 29, 2022 22:58
@mvo5 mvo5 added the Skip spread Indicate that spread job should not run label Sep 30, 2022
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch 4 times, most recently from b26f54c to 5a27362 Compare October 5, 2022 08:55
@sergio-costas
Copy link
Contributor Author

Ok, I created #12231 , which contains only the Snap monitoring code, as requested. That should simplify the revision process. When that MR is approved and merged, I will include the second part.

@sergio-costas sergio-costas marked this pull request as draft October 5, 2022 09:33
This allow to not having to import UUID
Create and remove other folders in the folder being monitored
to ensure that it doesn't affect the monitoring.
InDeleteSelf isn't triggered in /sys/fs, so we must monitor
the parent folder.
Under some circumstances there can be a race condition when
closing the inotify object. This code uses the native os.File
functions to fix this.
If the main program stops reading from the Event channel and
closes the inotify object, the read gofunc can get blocked.

This patch fixes this by ensuring that it exits on close if
it is waiting to write an event to the Event or Error channels.
If two threads try to monitor a snap, and it's the first time
that MONITOR is called, there can be a race condition.

This patch ensures that inotify is initialized once.
The "unsafe" pointer seems pointless. Also, there is no way that
n is less than the size of an event, because, according to
inotify documentation, if the buffer is too small, zero will be
returned, and that's a case already being managed. Anyway, with
a buffer of 64KBytes there is always size for, at least, one
event (taking into account that NAME_MAX is 255 bytes in linux).
InDeleteSelf isn't triggered in /sys/fs
Now it shows a notification when the autorefresh begins, and
another one when the snap has completed the autorefresh.
@sergio-costas sergio-costas force-pushed the DT-576-snapd-refresh-awareness-ux-implementation branch from 24d36cd to 7a3d031 Compare November 11, 2022 17:23
@sergio-costas
Copy link
Contributor Author

Closing this because all the work is being done in other branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔ Blocked Skip spread Indicate that spread job should not run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants