Add flag to allow you to disable tink-worker action logging rollup

[splaspood]

Description

Currently tink-worker attaches to the 'console' of each action container and logs its STDOUT/STDERR alongside tink-worker's normal output. This change allows this behavior to be disabled through a flag.

Why is this needed

We wish to log all action output centrally via syslog as configured in the docker 'daemon.json'. If we enable that, and tink-worker still wants to grab the action logs the conflict will cause actions to fail to execute.

[codecov]

Codecov Report

Merging #443 (f354b54) into master (db0edc3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   35.14%   35.14%           
=======================================
  Files          47       47           
  Lines        2888     2888           
=======================================
  Hits         1015     1015           
  Misses       1781     1781           
  Partials       92       92           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db0edc3...f354b54. Read the comment docs.

[gianarb]

Why we do not remove it all together?! What's the point about the current behavior?!

Thanks!!

[splaspood]

Why we do not remove it all together?! What's the point about the current behavior?!

Thanks!!

To know for sure we'd have to speak to those who did the initial design/coding around this. Currently, this allows you to see all the action logs in one place, alongside the tink-worker logs. This is probably a benefit to smaller users of tink without an external logging infrastructure. Or at least, that was my thinking. I'm fine getting rid of it entirely, but I figured that was a change that would require wider buy-in.

[gianarb]

Just to summarise the discussion we had on slack with @mmlb .

I thought about the possibility to remove this feature all together, mainly because it is easy to grow everything behind a feature flag but long term it makes things confusing but let me explain why we are keeping it today.

As far as it is today (before this PR), tink-worker removes all the containers it runs straight away, almost like docker run --rm works. It means that logs from a container by default do not get stored on file, or at least they get removed from the server when the container gets removed (and as you know it means at the end of the execution). This is a problem when you don't centralize logs somewhere (by default for example sandbox does not have this capability) because you won't have the ability to figure out what an action failed. To avoid that tink-worker prints all the logs coming from a container it runs to its stdout. This creates a bit of a mess in the tink-worker logs but it enables operators to look at action logs even without centralizing them.

The proposal we made to avoid that was to remove the equivalent of --rm from tink-worker removing only the containers who succeed. In this way, people will still be able to get logs from the container that failed. But it does not sound worth the effort and we prefer one more customization that you can apply to tink-worker