core: Allow preserving contents of RuntimeDirectory over process restart #6328

yuwata · 2017-07-11T04:56:15Z

This introduces RuntimeDirectoryPreserve= option which takes a boolean argument or restart.

Closes #6087.

boucman · 2017-07-11T12:47:25Z

man/systemd.exec.xml

+      <varlistentry>
+        <term><varname>RuntimeDirectoryPreserve=</varname></term>
+
+        <listitem><para>Takes one of <option>never</option>, <option>restart</option>, or <option>always</option>.


is it "no" or "never" or both ?

Only never. Current implimentation does not allow boolean.

this needs to be fixed here though, as you do suggest using "no" in the current version

boucman · 2017-07-11T12:48:40Z

man/systemd.exec.xml

+        are always removed when the service stops. If set to <option>restart</option> the directories are preserved
+        when the service is both automatically and manually restarted. Here, the automatic restart means the operation
+        specified in <varname>Restart=</varname>, and manual restart means the one triggered by
+        <command>systemctl restart foo.service</command>.


IIUC always means the directory is never removed, except on reboot (it's a tmpfs after all)

it might be worth saying that explicitely...

Oh, I forgot to mention it. Thanks.

poettering

Looks excellent already, just minor fixes, and please add the support for "systemd-run -p …" as described, thanks!

poettering · 2017-07-11T14:25:37Z

src/core/dbus-execute.c


                return 1;

+        } else if (streq(name, "RuntimeDirectoryPreserve")) {


hmm, could you also add the counterparts for this to src/shared/bus-unit-util.c's bus_append_unit_property_assignment()? We should really make sure that all new options come with that right-away (we are still missing some prop hookups for older props, and we should fix that too, but let's get this right right-away for the new ones).

Adding this means the new functionality can be tested easily using:

systemd-run -t -p RuntimeDirectory=foo -p RuntimeDirectoryPreserve=always /bin/sh

poettering · 2017-07-11T14:26:50Z

src/core/dbus-execute.c

+                if (mode != UNIT_CHECK) {
+                        c->runtime_directory_mode = m;
+
+                        unit_write_drop_in_private_format(u, mode, name, "RuntimeDirectoryMode=%u", m);


This should be output with %040o rather than %u, after all creation modes are usually written in octal style.

poettering · 2017-07-11T14:28:33Z

man/systemd.exec.xml

        <varname>$XDG_RUNTIME_DIR</varname> (for user services) when
-        the unit is started, and removed when the unit is stopped. The
+        the unit is started, and removed when the unit is stopped.
+        It is possible to preserve the direcotries if


direcotries → directories

poettering · 2017-07-11T14:29:12Z

man/systemd.exec.xml

+      <varlistentry>
+        <term><varname>RuntimeDirectoryPreserve=</varname></term>
+
+        <listitem><para>Takes one of <option>never</option>, <option>restart</option>, or <option>always</option>.


this needs to be fixed here though, as you do suggest using "no" in the current version

yuwata · 2017-07-13T02:11:38Z

@boucman @poettering Thank you for your comments. I've force-pushed the updated version. In the updated commits, I use a boolean argument rather than never and always.
Moreover, I've added several commits based on this functionality. If you feel they should be proposed in another PR, I will tentatively drop them.

fsateler · 2017-07-13T14:08:16Z

resolved fails to start:

Jul 13 07:47:30 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Trying to enqueue job systemd-resolved.service/start/replace
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Merged into installed job systemd-resolved.service/start as 1800
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Enqueued job systemd-resolved.service/start as 1800
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[7007]: systemd-resolved.service: Executing: /lib/systemd/systemd-resolved
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd-resolved[7007]: Fail to get current capabilities: Invalid argument
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Got notification message from PID 7007 (STOPPING=1, STATUS=Shutting down...)
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Child 7007 belongs to systemd-resolved.service
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Main process exited, code=exited, status=1/FAILURE
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Changed start -> failed
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: systemd-resolved.service: Job systemd-resolved.service/start finished, result=failed
Jul 13 07:47:31 autopkgtest-lxc-hckeil systemd[1]: Failed to start Network Name Resolution.

yuwata · 2017-07-13T14:32:38Z

@fsateler Thank you for testing my code. I have a question. In your testing environment, only resolved fails? networkd and timesyncd works well?

fsateler · 2017-07-13T14:34:08Z

@yuwata I did not test it myself, I'm just copying a snippet from he failed s390x autopkgtest. But it appears only resolved is failing

yuwata · 2017-07-13T14:37:51Z

@fsateler OK. I will read the log in detail. Recently in many PR the network related test have been failed on xenial-s390x. So I did not check the s390x log. Thanks.

yuwata · 2017-07-15T15:35:46Z

It seems that dropping privileges from networkd, resolved, and tymesyncd require more work. So, I will propose it in another PR. Now, the commits just add RuntimeDirectoryPreserve= and make RuntumeDirectory= support subdirectories.

poettering

looks excellent already, just one minor issue left

poettering · 2017-07-17T07:07:26Z

src/core/load-fragment.c

                }

