Multiple systemd/dracut fixes/changes

[c0d3z3r0]

Motivation and Context / Description

1. Remove hard dependency on bash

   zfs-import-* services have a hard dependency on bash while not everyone
   has bash installed. At this point /bin/sh is sufficient, so use that.

2. Fix systemd-import services

   On debian systemd complains about missing /bin/awk because it actually
   is located at /usr/bin/awk. It is not a good idea to hardcode binary
   paths because linux distros use different paths. According to systemd's
   man page it is absolutely safe to miss paths for binary located at
   standard locations (/bin, /sbin, /usr/bin, ...).

   Further, replace this more or less complicated awk command by egrep.

3. Move dracut specifics to dracut mount unit

   Dracut depends on BOOTFS environment variable to be set after pool
   import. This dracut specific systemd ExecStartPost command should not be
   called for any non-dracut systems, so let's move it to a static systemd
   unit that gets pulled in by the generated mount unit.

At the moment, the new systemd service does not get installed / packaged. I did not get this to work... any hint from you would be great :-) Update: Fixed.

How Has This Been Tested?

Tested WITHOUT dracut! Maybe someome with a dracut (test) setup can test this?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[c0d3z3r0]

Well, all three commits modify the same line in the two unit files and depend on each... If you insist, I can split them tomorrow. Just give me a hint, which of my changes you might not want to merge.

…

Am 18.03.2019 um 00:53 schrieb kpande ***@***.***>: we might want some but not all of these changes, can you break them into separate PRs for easier review? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[c0d3z3r0]

@behlendorf -m1 does not work here because of the negative match. (AFAIK. I can‘t test this right now.) When there are multiple matches, how should we know which one is the right one? Hoping the first one is always correct does not really sound right to me..

…

Am 18.03.2019 um 22:09 schrieb Brian Behlendorf ***@***.***>: @behlendorf commented on this pull request. In contrib/dracut/systemd/zfs-dracut-bootfs.service.in: > @@ -0,0 +1,10 @@ +[Unit] +Description=Set BOOTFS environment for dracut +Documentation=man:zfs(8) +DefaultDependencies=no +After=zfs-import.target +Requires=zfs-import.target + +[Service] +Type=oneshot +ExecStart=/bin/sh -c "/bin/systemctl set-environment ***@***.***@/zpool list -H -o bootfs | grep -v '^-$')" I believe you should add -m1 to the grep so it exits on first match (like the previous awk). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[cwedgwood]

@behlendorf -m1 does not work here because of the negative match. (AFAIK. I can‘t test this right now.) When there are multiple matches, how should we know which one is the right one? Hoping the first one is always correct does not really sound right to me..

head -1 perhaps? it doesn't solve the 'right one' issue but at least would be consistent with what we have/had so far

[behlendorf]

at least would be consistent with what we have/had so far.

On a properly configured system there should only be one match. But when that's not the case picking the first one at least allows us to do something sensible. Let's at least keep the behavior consistent until we can do something better. I'd be OK with head -1.

[c0d3z3r0]

I added head -1 now. Packaging still does not work; the tests are complaining about that, too. :/

[c0d3z3r0]

The packaging issue is resolved now, too

[rlaager]

I added head -1 now. Packaging still does not work; the tests are complaining about that, too. :/

This is a bit pedantic, but technically speaking, the head -1 should be on the commit that changes it to grep. I see it's there on the last commit, but I missed it the first time through when reviewing.

[c0d3z3r0]

I added head -1 now. Packaging still does not work; the tests are complaining about that, too. :/

This is a bit pedantic, but technically speaking, the head -1 should be on the commit that changes it to grep. I see it's there on the last commit, but I missed it the first time through when reviewing.

Full ACK. Fixed

[rlaager]

In thinking about this some more, a grep | head is ripe for replacement by awk. This avoids a second fork(). Why not just keep the existing approach, but remove the hard-coded /bin/?

[c0d3z3r0]

@rlaager

In thinking about this some more, a grep | head is ripe for replacement by awk. This avoids a second fork(). Why not just keep the existing approach, but remove the hard-coded /bin/?

grep | head is still better readable than awk '$1 != \"-\" {print; exit}') and two forks instead of one at this point does not make that big difference.

@rlaager @behlendorf
I just read the grep man page again and realized I was wrong.... -m1 does work with a negative match. I simply stumbled over that double negation in When the -v or --invert-match option is also used, grep stops after outputting NUM **non-matching** lines., where "non-matching" means lines that do not match the (inverted match!) regex.

Proof:

$ echo -e '-\n-\nrpool/ROOT\ndpool/ROOT' | grep -m1 -v '^-$'
rpool/ROOT

I will change this to grep -v -m 1 ...

[behlendorf]

The contrib/dracut/90zfs module directory does seem like a reasonable place to put these. I'm not an expert in this area either, but I'm OK with adding them here.

[behlendorf]

@c0d3z3r0 thanks for sorting this out. I'm happy with the updated version, but it would be great if we could get a few more reviewers who are familiar with this area to approve it too.

[behlendorf]

I've cherry picked the first two simple commits in to master since there's no reason to hold them up any longer. @c0d3z3r0 would you mind rebasing only your last commit on master while we allow some time for additional reviewers. Thanks.

e03b25a Fix systemd-import services
3b26189 Remove hard dependency on bash

My review was for commits which no longer exist on this PR.

[rlaager]

I removed my stale review. I don't see anything that jumps out at me as wrong, but I am not familiar with dracut, so I can't give a useful approval here.

[codecov]

Codecov Report

Merging #8510 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8510      +/-   ##
==========================================
- Coverage   78.76%   78.66%   -0.11%     
==========================================
  Files         381      381              
  Lines      117499   117499              
==========================================
- Hits        92553    92426     -127     
- Misses      24946    25073     +127

Flag	Coverage Δ
#kernel	79.28% <ø> (-0.02%)	⬇️
#user	67.25% <ø> (-0.02%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03b25a...74204e8. Read the comment docs.

[prometheanfire]

tested fine

For some reason I didn't have zfs.target enabled and that caused zfs-mount to not start. I don't see a change in this commit that would have done that though (probably one of the other dracut commits)

[c0d3z3r0]

@prometheanfire

tested fine

Thank you!

For some reason I didn't have zfs.target enabled and that caused zfs-mount to not start. I don't see a change in this commit that would have done that though (probably one of the other dracut commits)

Do you mean zfs.target in the rootfs / after initramfs (etc/systemd/system/zfs.target.in)? I could not find any usage of zfs.target in contrib/dracut

[prometheanfire]

ya, after initram

[behlendorf]

Thanks everybody. I'll go ahead and get this merged this tomorrow. If you have any remaining concerns please post before then.