rework unit dependency data structure to track why deps get created #7186

poettering · 2017-10-26T15:53:13Z

This is a first step towards "pin-pointed" reloads, among other things.

Also contains a number of other, minor fixes.

sourcejedi · 2017-10-31T10:39:43Z

There should be a way to turn this logic of, and DefaultDependencies=
appears to be the right option for that, hence let's downgrade this
dependency type from "implicit" to "default, and thus honour
DefaultDependencies=.

A follow-up for #7076.

tmp.mount has DefaultDependencies=no (and #7076 removed the explicit dependency on swap.target)

poettering · 2017-11-09T14:49:56Z

@sourcejedi indeed! thanks for the pointer. force pushed a new version now, that readds the After=swap.target for tmp.mount.

sourcejedi · 2017-11-09T17:41:16Z

src/core/unit.c

@@ -935,12 +980,12 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c) {
                    EXEC_OUTPUT_JOURNAL, EXEC_OUTPUT_JOURNAL_AND_CONSOLE,
                    EXEC_OUTPUT_KMSG, EXEC_OUTPUT_KMSG_AND_CONSOLE,
                    EXEC_OUTPUT_SYSLOG, EXEC_OUTPUT_SYSLOG_AND_CONSOLE))
-                return 0;
+                 return 0;


I probably can't review this as a whole. This line though, I think the old indentation might have been right. Maybe double-check this.

true, will fix

sourcejedi · 2017-11-09T18:11:20Z

test/TEST-16-UDEV-WANTS/testsuite.sh

+udevadm control --reload
+udevadm trigger /dev/sda
+
+while : ; do


Shouldn't we add a timeout or retry limit? I think it would be OK to have a false alarm if the test computer just hung for 30 seconds or something.

It's a test, and the tests already have a timeout applied generically from the test suite, hence I don't think we need additional timeout checks in the scripts...

Surely you have to set QEMU_TIMEOUT explicitly in the specific test, otherwise it defaults to infinity. I don't see that here?

https://github.com/systemd/systemd/search?q=QEMU_TIMEOUT

testsuite.service doesn't get a timeout because of Type=oneshot.

ah, right! will update

poettering · 2017-11-10T10:46:59Z

force pushed an updated version with the one indent issue corrected, ptal!

poettering · 2017-11-10T11:20:39Z

force pushed a new version now, which adds the suggested qemu timeout

keszybz

Looks very nice. Seems to work fine too in some simple tests. A few nitpicks and questions.

keszybz · 2017-11-10T12:08:39Z

src/gpt-auto-generator/gpt-auto-generator.c

