wrappers/internal: Add implicit graphical user daemons (fixed)#16601
wrappers/internal: Add implicit graphical user daemons (fixed)#16601sergio-costas wants to merge 3 commits intocanonical:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16601 +/- ##
=======================================
Coverage 77.52% 77.53%
=======================================
Files 1359 1348 -11
Lines 187225 187286 +61
Branches 2446 2446
=======================================
+ Hits 145152 145207 +55
- Misses 33298 33310 +12
+ Partials 8775 8769 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Wed Feb 18 14:23:19 UTC 2026 No spread failures reported |
adefde4 to
7526988
Compare
7f7fdf2 to
8f0ec17
Compare
|
It finally passes the spread test (although |
7c9aa01 to
085c60b
Compare
|
Fri Feb 27 10:27:18 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skip
|
ace1b8b to
9b95097
Compare
eaff17f to
ad8fdb7
Compare
|
Hi @sergio-costas , please mark this PR as "Ready for review" or ping me when this PR is ready for review. It would be great to get this landed for 2.75, which we're trying to cut around end of this week I believe. |
olivercalder
left a comment
There was a problem hiding this comment.
Code changes look reasonable, though I'm still a bit confused about the relationship between:
- what is done by systemd automatically
- what is done by snapd via
systemctl - what is done by snapd manually, and
- how the new changes fit into that
In particular, when/why would the service end up in the wrong target, when/why would the target change, what is responsible for setting up these symlinks in the usual case, and why do they need to be manually changed via this new code.
Other than questions, nothing obviously wrong I see. I left lots of nitpick comments about error messages and wrapping, but nothing major.
@bboozzoo should definitely take a look at this as well, as he worked on the existing userd code, and @Meulengracht should have a look because of the ongoing work on systemd relations, and @pedronis as well to make sure this is reasonable to him.
Thanks again @sergio-costas for working to get this fixed properly :)
| } else { | ||
| args = append(args, "--no-reload") | ||
| } | ||
| args = append(args, "reenable") |
There was a problem hiding this comment.
Do we need the --now flag to actually restart the services now? Otherwise they are re-enabled but not actually necessarily started, if I'm reading systemctl(1) correctly
There was a problem hiding this comment.
AFAIU, reenable doesn't restart the services, only updates the soft links...
There was a problem hiding this comment.
Right, my question is do we want to restart the services immediately, so should we add the --now flag as well?
| // will be wrong if a service is moved from default.target to | ||
| // graphical-session.target or vice-versa, so this clean up is required. | ||
|
|
||
| userSystemdQuery := path.Join(os.Getenv("HOME"), ".config", "systemd", "user", "*.target.wants") |
There was a problem hiding this comment.
I'm interested in where the source of truth lies for user services. The loop organization suggests that whatever is in $HOME/.config/systemd/user/ is correct, since we only look at symlinks in that directory. But then we maybe remove the existing symlinks directly and re-create them indirectly via systemctl.
So are we always sure that every user daemon service will have a symlink under one of the .target.wants directories, but that directory might just be the wrong one?
There was a problem hiding this comment.
That's a good point. Unfortunately, I haven't found a way of getting that info from systemctl, that's why I read it directly. But I prefer to make any change through the tools to ensure that I don't break anything: an error detecting something incorrect would, at most, result in disabling and re-enabling the service at login. Anyway, I think that I can add more tests, like ask systemctl if the user service is really enabled or not. What do you think?
There was a problem hiding this comment.
Hmmm, it's tough... what I'm worried about is a snap user daemon lacking any symlink at all under any target there, so being ignored by this function entirely. Not sure if that would be possible though.
I'd definitely prefer to use systemctl directly if possible, but get that it might not be possible.
Regarding the check you mentioned (ask systemctl whether the user service is really enabled), it seems that would be very important if:
- The user service was enabled under a particular target
- The snap refreshes and one of:
a. The user service no longer exists
b. The user service is now disabled but under a different target
And the converse would be:
- The user service is disabled or doesn't exist
- The snap refreshes and one of:
a. The user service now exists for the first time
b. The user service is now enabled but under a different target
So I think if a service becomes disabled, that's fine, because we see that the symlink is dead and we clean it up. But I'm not sure about the problem of service being previously disabled or nonexistent, would a symlink be created correctly in the user services directory under the new target?
| } | ||
| if oldKey != newKey { | ||
| // the daemon must be disabled and re-enabled to ensure that the | ||
| // .service file is placed in the right wanted-by folder |
There was a problem hiding this comment.
So when does this ensureSnapServiceSystemdUnits function get called? Is it part of the ensure loop? What happens in each situation:
- User is only signed in over ssh, no graphical session
- User is only signed in graphically
- User is only signed in over ssh, then logs in graphically as well
- User is only signed in graphically, then logs in over ssh as well
- User is not signed in at all
There was a problem hiding this comment.
AFAIK, it is called whenever a service is installed or refreshed, to update the corresponding .service files. In any of those cases, the .service file is checked for changes in the WantedBy section, and reenabled if there are changes to ensure that the global soft links are correct. The local soft links can only be modified by the user's daemon, so that part is checked during log-in.
There was a problem hiding this comment.
Maybe I should add a check to see if it is currently running, and do a reload in that case...
There was a problem hiding this comment.
Ah good, just running on refresh is much better than I was worried about. Did you add the check for if it is running? I would think if the snap is refreshed it would get reloaded as well automatically?
|
One other thought @sergio-costas, it may be easier to review and distinguish the changes if this were split into multiple PRs. One way to do so might be:
But I'm not sure which of these would come first. Splitting the PR up may make it easier to understand that, too :) |
|
At this stage we can't land this for 2.75 which is being cut next week, will not risk the release for this. |
I agree. I think it's a sensible decision to wait to next version. |
325d5d4 to
e5a9d99
Compare
sergio-costas
left a comment
There was a problem hiding this comment.
Added answers to comments.
d639dae to
fb1f3bf
Compare
|
@olivercalder Ok, I separated everything in three commits:
I also updated the commit messages to better explain what each commit does. I'll add some extra comments in some parts too. |
fb1f3bf to
35e38bb
Compare
olivercalder
left a comment
There was a problem hiding this comment.
Thanks for the changes! And splitting it up into three commits is great, much clearer. I'm curious whether it would be possible to split the commits into separate PRs, so it's easier to review and also easier to see the separation of concerns in the commit history. Would it be possible to do this?
| } else { | ||
| args = append(args, "--no-reload") | ||
| } | ||
| args = append(args, "reenable") |
There was a problem hiding this comment.
Right, my question is do we want to restart the services immediately, so should we add the --now flag as well?
usersession/agent/rest_api.go
Outdated
| // lacks a graphical system, but still wants to install a snap that has a daemon | ||
| // that is designed for it (for example, because it contains other tools that | ||
| // are useful in a text-only system; another case is for spread tests). | ||
|
|
There was a problem hiding this comment.
Empty line should be removed here
| // an user service that requires the `graphical-session` target, but we are in | ||
| // a non-graphical session, like the installer or an SSH session). In this | ||
| // case, we return nil to make snapd ignore this problem, assuming that the | ||
| // only reason why the service failed to launch is because of this. |
There was a problem hiding this comment.
This is perhaps a rather strong assumption, but there doesn't seem to be a way to know whether the service can launch successfully without trying, which requires the requisite graphical session.
I do think this is fine: snaps should be installable and the machine may never start a graphical session. And if it does, and the service fails, fine, that's only the case when the service is active. The snap can be installed and may have other apps which are useful when the graphical session is not active.
| } | ||
| if oldKey != newKey { | ||
| // the daemon must be disabled and re-enabled to ensure that the | ||
| // .service file is placed in the right wanted-by folder |
There was a problem hiding this comment.
Ah good, just running on refresh is much better than I was worried about. Did you add the check for if it is running? I would think if the snap is refreshed it would get reloaded as well automatically?
| // will be wrong if a service is moved from default.target to | ||
| // graphical-session.target or vice-versa, so this clean up is required. | ||
|
|
||
| userSystemdQuery := path.Join(os.Getenv("HOME"), ".config", "systemd", "user", "*.target.wants") |
There was a problem hiding this comment.
Hmmm, it's tough... what I'm worried about is a snap user daemon lacking any symlink at all under any target there, so being ignored by this function entirely. Not sure if that would be possible though.
I'd definitely prefer to use systemctl directly if possible, but get that it might not be possible.
Regarding the check you mentioned (ask systemctl whether the user service is really enabled), it seems that would be very important if:
- The user service was enabled under a particular target
- The snap refreshes and one of:
a. The user service no longer exists
b. The user service is now disabled but under a different target
And the converse would be:
- The user service is disabled or doesn't exist
- The snap refreshes and one of:
a. The user service now exists for the first time
b. The user service is now enabled but under a different target
So I think if a service becomes disabled, that's fine, because we see that the symlink is dead and we clean it up. But I'm not sure about the problem of service being previously disabled or nonexistent, would a symlink be created correctly in the user services directory under the new target?
| continue | ||
| } | ||
| for _, snapTarget := range snapTargetPaths { | ||
| // Remove any broken link (that's a service that was removed) |
There was a problem hiding this comment.
Hmm interesting... I wonder if we could run the cleanup when snaps are uninstalled too (and when refreshed)?
There was a problem hiding this comment.
There are no tests for these changes, we probable need some
There was a problem hiding this comment.
Yes, I want to add them, but they require the dbus-launch binary to be tested in CI et-al, so I pushed before the other patch.
About a snap lacking a symlink in the HOME folder... yes, it seems to happen, for example with prompting-client. I don't know why that one doesn't have it, but snapd-desktop-integration does.
There was a problem hiding this comment.
Also, I found that the snap userd daemon is launched only when some specific commands are executed (for example, a snap services) so... is there a way for the system's wide snapd to ask to all the user's snap daemon to do something?
They're all part of the same feature. Landing only one of the 3 MRs would be broken, so let's not do that. |
Are you sure? Wouldn't it be possible to land commits 2 and 3 separately to handle graphical user daemons with graphical-session target before we actually have any such daemons? Then landing commit 1 actually makes graphical user daemons with graphical-session target occur? |
uhh sure, that's fine |
I think any way we can simplify and break up this larger change will make it easier for reviewers to understand and quicker to get reviewed. So yes, please do try to make a separate PR for commit 2, separate PR for commit 3, and then a third PR for commit 1 based on 2 and 3. Sorry I'm being nitpicky about this :) I just do think it will make things easier and faster to land |
* daemon: Add implicit graphical user daemons This is an implicit implementation of graphical user daemons. Here, if an user-scoped daemon has the Wayland or the X11 plugs defined, it is presumed that it requires access to the desktop graphical environment, so it will depend on graphical-session.target instead of the default target. It implements the implicit way of SD-230. It also fixes all the problems with Gnome 50. This PR is an alternative to canonical#16091 * Add "s" (for seconds) in tests * Added test for desktop daemons * Fix typo * Added execute * Check that the test works * Update tests/lib/snaps/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update wrappers/internal/service_unit_gen.go Co-authored-by: Marco Trevisan <mail@3v1n0.net> * Added test for dbus daemons * Moved test into tests/main * Requested changes * Test to see if this is being run * Remove error * Update tests/main/test-user-daemon-in-graphical-environment/test-implicit-graphical-user-daemon/meta/snap.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/test-implicit-graphical-user-daemon/meta/snap.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/test-implicit-graphical-user-daemon/meta/snap.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/test-implicit-graphical-user-daemon/meta/snap.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Added systems * Moved the snap * Fix snaps-state * Fix paths in task.yaml * Set daemon experimental flag * Fixed test in dbus services * Update tests/main/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/test-user-daemon-in-graphical-environment/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Extra required changes * Update tests/lib/snaps/test-implicit-graphical-user-daemon/meta/snap.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> * Update tests/main/implicit-graphical-user-daemon/task.yaml Co-authored-by: Oliver Calder <oliver@calder.dev> --------- Co-authored-by: Oliver Calder <oliver@calder.dev> Co-authored-by: Marco Trevisan <mail@3v1n0.net> Always use PartOf instead of BindsTo Added extra tests The new Asserts allow to check that both scoped and unscoped plugs are detected.
If the `WantedBy` entry in the `Install` section of a .service file changes from `default` to `graphical-session`, the soft link at `/etc/systemd/user/default.target.wants` must be deleted and a new one at `/etc/systemd/user/graphical-session.target.wants` must be created for those user daemons that have changed. But this must be done only to those daemons that are enabled, to avoid enabling daemons that the user manually disabled. This can happen in two cases: * snapd is updated from the old versions that didn't support `graphical-session` target to a new version that supports it, so all the currently user services already installed will have their .service files updated with the new WantedBy entry. * a snap with an user service is updated, and the new version have added or removed the `desktop` plug in any of the user services. In both cases, a `systemctl --user reenable SERVICE-NAME` must be run to update the symlinks at `/etc/systemd/user/default.target.wants` and `/etc/systemd/user/graphical-session.target.wants` folders, but only if it is globally enabled. That command removes the symlinks at the wrong places and re-creates them in the right places. This patch does that, but only for globally-enabled (preset) user daemons. Locally-enabled daemons (thus, with their symlinks placed at $HOME/.config/systemd/user/XXXXX.target.wants`) aren't modified. That is a task for the next commit.
In some cases, snapd insists on enabling user services not only globally, as expected (thus, adding a symlink at `/etc/systemd/user/XXXXX.target.wants`), but also locally (thus, at $HOME/.config/systemd/user/XXXX.target.wants`). This means that if an user service is migrated from `default.target` to `graphical-session.target` (or vice-versa), the soft link at the user's HOME folder will be in the wrong subfolder. Unfortunately, the main snapd service seems to not have the capability of accessing and managing the local user services, having to rely on the user's local daemon (`snap userd`), which runs as an user's service. So a change in that daemon (which has its source code at `usersession/userd` folder) is required to check the locally enabled user services and ensure that their soft link is in the right place. This commit does this by checking the links in the user folder every time user session daemon is launched, and runs as the user a `systemctl --user reenable SNAP-SERVICE-NAME` to rebuild the wrong softlink.
35e38bb to
c900abe
Compare
|
@olivercalder Ok, three PRs: #16685 , #16686 and #16687 . They must be merged in that order (specified on the first line of the description). Is there something I can do to ensure that they can only be merged that way, other than mark the second and third as |
This is a (fixed) implicit implementation of graphical user daemons. Here, if an user-scoped daemon has the desktop plug defined (explicitly or implicitly), it is presumed that it requires access to the desktop graphical environment, so it will depend on graphical-session.target instead of the default target.
Implements the implicit way of the specification SD-230.
This new PR fixes the problem when using BindsTo, preventing Gnome Shell 50 to launch.
Also fixes the problem with the
XXX.wantssymlink: now, if an user daemon is globally enabled and itsWantedByentry changes, it will be disabled and re-enabled to ensure that the symlink is located in the right place.It also checks during install if an user daemon fails to launch. In that case, it checks the WantedBy entry to check the target, and if it is different than
default, it will check if that target is currently active. If it isn't, it will presume that that is the reason for the failure, and continue the install. This fixes the Spread tests.Finally, it fixes the soft links in the
$HOME/.config/systemd/user/XXXXX.target.wantswhen a service is moved into a differentWantedBy.Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?