trivago / gollum

An n:m message multiplexer written in Go
http://gollum.readthedocs.org/en/latest/
Apache License 2.0
940 stars 74 forks source link

Multiple problems in consumer.Syslogd #185

Open ppar opened 7 years ago

ppar commented 7 years ago

[1] Debian 9 x86_64, logger from bsdutils 1:2.28.2-1

arnecls commented 7 years ago
  1. "simply reusing" is not really possible as the socket can be owned by another process. A possible solution for this case is implemented in consumer.Socket.
  2. Can be solved by pushing the error instead of just reporting it, but again, see 1
  3. That's a bug
  4. Interesting point. Until now we tried to fix the problem if possible (e.g. by forcing the correct protocol). We could change this to "don't startup" now as config errors will always stop gollum from starting, but this has to be consequently fixed in all other plugins, too.
  5. Needs further investigation but this might not be a problem of the underlying library or the logger tool (which I personally don't think, but syslog clients seem to be broken quite often)
  6. We did not touch syslogd after adding metadata, but this is a good idea.
ppar commented 7 years ago

"simply reusing" is not really possible as the socket can be owned by another process.

Apparently it's the server.ListenUnixgram() call that fails if the socket already exists, whether's it's in use or not - so the consistent behavior here would be to just fail hard and terminate startup if this happens.

Same for the .ListenTCP() and .ListenUDP() calls in Consume(), it doesn't look like their failures abort startup either.

Removing the socket at shutdown like consumer.Socket seems to do solves the idempotency problem anyway.

We could change this to "don't startup" now as config errors will always stop gollum from starting, but this has to be consequently fixed in all other plugins, too.

Agreed.