@@ -688,7 +688,7 @@ static int add_mounts(void) {
 }

 int main(int argc, char *argv[]) {
-        int r = 0, k;
+        int r, k;


Commit message is not ideal, but OK.

keszybz · 2017-11-10T12:11:55Z

src/core/device.c

+        if (bound_by) {
+                r = parse_boolean(bound_by);
+                if (r < 0)
+                        log_warning_errno(r, "Failed to parse SYSTEMD_MOUNT_DEVICE_BOUND udev property of %s, ignoring: %m", strna(d->sysfs));


It'd be nice to include the rhs in the error message.

ah, right-hand-side, got it. "rvalue" is the term I got taught for that...

keszybz · 2017-11-10T12:16:42Z

man/systemd.unit.xml

+    trailing or duplicate <literal>/</literal> characters are removed from the string before transformation. Example:
+    <filename>/foo//bar/baz/</filename> becomes <literal>foo-bar-baz</literal>.</para>
+
+    <para>This escaping is fully reversible. The


..., as long as it is known whether the escaped string was a path (the unescaping results are different for paths and non-path strings).
...
Use <command>systemd-escape --path</command> to escape path strings, and <command>systemd-escape</command> without <option>--path</option> otherwise.

keszybz · 2017-11-10T13:10:19Z

src/core/unit.h

+typedef enum UnitDependencyMask {
+        /* Configured directly by the unit file, .wants/.requries symlink or drop-in, or as an immediate result of a
+         * non-dependency option configured that way.  */
+        UNIT_DEPENDENCY_FILE               = 1 << 0,


It'd be great to separate those cases. For users, information whether a dependency is from .wants symlink or from Wants= line is very useful. But it's all internal stuff, so maybe let's leave this as is now, and consider making the mask more granular later.

yeah, i had this a lot more granular earlier (including separate bits for dropins), but i got lost in it, and then dumbed it down again. But yeah, we can make this more granular as we go and what we need it for.

keszybz · 2017-11-10T15:14:01Z

src/core/mount.c

        pm = get_mount_parameters_fragment(m);
        if (pm && pm->what &&
            path_is_absolute(pm->what) &&
            (mount_is_bind(pm) || mount_is_loop(pm) || !mount_is_network(pm))) {

-                r = unit_require_mounts_for(UNIT(m), pm->what);
+                r = unit_require_mounts_for(UNIT(m), pm->what, UNIT_DEPENDENCY_FILE);


U_D_FILE matches the comment for U_D_FILE in unit.h. Nevertheless, U_D_IMPLICIT would seem more natural. Why is it _FILE here and _IMPLICIT in line 302?

it's all depending on where the dependency was configured: deps that are created exclusively based on the name of the unit, are of the "implicit" kind, and that's the case for the dep in 302: we check m->where, which in turn is derived directly from the unit name. That means even if the unit had no unit file at all, the dep on its parent would be created. Or to say this differently: regardless what the unit file defines, the parent deps is always created, hence it is implicit.

This is different for the dep in 314: it depends on stuff that is configured in the unit file, as we use get_mount_parameters_fragment() and then use pm->what from that. If the unit file would not exist at all (maybe because the mount unit appeared just from /proc/self/mountinfo), get_mount_parameters_fragment() would return NULL, and no dep of this kind would be created. Moreover, depending on the contents of the unit file pm->what differ greatly, hence this dep is created as UNIT_DEPENDENCY_FILE.

Or to see this from a different standpoint: let's say we implement "pinpointed" reloads, i.e. we want to flush out every single bit of config of the unit that was generated from the old unit file, and then load the new unit file with the new config. When flushing out, we'd like to remove the deps created due to pm->what in preparation that it might change with the new unit file. OTOH the dep for m->what we don't have to flush out, since the it only depends on the name of the unit, and that's not gonna change during the reload. Hence the latter is marked as _IMPLICIT, the other AS _FILE

OK, makes sense.

keszybz · 2017-11-10T15:42:36Z

src/core/unit.c

+        if (di.origin_mask == 0 || di.destination_mask == 0) {
+                /* No bit set anymore, let's drop the whole entry */
+                assert_se(hashmap_remove(u->dependencies[d], other));
+                log_unit_error(u, "Removing %s dependency %s vs. %s", unit_dependency_to_string(d), u->id, other->id);


log_unit_debug? Also this is pretty unclear. Maybe ("%s lost %s=%s dependency", unit_dependency_to_string(d), u->id, other->id)? (I think it's better to keep the source unit first in the message for easier parsing, hence this convoluted sentence. Maybe you can come up with something better.)

uh, this is debugging left-over...

but we might as well keep it, and fix the log message as you say.

keszybz · 2017-11-10T15:50:38Z

test/TEST-16-UDEV-WANTS/test.sh

+# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*-
+# ex: ts=8 sw=4 sts=4 et filetype=sh
+set -e
+TEST_DESCRIPTION="UDEV SYSTEMD_WANTS property"


Some other PR is also adding TEST-16-* I think. So if you were respinning this, it'd be good to change to 17.

keszybz · 2017-11-10T15:51:34Z

test/TEST-16-UDEV-WANTS/test.sh

+        ln -s /dev/null $initdir/etc/systemd/system/systemd-networkd.service
+        ln -s /dev/null $initdir/etc/systemd/system/systemd-networkd.socket
+        ln -s /dev/null $initdir/etc/systemd/system/systemd-resolved.service
+echo OK > /testok


Is this supposed to be here?

no, it's not, not sure where that came from...

keszybz · 2017-11-10T15:52:17Z

test/TEST-16-UDEV-WANTS/test.sh

+Description=Testsuite service
+
+[Service]
+ExecStart=/bin/sh -x -c '/testsuite.sh'


/bin/sh -x /testsuite.sh?

keszybz · 2017-11-10T15:53:39Z

test/TEST-16-UDEV-WANTS/testsuite.sh

+    sleep .5
+done
+
+cat > /run/udev/rules.d/50-testuite.rules <<EOF


Typo in testuite.

This way these dirs will be created automatically if they are missing, thus always guaranteeing optimal speedy behaviour. (Well at least, after systemd/mkosi#181 is merged)

On Fedora /etc/init.d is a symlink to /etc/rc.d/init.d. Our build scripts default to /etc/init.d since that is the LSB default. Let's make sure the build script thus follows the symlink correctly and configures to path explicitly, since otherwise our build artifacts in $DESTDIR are incompatible with the setup we actually need for Fedora.

This replaces the dependencies Set* objects by Hashmap* objects, where the key is the depending Unit, and the value is a bitmask encoding why the specific dependency was created. The bitmask contains a number of different, defined bits, that indicate why dependencies exist, for example whether they are created due to explicitly configured deps in files, by udev rules or implicitly. Note that memory usage is not increased by this change, even though we store more information, as we manage to encode the bit mask inside the value pointer each Hashmap entry contains. Why this all? When we know how a dependency came to be, we can update dependencies correctly when a configuration source changes but others are left unaltered. Specifically: 1. We can fix UDEV_WANTS dependency generation: so far we kept adding dependencies configured that way, but if a device lost such a dependency we couldn't them again as there was no scheme for removing of dependencies in place. 2. We can implement "pin-pointed" reload of unit files. If we know what dependencies were created as result of configuration in a unit file, then we know what to flush out when we want to reload it. 3. It's useful for debugging: "systemd-analyze dump" now shows this information, helping substantially with understanding how systemd's dependency tree came to be the way it came to be.

Let's log when we can't parse the udev property, and always use the most precise, correct types.

…ncy mask let's make use of the dependency mask, and add internal API to remove dependencies ago, based on bits in the dependency mask.

Let's drop use of one variable and make the rest more explicit.

…he right moment Previously dependencies configured with SYSTEMD_WANTS would be collected on a device unit as long as it was loaded. let's fix that, and remove dependencies again when SYTEMD_WANTS changes.

…e it with sysfs path This should make cases like the user's setup in systemd#7109 a lot easier to handle, as in that case we'll do the right escaping automatically.

…plicit" There should be a way to turn this logic of, and DefaultDependencies= appears to be the right option for that, hence let's downgrade this dependency type from "implicit" to "default, and thus honour DefaultDependencies=. This also drops mount_get_fstype() as we only have a single user needing this now. A follow-up for systemd#7076.

poettering · 2017-11-10T18:54:14Z

force pushed a new version now, with all of @keszybz's points addresses.

Thanks for the review!

yuwata · 2017-11-11T15:09:39Z

Please wait to merge this. With this PR, systemd in dracut image segfaults. I do not get any information even if with systemd.log_level=debug boot option...
Fortunately(?), systemd with this PR works fine with old dracut image, that is, systemd in dracut image does not include this PR.

yuwata · 2017-11-11T23:52:27Z

~~4b58699 causes my issue.~~

yuwata · 2017-11-12T00:11:13Z

Sorry, 3e3852b causes my issue.

yuwata · 2017-11-12T00:24:05Z

src/core/mount.c

@@ -517,6 +493,13 @@ static int mount_add_default_dependencies(Mount *m) {
        if (r < 0)
                return r;

+        /* If this is a tmpfs mount then we have to unmount it before we try to deactivate swaps */
+        if (streq(p->fstype, "tmpfs")) {


p->fstype may be NULL. I am not sure but I guess that streq_ptr() is suitable here.

By changing to streq_ptr(), my system boots and works fine.

yuwata · 2017-11-12T12:45:42Z

In the commit message of 4b58699, "let's us it" -> "let's use it".

No need to set an empty string here, sd-bus serializes NULL as empty string anway.

keszybz · 2017-11-12T15:07:34Z

I updated @poettering's PR with your suggested changes (typo and streq_ptr). I'll merge it if semaphore passes.

…t message The PR systemd#7186 changes requires_mounts_for from strv to Hashmap. So, it is necessary to implement a function to get property for RequiresMountsFor=. This introduces property_get_requires_mounts_for() which read the Hashmap and send messages to bus. Fixes systemd#7321.

…t message PR systemd#7186 changes requires_mounts_for from strv to Hashmap. So, it is necessary to implement a function for getting the property RequiresMountsFor=. This introduces property_get_requires_mounts_for() which read the Hashmap and send messages to bus. Fixes systemd#7321.

…t message PR systemd#7186 changes requires_mounts_for from strv to Hashmap. So, it is necessary to implement a function for getting the property RequiresMountsFor=. This introduces property_get_requires_mounts_for() which reads the Hashmap and sends messages to bus. Fixes systemd#7321.

…t message (#7322) PR #7186 changes requires_mounts_for from strv to Hashmap. So, it is necessary to implement a function for getting the property RequiresMountsFor=. This introduces property_get_requires_mounts_for() which reads the Hashmap and sends messages to bus. Fixes #7321.

poettering added documentation mkosi pid1 labels Oct 26, 2017

poettering force-pushed the track-deps branch from 9808241 to db86056 Compare October 26, 2017 15:54

poettering force-pushed the track-deps branch from db86056 to 8df6076 Compare November 9, 2017 14:48

sourcejedi reviewed Nov 9, 2017

View reviewed changes

poettering force-pushed the track-deps branch from 8df6076 to 2c2dd16 Compare November 10, 2017 10:46

poettering force-pushed the track-deps branch from 2c2dd16 to 48cd15c Compare November 10, 2017 11:20

keszybz reviewed Nov 10, 2017

View reviewed changes

poettering added 14 commits November 10, 2017 19:00

virt: trivial whitespace fixes

45e1c7d

core: include a bad /var/run symlink in the "tainted" string

e37fae1

mkosi: configure mkosi.cache/ and mkosi.builddir/ explicitly

f33a319

This way these dirs will be created automatically if they are missing, thus always guaranteeing optimal speedy behaviour. (Well at least, after systemd/mkosi#181 is merged)

gpt-auto-generator: make sure "r" is always set

8f50e86

device: rework device_is_bound_by_mounts() a bit

40b1a32

Let's log when we can't parse the udev property, and always use the most precise, correct types.

man: extend documentation on the unit name escaping logic

2651d03

core: add internal API to remove dependencies again, based on depende…

c999cf3

…ncy mask let's make use of the dependency mask, and add internal API to remove dependencies ago, based on bits in the dependency mask.

device: Let's simplify device_add_udev_wants() a bit

de04054

Let's drop use of one variable and make the rest more explicit.

core: remove SYSTEMD_WANTS udev property configured dependencies at t…

38b9b72

…he right moment Previously dependencies configured with SYSTEMD_WANTS would be collected on a device unit as long as it was loaded. let's fix that, and remove dependencies again when SYTEMD_WANTS changes.

test: add test case for adding/removing dependencies via udev rules

3ac62a0

core: when a unit template is specified in SYSTEMD_WANTS=, instantiat…

dcebc9b

…e it with sysfs path This should make cases like the user's setup in systemd#7109 a lot easier to handle, as in that case we'll do the right escaping automatically.

poettering force-pushed the track-deps branch from 48cd15c to 4b9db1d Compare November 10, 2017 18:53

keszybz added the 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 label Nov 11, 2017

yuwata removed the 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 label Nov 11, 2017

yuwata requested changes Nov 12, 2017

View reviewed changes

yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 12, 2017

poettering and others added 3 commits November 12, 2017 14:27

core: sd-bus can handle NULL strings nicely, let's use it

ecd4832

No need to set an empty string here, sd-bus serializes NULL as empty string anway.

update TODO

17b6f89

core/mount: fstype may be NULL

74b1731

keszybz force-pushed the track-deps branch from 4b9db1d to 74b1731 Compare November 12, 2017 15:06

keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 12, 2017

keszybz merged commit f886559 into systemd:master Nov 12, 2017

yuwata mentioned this pull request Nov 14, 2017

core/dbus-unit: add property_get_requires_mounts_for() to send correct message #7322

Merged

Uh oh!

rework unit dependency data structure to track why deps get created #7186

rework unit dependency data structure to track why deps get created #7186

Uh oh!

Conversation

poettering commented Oct 26, 2017

Uh oh!

sourcejedi commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering commented Nov 9, 2017

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!

Choose a reason for hiding this comment

Uh oh!

poettering commented Nov 10, 2017

Uh oh!

poettering commented Nov 10, 2017

Uh oh!

keszybz 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!

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!

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!

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 commented Nov 10, 2017

Uh oh!

yuwata commented Nov 11, 2017

Uh oh!

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

Uh oh!

Uh oh!

yuwata commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcejedi commented Oct 31, 2017 •

edited

Loading

yuwata commented Nov 11, 2017 •

edited

Loading