Use zap logger with logrotation, file logging for ip-reconciler #210
Conversation
xagent003
force-pushed
the
private/arjun/upstreamLoggingFix
branch
from
665119a
to
3cb6f11
Compare
Makes sense to me.
Makes sense to me. Thanks for the effort in this one.
I don't understand why you are removing the |
| @@ -13,11 +13,11 @@ const defaultReconcilerTimeout = 30 | |||
|
|
|||
| func main() { | |||
| kubeConfigFile := flag.String("kubeconfig", "", "the path to the Kubernetes configuration file") | |||
| logLevel := flag.String("log-level", "error", "the logging level for the `ip-reconciler` app. Valid values are: \"debug\", \"verbose\", \"error\", and \"panic\".") | |||
| logFile := flag.String("log-file", "/host/var/log/ip-reconciler.log", "File on host for ip-reconciler to log to") | |||
There was a problem hiding this comment.
I don't understand why you are removing the log level parameter. It still makes sense.
Also, you cannot use this default - i.e. what if the cronjob spec does not feature the volume mount ? It will fail.
You should use a default on the container file system, but override that in the daemonset spec shipped in whereabouts.
| @@ -8,7 +8,7 @@ metadata: | |||
| app: whereabouts | |||
| spec: | |||
| concurrencyPolicy: Forbid | |||
| successfulJobsHistoryLimit: 0 | |||
| successfulJobsHistoryLimit: 3 | |||
There was a problem hiding this comment.
If you're now logging to the host's file-system, why would we want to preserve these around ?
|
@maiqueb you're right, I think I can make the log-level configureable in zap/lumberjack... will try adding that. |
|
Thanks for the review Miguel. I'm in favor of this PR, these changes make sense. +1 re: log-level configuration, good to go with that in place. Thanks |
Fix for #198
This solves many logging issues we've had with whereabouts and the ip-reconciler. We already have this working on our custom whereabouts fork we use in production (https://github.com/platform9/whereabouts), would like to get it merged upstream:
ip-reconciler logs are lost once the Pod is terminated. This was made worse by changing the succesful job history to 0 since all the bugs we've seen were when the Job was Succesful. So, have it mount a volume on host and write to a proper log file. This way if container is cleaned up, the logs are still accessible.
whereabouts is using a custom logging package which is a wrapper around Printf. Instead, use a common logger like Zap logger. Also, use lumberjack package for built in file rotation. This works for both whereabouts and new file based ip-reconciler logging in point 1 above, and the logs are rotated automatically.
Yes, the default logging level is debug. I think we need it until this matures as we see a lot of whereabouts and ip-reconciler issues that simply would not be debuggable otherwise.