Re: [PATCH v2 27/29] xfs_scrub: integrate services with systemd
From: Eric Sandeen <hidden>
Date: 2018-01-31 20:30:24
On 1/31/18 12:41 AM, Darrick J. Wong wrote:
From: Darrick J. Wong <redacted> Create a systemd service unit so that we can run the online scrubber under systemd with (somewhat) appropriate containment. Signed-off-by: Darrick J. Wong <redacted> --- v2: fix some of the debian packaging weirdness, kudos to Nathan Scott!
Er, hang on: checking for SYSTEMD... configure: error: Package requirements (systemd) were not met: now systemd is a /build/ requirement for xfsprogs? nope nope. :) This is only used for unit file install targets, right? If systemd is missing, surely those install targets should just be skipped. Looks like the Makefile DTRT, but the stuff in m4/ should be non-fatal? (I can't tell what makes this fatal ... I think its' fatal?
checking for SYSTEMD... configure: error: Package requirements (systemd) were not met: No package 'systemd' found Consider adjusting the PKG_CONFIG_PATH environment variable if you installed software in a non-standard prefix. Alternatively, you may set the environment variables SYSTEMD_CFLAGS and SYSTEMD_LIBS to avoid the need to call pkg-config. See the pkg-config man page for more details. # echo $? 1
looks fatal.) ...
quoted hunk ↗ jump to hunk
diff --git a/scrub/Makefile b/scrub/Makefile index ca6dab0..0632794 100644 --- a/scrub/Makefile +++ b/scrub/Makefile@@ -15,6 +15,19 @@ LTCOMMAND = xfs_scrub INSTALL_SCRUB = install-scrub XFS_SCRUB_ALL_PROG = xfs_scrub_all XFS_SCRUB_ARGS = -b -n +ifeq ($(HAVE_SYSTEMD),yes) +INSTALL_SCRUB += install-systemd +SYSTEMD_SERVICES = xfs_scrub@.service xfs_scrub_all.service xfs_scrub_all.timer xfs_scrub_fail@.service +OPTIONAL_TARGETS += $(SYSTEMD_SERVICES) +endif +ifeq ($(HAVE_CROND),yes) +INSTALL_SCRUB += install-crond +CRONTABS = xfs_scrub_all.cron +OPTIONAL_TARGETS += $(CRONTABS) +# Don't enable the crontab by default for now +CROND_DIR = $(PKG_LIB_DIR)/$(PKG_NAME) +endif
Wouldn't an else if arrangement make more sense? Why would we install both? I'd have expected "Install systemd if available, else install crond if available, else don't install anything." ... and then the cron job can stop checking for systemd too:
quoted hunk ↗ jump to hunk
diff --git a/scrub/xfs_scrub_all.cron.in b/scrub/xfs_scrub_all.cron.in new file mode 100644 index 0000000..3dea929 --- /dev/null +++ b/scrub/xfs_scrub_all.cron.in@@ -0,0 +1 @@ +10 3 * * 0 root test -e /run/systemd/system || @sbindir@/xfs_scrub_alldiff --git a/scrub/xfs_scrub_all.in b/scrub/xfs_scrub_all.in
unless I'm missing something? Thanks, -Eric