Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

systemd: read initconfdir #12946

Merged
merged 1 commit into from Feb 22, 2022
Merged

systemd: read initconfdir #12946

merged 1 commit into from Feb 22, 2022

Conversation

pikrzysztof
Copy link
Contributor

@pikrzysztof pikrzysztof commented Jan 7, 2022 •
edited

Systemd units do not read @initconfdir@ but refer to variables defined
there.

also a minor fixup in zfs-scrub service file

Motivation and Context

some systemd unit files refer to undefined variables which is a bug. Pulling the config from @initconfdir@/zfs is the best idea - makes zfs behavior consistent.

Description

How Has This Been Tested?

I built & installed zfs with this patch and checked that the systemd modules work as expected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 7, 2022
@szubersk
Copy link
Contributor

Fixing import issues here is orthogonal to fixing the delayed drives detection. I'd strongly suggest opening a separate PR for clarity and better tracking. Additionally, the description of the change is misleading as it hides sleep 30 in zpool import under configuration file reading.

@pikrzysztof
Copy link
Contributor Author

pikrzysztof commented Feb 15, 2022 via email

@szubersk
Copy link
Contributor

@pikrzysztof, if you still feel like getting this PR merged, feel free to tidy it up. It is good work in area of systemd unit files and it would be a waste to lose it.

szubersk
szubersk approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@szubersk szubersk left a comment

Choose a reason for hiding this comment

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

Good job!

behlendorf
behlendorf approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, good stuff. My only concern is that it may be a little confusing that most of these environment variables don't apply to the systemd units. Instead, most entries are either for the init scripts or to set DKMS build options.

I'm sure users would find it helpful if we made a pass over the default etc/default/zfs.in file and updated the comments to make it clearer which options apply where. But that's not a new issue so I think it'd be fine to tackle in a follow up PR and doesn't need to block this.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 16, 2022
@behlendorf
Copy link
Contributor

@pikrzysztof if you can rebase this on the latest commits in the master branch and force update the PR so we can get a clean CI run this should be good to go.

@pikrzysztof
systemd: read initconfdir
9c93199 
Systemd units do not read @initconfdir@ but refer to variables defined
there.

also a minor fixup in zfs-scrub service file

Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
@szubersk
Copy link
Contributor

Thank you, @pikrzysztof, for such fast reaction!

@pikrzysztof
Copy link
Contributor Author

@behlendorf it should be good now, thanks

gmelikov
gmelikov reviewed Feb 21, 2022
View reviewed changes
Copy link
Member

@gmelikov gmelikov left a comment

Choose a reason for hiding this comment

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

LGTM

@behlendorf behlendorf merged commit ccdcc1d into openzfs:master Feb 22, 2022
@pikrzysztof pikrzysztof deleted the kris--fix-systemd-units branch February 28, 2022 17:58
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@pikrzysztof @nicman23
systemd: read initconfdir
620c1db 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@pikrzysztof @nicman23
systemd: read initconfdir
08a0ce8 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Aug 30, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
0a937ea 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 1, 2022
@pikrzysztof @lundman
systemd: read initconfdir
5e57fd5 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
36b4efc 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
e1ff688 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
22b9ac4 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
bd1b850 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
56762dd 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@pikrzysztof @andrewc12
systemd: read initconfdir
ccca036 
Systemd units do not read @initconfdir@ but refer to variables defined
there, also a minor fixup in zfs-scrub service file.

Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Damian Szuberski <szuberskidamian@gmail.com>
Signed-off-by: Krzysztof Piecuch <piecuch@kpiecuch.pl>
Closes openzfs#12946
@gyakovlev
Copy link
Contributor

gyakovlev commented Jan 27, 2023 •
edited

this unfortunately results in illegal configuration on gentoo with systemd systems

 QA Notice: systemd units using /etc/conf.d detected:         
 * /usr/lib/systemd/system/zfs-import-cache.service:EnvironmentFile=-/etc/conf.d/zfs                                            
 * /usr/lib/systemd/system/zfs-import-scan.service:EnvironmentFile=-/etc/conf.d/zfs
 ....

basically we do not allow using openrc configuration files (/etc/conf.d/zfs) on systemd systems, those files may not exist.

not sure how to fix it yet, just leaving a comment for visibility, will try to figure out how to handle that, maybe just downstream patch, idk yet.

https://bugs.gentoo.org/892269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabijaczleweli nabijaczleweli nabijaczleweli left review comments

@behlendorf behlendorf behlendorf approved these changes

@gmelikov gmelikov gmelikov approved these changes

@szubersk szubersk szubersk approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pikrzysztof @szubersk @behlendorf @gyakovlev @gmelikov @nabijaczleweli