[PATCH v2] kdump-dep-generator: Add kdump service dependencies on the fly

[WANG Chao]

kdump-dep-generator is a systemd generator, used to write out kdump
service dependencies.

Currently it's only useful for ssh dump case. And in ssh dump case, it
writes out a dependency which kdump.service "Wants"
network-online.target:

# ls -l /run/systemd/generator/kdump.service.wants/
  [..] network-online.target -> /usr/lib/systemd/system/network-online.target

So that kdump.service will pull in network-online.target and delayed
start until network-online.target is reached.

In the future, we could use generator to write out kdump.service
dynamically and get rid of the static defined kdump.service at all.

v1->v2:
Vivek: not using hardcoded run time generator path, use what systemd pass in.

Signed-off-by: WANG Chao 
---
 kdump-dep-generator.sh | 22 ++++++++++++++++++++++
 kexec-tools.spec       |  3 +++
 2 files changed, 25 insertions(+)
 create mode 100644 kdump-dep-generator.sh

diff --git a/kdump-dep-generator.sh b/kdump-dep-generator.sh
new file mode 100644
index 0000000..b6fab2d
--- /dev/null
+++ b/kdump-dep-generator.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+# More details about systemd generator:
+# http://www.freedesktop.org/wiki/Software/systemd/Generators/
+
+. /usr/lib/kdump/kdump-lib.sh
+
+# If invokded with no arguments for testing purpose, output to /tmp to
+# avoid overriding the existing.
+dest_dir="/tmp"
+
+if [ -n "$1" ]; then
+    dest_dir=$1
+fi
+
+systemd_dir=/usr/lib/systemd/system
+kdump_wants=$dest_dir/kdump.service.wants
+
+if is_ssh_dump_target; then
+    mkdir -p $kdump_wants
+    ln -sf $systemd_dir/network-online.target $kdump_wants/
+fi
diff --git a/kexec-tools.spec b/kexec-tools.spec
index cfe14c4..e6ffade 100644
--- a/kexec-tools.spec
+++ b/kexec-tools.spec
@@ -26,6 +26,7 @@ Source18: kdump.sysconfig.s390x
 Source19: eppic_030413.tar.gz
 Source20: kdump-lib.sh
 Source21: kdump-in-cluster-environment.txt
+Source22: kdump-dep-generator.sh

#######################################
 # These are sources for mkdumpramfs
@@ -181,6 +182,7 @@ install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_sysconfdir}/udev/rules.d/98-kexec.r
 %endif
 install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
 install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
+install -m 755 -D %{SOURCE22} $RPM_BUILD_ROOT%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
 mkdir -p $RPM_BUILD_ROOT/usr/sbin
 install -m 755 %{SOURCE17} $RPM_BUILD_ROOT/usr/sbin/rhcrashkernel-param

