Change to systemd type simple

[Tom-Fawcett]

Systemd does not require a process to daemonize.

Using Type=simple (where possible) is simpler and more robust.

Sources:

• https://www.freedesktop.org/software/systemd/man/daemon.html
• https://jdebp.eu/FGA/systemd-house-of-horror/daemonize.html

[repeatedly]

Thanks for the patch.
I will check this change doesn't break existing environment or not.

[robinbowes]

This is generally a good idea but in reality, as ever, it's not quite as clear cut.

systemd uses process groups to determine any processes that are started by the command in the unit file so there is no need to rely on PID files. This also works if a program starts, forks other processes and then exits. In other words, sometimes "forking" is the best option.

In the case of td-agent, I suspect that "simple" will be the right option.

I would also suggest that it might be sensible to use the --no-supervisor option as systemd will take care of supervision.

[robinbowes]

Further useful discussion here: https://www.redhat.com/en/blog/converting-traditional-sysv-init-scripts-red-hat-enterprise-linux-7-systemd-unit-files

[repeatedly]

Now, I'm testing this patch and here is the result.

 td-agent.service - td-agent: Fluentd based data collector for Treasure Data
   Loaded: loaded (/lib/systemd/system/td-agent.service; disabled; vendor preset: enabled)
   Active: active (running) since Tue 2018-04-10 19:04:19 PDT; 5s ago
     Docs: https://docs.treasuredata.com/articles/td-agent
 Main PID: 52735 (fluentd)
   CGroup: /system.slice/td-agent.service
           ├─52735 /opt/td-agent/embedded/bin/ruby /opt/td-agent/embedded/bin/fluentd --log /var/log/td-agent/td-agent.log
           └─52740 /opt/td-agent/embedded/bin/ruby -Eascii-8bit:ascii-8bit /opt/td-agent/embedded/bin/fluentd --log /var/log/td-agent/td-agent.log --under-sup

Apr 10 19:04:19 ubuntu systemd[1]: Started td-agent: Fluentd based data collector for Treasure Data.

From td-agent log, supervisor seems to recevie 2 same signals.

2018-04-10 19:04:54 -0700 [info]: Received graceful stop
2018-04-10 19:04:54 -0700 [info]: Received graceful stop

How to avoid this 2 signal problem?

[Tom-Fawcett]

@repeatedly I have pushed a fix.

Relevant docs:

https://www.freedesktop.org/software/systemd/man/systemd.service.html#ExecStop=
https://www.freedesktop.org/software/systemd/man/systemd.kill.html#KillMode=

@robinbowes' suggestion to use --no-supervisor is also worth considering, though I haven't tested that.

[repeatedly]

suggestion to use --no-supervisor is also worth considering,

We can't add --no-supervisor because fluentd's several features depend on supervisor, e.g. workers.

[sandstrom]

Is there a reason why td-agent is logging to a file instead of STDOUT? (which is then captured by systemd and accessible via its companion journald)

[robinbowes]

@sandstrom I can't comment in this specific case, but I've recently changed the hadoop-ptd services to do just that (log to STDOUT/journald)

[sandstrom]

@robinbowes Got it!

@repeatedly Is there a reason why you are logging to a file instead of stdout / journald when running under systemd?

[repeatedly]

@sandstrom Because some users monitor td-agent.log in their monitoring system.
In simple, using stdout is good idea but breaking existing environment is bad for existing users.

So how about providing 3 unit files for the migration?

td-agent 3

• td-agent.service (same as td-agent-fork.service)
• td-agent-fork.service
• td-agent-simple.service

td-agent 4

• td-agent.service (same as td-agent-simple.service)
• td-agent-fork.service
• td-agent-simple.service

[sandstrom]

@repeatedly Good point! (it is a breaking change)

I am not sure I'd provide three unit files though, I think it can cause needless confusion (and the names aren't enough to explain the difference, one would still need to dig around in the unit files themselves to learn that they do logging differently).

Instead I'd add the two examples (td-agent-fork.service and td-agent-simple.service) to the docs, along with a link to this page or this one (but I'd only have one unit file installed automatically, with the package). Then simply reference this part of the docs in the changelog.

That way people can easily adjust their service-file if needed.

(this is just my thinking, it is your decision to choose how you want to solve this, of course)

[repeatedly]

@Tom-Fawcett How about above 3 unit approches for exsting users?

[Tom-Fawcett]

@repeatedly I don't think the change in this PR is breaking, I intentionally did not change user functionality like logging to file vs journal. Therefore I don't believe the 3 unit approach is required.

However, I would not want you to just take my word for it. As I have not, for example, tested how your packages upgrade.

[repeatedly]

I don't think the change in this PR is breaking

I noticed current patch breaks existing environment because pid file is removed.
If we move to simple, we need to put pid file for existing users, pid file is used for signal or something.

[Tom-Fawcett]

Ah yes, if users rely on the PID file for something other than init that may be an issue.

[repeatedly]

Does systemd provide the feature to create additional file after start?

[sandstrom]

@repeatedly No, systemd cannot create pid-files.

The functionality that pid-files provide (looking up the process id of a process) is handled by systemd (for example systemctl status sshd). So there shouldn't be much need for it.

But if you want to be on the safe side, make this a breaking change (i.e. add it only during a major version bump).

---

Aside: any chance you'll have time to look at an Ubuntu 18.04 package? Would be awesome! 😄

[repeatedly]

Hmm... I see.
No way to create pid file maunally? No before/after hook?

any chance you'll have time to look

Will be released: #176

[minimum2scp]

@repeatedly

I found similar issue at gogs/gogs#3120.
Maybe you can use ExecStartPost like this:

ExecStartPost=/bin/sh -c 'echo $MAINPID > /var/run/td-agent/td-agent.pid'

[sandstrom]

Good idea, and then drop the pid-file altogether as a breaking change in the next major release (or make all of this a breaking change, depending on what's easiest).

[robinbowes]

How about only dropping the PID file if an env var is set? That way you can easily deprecate.

[repeatedly]

How about only dropping the PID file if an env var is set?

What does this mean? Does systemd support env var based condition? @minimum2scp ExecStartPost approach seems good for me.

[robinbowes]

I mean something like this...

1. Create a file /etc/default/td-agent containing:

   CREATE_PIDFILE=1
   

2. Add this to the systemd unit file:

   EnvironmentFile=-/etc/default/td-agent
   

3. Use this as the ExecStartPost fragment:

   ExecStartPost=/bin/sh -c '[[ -n $CREATE_PIDFILE ]] && echo $MAINPID > /var/run/td-agent/td-agent.pid'
   

[cedws]

This resolves #279.