runc
runc copied to clipboard
opencontainers
libcontainer logging
As pointed out by @mrunalp in https://github.com/opencontainers/runc/pull/3433#issuecomment-1079806993, libcontainer packages should not do any logging, since this is a library used by other users.
Unfortunately, libcontainer is also a part of runc which is a CLI tool and we expect it to do some logging.
This can be solved by having a logger in libcontainer, which defaults to a shallow implementation that does nothing, and then runc can redefine it to use logrus. Something like libcontainer.SetLogger() perhaps (where the argument is logrus compatible logger).
WDYT @opencontainers/runc-maintainers?
One other thing somewhat releated to this issue is getting rid of logrus entirely, since it is no longer maintained. I am not sure though if there's a better alternative that supports at least JSON output. Maybe @thaJeztah has some ideas?
Alternatively, libcontainer can have a build tag to disable any logging -- by creating a shallow logrus implementation. Not sure if that's a good idea, just thinking out loud here.
Yeh, there may be alternatives to logrus, OTOH, it's still widely used (and I think I was given write access to that repository in case urgent things are needed).
One option could be to use a "context logger", similar to how containerd approaches this; in this case the logger can be attached to the context, so we can still pass it (but others may skip that); https://github.com/containerd/containerd/blob/main/log/context.go
Overall, I agree that for library code, logging should be optional (at least). In this specific case (sync.Once()), it's a tricky one; we can store the error, but that means it will always be returned (of course caller can decide what to do)
We recently spent some time finding a logger and ended up choosing https://github.com/uber-go/zap for a custom logger wrapper we're currently writing at work. But as @thaJeztah said, it's still widely used and we still use it for a lot of our services.
@thaJeztah do you mean using context.Values for storing a logger instance and propagating that around? I think that's generally not deemed to be idiomatic[1] just because you're abstracting the logger instance which ideally should be an explicit argument to functions. Though I believe you are referencing containerd here wherein they are facilitating this through another package But it still doesn't feel idiomatic enough.
[1] - https://dave.cheney.net/2017/01/26/context-is-for-cancelation
The issue in question is make libcontainer logging optional. In other words, if runc is using libcontainer, log all the way. If someone else is using libcontainer, do not log (by default).
I found that some log printing for libcontainer has been added in https://gitee.com/src-openeuler/runc, you can refer to its related implementation
There's also https://github.com/go-logr/logr, which tries to separate interface from implementation