@@ -320,6 +322,7 @@ done
 %{_mandir}/man8/*
 %{_mandir}/man5/*
 %{_unitdir}/kdump.service
+%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
 %doc News
 %doc COPYING
 %doc TODO

[Vivek Goyal]

On Wed, Apr 09, 2014 at 12:27:29AM +0800, WANG Chao wrote:
> kdump-dep-generator is a systemd generator, used to write out kdump
> service dependencies.
>
> Currently it's only useful for ssh dump case. And in ssh dump case, it
> writes out a dependency which kdump.service "Wants"
> network-online.target:
>
> # ls -l /run/systemd/generator/kdump.service.wants/
>   [..] network-online.target -> /usr/lib/systemd/system/network-online.target
>
> So that kdump.service will pull in network-online.target and delayed
> start until network-online.target is reached.
>
> In the future, we could use generator to write out kdump.service
> dynamically and get rid of the static defined kdump.service at all.
>
> v1->v2:
> Vivek: not using hardcoded run time generator path, use what systemd pass in.
> Signed-off-by: WANG Chao
>
> kdump-dep-generator.sh | 22 ++++++++++++++++++++++
>  kexec-tools.spec       |  3 +++
>  2 files changed, 25 insertions(+)
>  create mode 100644 kdump-dep-generator.sh
>
> diff --git a/kdump-dep-generator.sh b/kdump-dep-generator.sh
> new file mode 100644
> index 0000000..b6fab2d
> --- /dev/null
> +++ b/kdump-dep-generator.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
>
> +# More details about systemd generator:
> +# http://www.freedesktop.org/wiki/Software/systemd/Generators/
>
> +. /usr/lib/kdump/kdump-lib.sh
>
> +# If invokded with no arguments for testing purpose, output to /tmp to
> +# avoid overriding the existing.
> +dest_dir="/tmp"
>
> +if [ -n "$1" ]; then
> dest_dir=$1
>
> +fi
>
> +systemd_dir=/usr/lib/systemd/system

Hi Chao,

I don't like this hardcoding of /usr dir either. I don't think we should
rely on hardcoded paths.

I am re-reading generators link and following is my understanding.

- generators are supposed to generate unit files. symlink document is
  talking of is creating symlinks in other .wants/ dir of other unit
  files and not vice-a versa.

- IIUC, there are 3 paths where unit files can be present. /usr/lib/,
  /run/.. and /etc/systemd/. Here /usr has lowest prio, /run has higer
  prio and /etc/systemd has highest prio.

To me that means if there are kdump.service unit files present in
  all three pathes and if you do "systemctl enable kdump" then systmed
  will first look into /etc, then in /run and then in /usr/ for a
  kdump.service.

IOW, there is an hierarchy of unit files and hence we should not force
  linking to a particular file in particular hiearchy. We should just
  deal with unit file names and let systemd figure out which directory
  to look into.

- So here is the proposal. I think let us generate a kdump.service file
  dynamically. I guess you can use /usr/lib/systemd/system/kdump.service
  as template. And put Wants=network-online in that kdump.service file.

That way there will be two kdump.service files. One in /run/ and other
  in /usr/ and one present in /run should override the one in /usr.

I think that's probably a better way to solve the problem.

Also if you are using existing kdump.service file as template, then you
can specify this in new kdump.service using SourcePath=.

Using existing file as template will help us so that if there are
any patches in kdump.service, we don't have to change kdump-dep-generator
all the time and make same changes there.

Thanks
Vivek
> +kdump_wants=$dest_dir/kdump.service.wants
>
> +if is_ssh_dump_target; then
> mkdir -p $kdump_wants
> ln -sf $systemd_dir/network-online.target $kdump_wants/
>
> +fi
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index cfe14c4..e6ffade 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -26,6 +26,7 @@ Source18: kdump.sysconfig.s390x
>  Source19: eppic_030413.tar.gz
>  Source20: kdump-lib.sh
>  Source21: kdump-in-cluster-environment.txt
> +Source22: kdump-dep-generator.sh
>
> #######################################
>  # These are sources for mkdumpramfs
> @@ -181,6 +182,7 @@ install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_sysconfdir}/udev/rules.d/98-kexec.r
>  %endif
>  install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
>  install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
> +install -m 755 -D %{SOURCE22} $RPM_BUILD_ROOT%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
>  mkdir -p $RPM_BUILD_ROOT/usr/sbin
>  install -m 755 %{SOURCE17} $RPM_BUILD_ROOT/usr/sbin/rhcrashkernel-param
>
> @@ -320,6 +322,7 @@ done
>  %{_mandir}/man8/*
>  %{_mandir}/man5/*
>  %{_unitdir}/kdump.service
> +%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
>  %doc News
>  %doc COPYING
>  %doc TODO
> -- 
> 1.8.5.3

[Vivek Goyal]

On Tue, Apr 08, 2014 at 01:23:33PM -0400, Vivek Goyal wrote:
> On Wed, Apr 09, 2014 at 12:27:29AM +0800, WANG Chao wrote:
> > kdump-dep-generator is a systemd generator, used to write out kdump
> > service dependencies.
> >
> > Currently it's only useful for ssh dump case. And in ssh dump case, it
> > writes out a dependency which kdump.service "Wants"
> > network-online.target:
> >
> > # ls -l /run/systemd/generator/kdump.service.wants/
> >   [..] network-online.target -> /usr/lib/systemd/system/network-online.target
> >
> > So that kdump.service will pull in network-online.target and delayed
> > start until network-online.target is reached.
> >
> > In the future, we could use generator to write out kdump.service
> > dynamically and get rid of the static defined kdump.service at all.
> >
> > v1->v2:
> > Vivek: not using hardcoded run time generator path, use what systemd pass in.
> > Signed-off-by: WANG Chao
> >
> > kdump-dep-generator.sh | 22 ++++++++++++++++++++++
> >  kexec-tools.spec       |  3 +++
> >  2 files changed, 25 insertions(+)
> >  create mode 100644 kdump-dep-generator.sh
> >
> > diff --git a/kdump-dep-generator.sh b/kdump-dep-generator.sh
> > new file mode 100644
> > index 0000000..b6fab2d
> > --- /dev/null
> > +++ b/kdump-dep-generator.sh
> > @@ -0,0 +1,22 @@
> > +#!/bin/sh
> >
> > +# More details about systemd generator:
> > +# http://www.freedesktop.org/wiki/Software/systemd/Generators/
> >
> > +. /usr/lib/kdump/kdump-lib.sh
> >
> > +# If invokded with no arguments for testing purpose, output to /tmp to
> > +# avoid overriding the existing.
> > +dest_dir="/tmp"
> >
> > +if [ -n "$1" ]; then
> > dest_dir=$1
> >
> > +fi
> >
> > +systemd_dir=/usr/lib/systemd/system
>
> Hi Chao,
>
> I don't like this hardcoding of /usr dir either. I don't think we should
> rely on hardcoded paths.
>
> I am re-reading generators link and following is my understanding.
>
> generators are supposed to generate unit files. symlink document is
> talking of is creating symlinks in other .wants/ dir of other unit
> files and not vice-a versa.
>
> IIUC, there are 3 paths where unit files can be present. /usr/lib/,
> /run/.. and /etc/systemd/. Here /usr has lowest prio, /run has higer
> prio and /etc/systemd has highest prio.
>
> To me that means if there are kdump.service unit files present in
> all three pathes and if you do "systemctl enable kdump" then systmed
> will first look into /etc, then in /run and then in /usr/ for a
> kdump.service.
>
> IOW, there is an hierarchy of unit files and hence we should not force
> linking to a particular file in particular hiearchy. We should just
> deal with unit file names and let systemd figure out which directory
> to look into.

Chao, I just met lennart again and discussed this patch. He says that
creating symlink this way is fine. He mentioned that even though we
create a link to file in /usr/lib/ he will first look for that file
in /etc/systemd/ and use that if available otherwise will fall back
to the one in /usr/lib/. So ordering should not be a problem.

He was also fine with creating link to /usr/lib/systemd/ one as it
will always be present.

So I guess we are fine with this patch and you don't have to make further
modifications.

Thanks
Vivek
> So here is the proposal. I think let us generate a kdump.service file
> dynamically. I guess you can use /usr/lib/systemd/system/kdump.service
> as template. And put Wants=network-online in that kdump.service file.
>
> That way there will be two kdump.service files. One in /run/ and other
> in /usr/ and one present in /run should override the one in /usr.
>
> I think that's probably a better way to solve the problem.
>
> Also if you are using existing kdump.service file as template, then you
> can specify this in new kdump.service using SourcePath=.
>
> Using existing file as template will help us so that if there are
> any patches in kdump.service, we don't have to change kdump-dep-generator
> all the time and make same changes there.
>
> Thanks
> Vivek
> > +kdump_wants=$dest_dir/kdump.service.wants
> >
> > +if is_ssh_dump_target; then
> > mkdir -p $kdump_wants
> > ln -sf $systemd_dir/network-online.target $kdump_wants/
> >
> > +fi
> > diff --git a/kexec-tools.spec b/kexec-tools.spec
> > index cfe14c4..e6ffade 100644
> > --- a/kexec-tools.spec
> > +++ b/kexec-tools.spec
> > @@ -26,6 +26,7 @@ Source18: kdump.sysconfig.s390x
> >  Source19: eppic_030413.tar.gz
> >  Source20: kdump-lib.sh
> >  Source21: kdump-in-cluster-environment.txt
> > +Source22: kdump-dep-generator.sh
> >
> > #######################################
> >  # These are sources for mkdumpramfs
> > @@ -181,6 +182,7 @@ install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_sysconfdir}/udev/rules.d/98-kexec.r
> >  %endif
> >  install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
> >  install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
> > +install -m 755 -D %{SOURCE22} $RPM_BUILD_ROOT%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
> >  mkdir -p $RPM_BUILD_ROOT/usr/sbin
> >  install -m 755 %{SOURCE17} $RPM_BUILD_ROOT/usr/sbin/rhcrashkernel-param
> >
> > @@ -320,6 +322,7 @@ done
> >  %{_mandir}/man8/*
> >  %{_mandir}/man5/*
> >  %{_unitdir}/kdump.service
> > +%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
> >  %doc News
> >  %doc COPYING
> >  %doc TODO
> > -- 
> > 1.8.5.3

[Vivek Goyal]

On Wed, Apr 09, 2014 at 12:27:29AM +0800, WANG Chao wrote:
> kdump-dep-generator is a systemd generator, used to write out kdump
> service dependencies.
>
> Currently it's only useful for ssh dump case. And in ssh dump case, it
> writes out a dependency which kdump.service "Wants"
> network-online.target:
>
> # ls -l /run/systemd/generator/kdump.service.wants/
>   [..] network-online.target -> /usr/lib/systemd/system/network-online.target
>
> So that kdump.service will pull in network-online.target and delayed
> start until network-online.target is reached.
>
> In the future, we could use generator to write out kdump.service
> dynamically and get rid of the static defined kdump.service at all.
>
> v1->v2:
> Vivek: not using hardcoded run time generator path, use what systemd pass in.
>
> Signed-off-by: WANG Chao

Looks good to me.

Acked-by: Vivek Goyal

Thanks
Vivek
> kdump-dep-generator.sh | 22 ++++++++++++++++++++++
>  kexec-tools.spec       |  3 +++
>  2 files changed, 25 insertions(+)
>  create mode 100644 kdump-dep-generator.sh
>
> diff --git a/kdump-dep-generator.sh b/kdump-dep-generator.sh
> new file mode 100644
> index 0000000..b6fab2d
> --- /dev/null
> +++ b/kdump-dep-generator.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
>
> +# More details about systemd generator:
> +# http://www.freedesktop.org/wiki/Software/systemd/Generators/
>
> +. /usr/lib/kdump/kdump-lib.sh
>
> +# If invokded with no arguments for testing purpose, output to /tmp to
> +# avoid overriding the existing.
> +dest_dir="/tmp"
>
> +if [ -n "$1" ]; then
> dest_dir=$1
>
> +fi
>
> +systemd_dir=/usr/lib/systemd/system
> +kdump_wants=$dest_dir/kdump.service.wants
>
> +if is_ssh_dump_target; then
> mkdir -p $kdump_wants
> ln -sf $systemd_dir/network-online.target $kdump_wants/
>
> +fi
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index cfe14c4..e6ffade 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -26,6 +26,7 @@ Source18: kdump.sysconfig.s390x
>  Source19: eppic_030413.tar.gz
>  Source20: kdump-lib.sh
>  Source21: kdump-in-cluster-environment.txt
> +Source22: kdump-dep-generator.sh
>
> #######################################
>  # These are sources for mkdumpramfs
> @@ -181,6 +182,7 @@ install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_sysconfdir}/udev/rules.d/98-kexec.r
>  %endif
>  install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
>  install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
> +install -m 755 -D %{SOURCE22} $RPM_BUILD_ROOT%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
>  mkdir -p $RPM_BUILD_ROOT/usr/sbin
>  install -m 755 %{SOURCE17} $RPM_BUILD_ROOT/usr/sbin/rhcrashkernel-param
>
> @@ -320,6 +322,7 @@ done
>  %{_mandir}/man8/*
>  %{_mandir}/man5/*
>  %{_unitdir}/kdump.service
> +%{_prefix}/lib/systemd/system-generators/kdump-dep-generator.sh
>  %doc News
>  %doc COPYING
>  %doc TODO
> -- 
> 1.8.5.3