Implement container logger into shim binary plugin #738
Conversation
maxux
requested review from
LeeSmet,
delandtj,
muhamadazmy and
zaibon
as code owners
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
==========================================
+ Coverage 24.74% 34.07% +9.32%
==========================================
Files 63 68 +5
Lines 4470 5113 +643
==========================================
+ Hits 1106 1742 +636
+ Misses 3204 3118 -86
- Partials 160 253 +93
Continue to review full report at Codecov.
|
zaibon
left a comment
zaibon
left a comment
There was a problem hiding this comment.
Couple of remarks on the code itself.
I also would like we decide whether we keep logging to disk at all or not.
Normally nobody would ever be able to read those logs, so I'm not sure this is worth it to create them at all.
If we decide not to log to always log to disk, then the code needs to be adapted to only start the logging-shim when the user asked for it.
I'm also a bit concerned regarding the memory usage of those shim. Maybe we could measure how much capacity they use and count that in the resource unit computation of the reservation.
Another idea to limit the amount of memory for for those daemon is to only have one per user per node. But this requries a bit more management
Since we potentially need to run lot of logger instance because we need one per container, we need it to have the smallest memory footprint possible. After some test, the Go version was using ~3.6 MB of memory for a 3 MB binary, with a basic Rust PoC, memory dropped down to ~1.6 MB. This C version with static jansson and hiredis libraries bundled, stripped binary size is 87K (138K fully static with musl). On runtime, using glibc, shim-logs with 2 redis connections (one for stdout and one for stderr) while transfering data, consume ~230 KB of memory. The static musl versions seems to use even less but could not get accurate values. This only depends on hiredis and jansson external libs which are compiled staticly by default with the Makefile. In addition, there is a tools directory which contains a wrapper which simulate a binary execution like it would be with containerd. This is useful to test without actually run a container.
In order to solve #718 we will move container logging feature from
contdtoshim. They provide a way to use external library/binary to handle logging.This pull request move logging functions from
contdto a newshim-zoslogbinary decidated to logging purpose.When this pull request will be ready and approved, restarting
contdwon't cut logs anymore and fifo will never be not read, except if something really bad happen to the newshim-zoslogwhich still need to be tested :)