-                if (!filename_is_valid(k)) {
+                if (!path_is_safe(k)) {


hmm, I think we should not permit absolute paths here, just to avoid confusion

I've add path_is_absolute() here in the revised version.

poettering · 2017-07-17T07:08:06Z

src/shared/bus-unit-util.c

+
+                r = sd_bus_message_append(m, "v", "u", mode);
+
+


spurious newline

…tart This introduces RuntimeDirectoryPreserve= option which takes a boolean argument or 'restart'. Closes systemd#6087.

yuwata · 2017-07-17T07:32:12Z

@poettering Thank you for your review. Updated.
In the updated version, first create their parents with mode 0755, and create the lowest subdirectories with the mode specified in RuntimeDirectoryMode=.

poettering · 2017-07-17T08:01:50Z

thanks! lgtm!

evverx · 2017-07-17T21:32:34Z

I've just filed #6391 about not calling chown when root is not assigned to User explicitly.

boucman reviewed Jul 11, 2017

View reviewed changes

poettering requested changes Jul 11, 2017

View reviewed changes

poettering added pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jul 11, 2017

yuwata force-pushed the runtime-preserve branch from acdbcde to cff52d4 Compare July 13, 2017 02:03

yuwata force-pushed the runtime-preserve branch from cff52d4 to f870d18 Compare July 13, 2017 07:24

yuwata force-pushed the runtime-preserve branch 3 times, most recently from b758b74 to 7030367 Compare July 15, 2017 15:28

poettering requested changes Jul 17, 2017

View reviewed changes

core: allow preserving contents of RuntimeDirectory= over process res…

53f47df

…tart This introduces RuntimeDirectoryPreserve= option which takes a boolean argument or 'restart'. Closes systemd#6087.

yuwata force-pushed the runtime-preserve branch from 7030367 to 41a0fb7 Compare July 17, 2017 07:23

core: support subdirectories in RuntimeDirectory= option

23a7448

yuwata force-pushed the runtime-preserve branch from 41a0fb7 to 23a7448 Compare July 17, 2017 07:31

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jul 17, 2017

poettering approved these changes Jul 17, 2017

View reviewed changes

poettering merged commit 7398320 into systemd:master Jul 17, 2017

yuwata mentioned this pull request Jul 17, 2017

core: add {State,Cache,Log,Configuration}Directory= #6384

Merged

yuwata mentioned this pull request Jul 18, 2017

Launch resolved & networkd unprivileged using RuntimeDirectory & Ambient capabilities #6393

Closed

yuwata deleted the runtime-preserve branch November 12, 2017 05:42


		return 1;

		} else if (streq(name, "RuntimeDirectoryPreserve")) {

Uh oh!

core: Allow preserving contents of RuntimeDirectory over process restart #6328

core: Allow preserving contents of RuntimeDirectory over process restart #6328

Uh oh!

Conversation

yuwata commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsateler commented Jul 13, 2017

Uh oh!

yuwata commented Jul 13, 2017

Uh oh!

fsateler commented Jul 13, 2017

Uh oh!

yuwata commented Jul 13, 2017

Uh oh!

yuwata commented Jul 15, 2017

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Jul 17, 2017

Uh oh!

poettering commented Jul 17, 2017

Uh oh!

evverx commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

yuwata commented Jul 11, 2017 •

edited

Loading

yuwata commented Jul 13, 2017 •

edited

Loading