Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Support for targeting systemd --user #1393

Merged
merged 2 commits into from
Jul 11, 2016

Conversation

hmalphettes
Copy link
Contributor

@hmalphettes hmalphettes commented Nov 23, 2015

Hi! I am looking at using fleet to manage a handful of machines with ssh access but without root access.
It might be a far shot or a misguided idea: please let me know if this is a dead-end.

If this is interesting I added 2 options to fleetd's configuration file to support that:

# fleetd.conf
units_dir=/home/vagrant/fleetunits
systemd_user=true

On archlinux, I can deploy example/hello.service but I get
AgentReconciler task failed: type=LoadUnit job=hello.service reason="unit scheduled here but not loaded" err=No such file or directory.
Some debugging shows that it comes from dbus's call to LinkUnitFiles; I have not figured out which folder it tries to link into yet.

Thanks for your attention.

@hmalphettes hmalphettes mentioned this pull request Nov 27, 2015
@jonboulle jonboulle added this to the v0.12.0 milestone Feb 10, 2016
@jonboulle jonboulle modified the milestones: v0.13.0, v0.12.0 Feb 10, 2016
@hmalphettes
Copy link
Contributor Author

Hi and thanks for your attention.
I found out systemd --user was disabled in centos/redhat-7.
It turns out that this feature might be refactored heavily by the systemd developers before it goes any further:
https://bugzilla.redhat.com/show_bug.cgi?id=1198655#c3

So maybe it is too early to work on this for fleet?

If you have further insights regarding the future of systemd --user I am interested.

@antrik
Copy link
Contributor

antrik commented Mar 21, 2016

@hmalphettes at a guess, your fleetd.conf might be the culprit: although I haven't tried following the code in question, I have serious doubts this can possibly work with a relative directory...

Aside from that, I have no idea whether the approach is fundamentally feasible.

@@ -81,6 +81,7 @@ func main() {
cfgset.String("metadata", "", "List of key-value metadata to assign to the fleet machine")
cfgset.String("agent_ttl", agent.DefaultTTL, "TTL in seconds of fleet machine state in etcd")
cfgset.String("units_directory", "/run/fleet/units/", "Path to the fleet units directory")
cfgset.Bool("systemd_system", true, "When false use systemd --user)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the other way around? I hope it's not just me -- but this looks kinda backwards...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! Thanks for the review!
Fixed here: ade6154
and edited the issue description too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could just squash this into the previous commit, if it's not too much trouble?... If we all agree it's better this way around, there is no need to have the extra churn in permanent history :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Done.

@jonboulle
Copy link
Contributor

It turns out that this feature might be refactored heavily by the systemd developers before it goes any further:

I actually like how simple/clean this patch is, but yeah, might be worth being a little more certain about the future of --user before committing to it. Could you ask systemd-devel upstream?

@hmalphettes
Copy link
Contributor Author

@jonboulle I did ask the systemd-devel upstream on bugzilla and here is the response from Lukáš Nykrýn:

We are not even sure that --user will stay in upstream as is in the future.

https://bugzilla.redhat.com/show_bug.cgi?id=1198655#c4

I'll try some other avenues to get more info then.

@antrik I did not take the time to test with an absolute path for the units_dir... yet.

@jonboulle
Copy link
Contributor

@hmalphettes I was actually referring to systemd-devel (or the GitHub project) - the bugzilla is just for RH, not really upstream.

@hmalphettes hmalphettes changed the title Support for targetting systemd --user Support for targeting systemd --user Mar 30, 2016
@hmalphettes
Copy link
Contributor Author

@jonboulle
Copy link
Contributor

Let's punt this until we see what they say.

@lemenkov
Copy link

Sorry for the noise - didn't get the subject of this thread properly :)

@hmalphettes
Copy link
Contributor Author

So far 2 answers on the mailing list of downstream adopters who are interested:
https://lists.freedesktop.org/archives/systemd-devel/2016-April/036169.html
https://lists.freedesktop.org/archives/systemd-devel/2016-April/036170.html

I also went on IRC to get this idea reviewed. None of the systemd-devs pitched in. Apologies, I can't find the logs.
2 persons answered. They basically stated that systemd --user was feature complete and they were surprised I asked about it.

I'll keep waving my arms and asking questions when the opportunity shows up.
Cheers!

@hmalphettes
Copy link
Contributor Author

@jonboulle I can't make it to Berlin for CoreOS Fest. But some of you will be there and Systemd's own Lennart will be too.
https://coreos.com/blog/coreos-fest-schedule.html

will it be possible to relay then question then?

@kayrus
Copy link
Contributor

kayrus commented Apr 14, 2016

IRC log

@dongsupark
Copy link
Contributor

+1 for this PR.
BTW functional/systemd_test.go needs to be also updated, as it doesn't build.

@tixxdz
Copy link
Contributor

tixxdz commented Apr 29, 2016

How about logind and maybe startups without user login loginctl enable-linger username ?

@hectorj2f
Copy link
Contributor

I strongly believe that using systemd --user could offer a better response time when starting units with fleet. The reason is that fleet always runs a systemd link operation for each unit, and this operation is quite expensive, performance and scalability wise. So, if systemd knows where the units are located, there is not need for such a link operation.

@dongsupark dongsupark force-pushed the master branch 2 times, most recently from 23d4b13 to 6fb1256 Compare July 1, 2016 11:07
@dongsupark
Copy link
Contributor

@hmalphettes gentle ping.
I think the only thing we need to do is updating a functional test, as I mentioned. After it's done, I'd be happily merging this PR.

@hmalphettes
Copy link
Contributor Author

@dongsupark thanks for the reminder and cheers to that!
I did not realise we were this close to merge it.

@hmalphettes hmalphettes force-pushed the systemd-user branch 2 times, most recently from c97499e to 711d073 Compare July 8, 2016 08:31
@@ -35,7 +35,7 @@ func TestSystemdUnitFlow(t *testing.T) {
}
defer os.RemoveAll(uDir)

mgr, err := systemd.NewSystemdUnitManager(uDir)
mgr, err := systemd.NewSystemdUnitManager(uDir, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dongsupark Fixed functional/systemd_test.go here.
Thanks again for the reminder!

@dongsupark
Copy link
Contributor

@hmalphettes Thanks. LGTM.
Though it's strange, semaphoreci isn't triggered automatically. Anyway I just ran the functional tests manually, and it's working.
One minor nit is that commit titles are not prefixed by subsystem names. But I'd say, that's no big deal, as this PR by itself is so simple as anyone can understand.
As a long-running PR is pain for everyone, I'd just merge this one next week, unless anyone has other opinion.
Thanks.

@hmalphettes
Copy link
Contributor Author

@dongsupark OK, I'll rewrite the commit messages.

@hmalphettes
Copy link
Contributor Author

hmalphettes commented Jul 9, 2016

@hmalphettes
Copy link
Contributor Author

@dongsupark One thing that worries me when reading this PR: the initial description suggests a value for the units_dir that is wrong: a relative path.

May I edit the initial description and replace units_dir=./fleetunits by units_dir=/home/vagrant/fleetunits ?

The default location of the units directory for fleet is owned by root.
When we use `systemd --user` we want this directory to be owned by
a different user.

Fixes coreos#1393
systemd offers users the ability to manage services under the user's
control with a per-user systemd instance.
This parameter enables fleet to target such instance of systemd.

Fixes coreos#1393
@dongsupark
Copy link
Contributor

@hmalphettes LGTM.
And sure, you can edit the initial description to replace a relative path with an absolute path.

@hmalphettes
Copy link
Contributor Author

@dongsupark ok. Edited. Cheers!

@dongsupark dongsupark merged commit 68566cf into coreos:master Jul 11, 2016
dongsupark pushed a commit that referenced this pull request Jul 11, 2016
Support for targeting `systemd --user`
@@ -64,6 +60,13 @@ func NewSystemdUnitManager(uDir string) (*systemdUnitManager, error) {
return &mgr, nil
}

func createDbusConnection(systemdUser bool) (*dbus.Conn, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indirection of this function seems unnecessary - I'd just inline the conditional into NewSystemdUnitManager

dongsupark pushed a commit to dongsupark/fleet that referenced this pull request Jul 14, 2016
Let's get dbus connection directly inside NewSystemdUnitManager,
instead of calling a separate function createDbusConnection().

Suggested by @jonboulle.
See coreos#1393 (comment)
dongsupark pushed a commit to dongsupark/fleet that referenced this pull request Jul 14, 2016
Let's get dbus connection directly inside NewSystemdUnitManager,
instead of calling a separate function createDbusConnection().

Suggested by @jonboulle.
See coreos#1393 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants