Glog-ify logging #20
Glog-ify logging #20
Conversation
Locally, I pull first from artifactory which might have a custom version of ubuntu that has tzdata already. The conditional makes extending the use in other environments a lot easier. Also takes care of warning `debconf: delaying package configuration, since apt-utils is not installed` using flag `--no-install-recommends apt-utils` phusion/baseimage-docker#319
Might as well be thorough
- Stability Improvements - No text fixes
To Verify:
apt-get update; \
apt-get install tzdata -y --no-install-recommends apt-utils; \
Versus:
apt-get update && apt-get install tzdata -y --no-install-recommends apt-utils; \
It seems like the single line should work, but I did get an error in a previous dev build
```
Step 2 : RUN if (dpkg -l | grep -cq tzdata); then echo "tzdata package already installed"; else echo "Installing tzdata to avoid go panic caused by missing timezone data" apt-get update && apt-get install tzdata -y --no-install-recommends apt-utils; fi
---> Running in xxx
Installing tzdata to avoid go panic caused by missing timezone data apt-get update
Reading package lists...
Building dependency tree...
Reading state information...
Package apt-utils is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
E: Unable to locate package tzdata
E: Package 'apt-utils' has no installation candidate
The command '/bin/sh -c if (dpkg -l | grep -cq tzdata); then echo "tzdata package already installed"; else echo "Installing tzdata to avoid go panic caused by missing timezone data" apt-get update && apt-get install tzdata -y --no-install-recommends apt-utils; fi' returned a non-zero code: 100
```
Adding checks for tzdata in Dockerfile
Pull from upstream
Modifies the makefile and uses a chain of GNU conditionals to support environments behind firewalls/with proxies. If the environment variables are set, then it will use them. If they are not set then make will run docker build normally. No modifications are necessary in the Dockerfile since Docker has [pre-defined variables](https://docs.docker.com/engine/reference/builder/#predefined-args)
Docker Build Support for Proxies
Pull from upstream
Replace spaces with tab to avoid Makefile syntax failure
Formatted and ordered includes standard libs kube-monkey project k8 and client-go
Where applicable, used glog instead of fmt fmt is still used where an err is returned. when the err is finally printed after the return chain, glog is used to actually print
promote code re-use! same code found in chaos.CreateClient 👍
Before update, glog is defined twice (an extra version in client-go vendor) If you try to run without updating the program fails with the error ``` /kube-monkey flag redefined: log_dir panic: /kube-monkey flag redefined: log_dir ``` This is fixed by running `glide install --strip-vendor`
Copy pasta is hard
Sample on how to configure glog level and recommend logging level
|
What I didn't do what set some constants in a util file and define the v levels programmatically i.e. so instead of hard coded I'm not sure how to do it well in go (I'm thinking in c style code). Most importantly client-go doesn't do that. It hardcodes numbers as well, just do a search. Instead I tried to follow the community conventions |
|
Right now glog uses the standard logger which has a long prefix. I didn't find a way to reduce/customize the standard prefix but it has something to do with changing the LstdFlags |
|
Happy Halloween 🎃 |
|
The hard-coded log levels and prefix is ok. Just having a logging lib is a huge win, so thanks a lot for this. Could you explain why logs need to be redirected to stderr for |
|
glog usually runs with an executable and will output logs to a file. You can specify that file by passing it command line arguments. Background for others:Kube-monkey runs within kubernetes as a pod, deployment whatever. What kubernetes does behind the scene is redirect stdout and stderr to a file on the server that is running the kubernetes cluster. If you try to glog to a file, it will log to a file mounted on the pod! It's not fun to specify another mount point for log files and then have users externally look at those files since kubernetes provides a simple logging solution. Answering your question:The logs could be redirected to stdout but glog doesn't support that afaik (nothing in the docs and searching for glog stdout brings nothing in google and the repo). For the purposes of kube-monkey it practically doesn't matter, in the literal sense of practically; stderr and stdout are both redirected from kubernetes and there's a handy override to send everything to stderr. From the docs When logtostderr is false, |
|
I should specify in my sample output showing The --v=5 does not specify the logging level for kubemonkey, but rather the v log level for For ex: while kubernetes is pulling a new build and is creating the container you'll see this Rather than the minimal |
|
There might be value in making this configurable via kube-monkey configs. In my specific use case, for eg., I would find it valuable to write to file in addition to Could we introduce a |
Info not Errors :)
10 minute test on glog configs for logtostderr and alsologtostderr Results of test --> if alsologtostderr is true, it wiill write to stderr no matter if logtostderr is false and put before or after alsologtostderr temporary logs are stored at the /tmp directory and can be overriden without modifying the configs
10 minute test on glog configs for logtostderr and alsologtostderr Results of test --> if alsologtostderr is true, it will write to logs and stderr regardless of logtostderr. (If logtostderr is not specified, set false, set true, set before alsologtostderr is set, set after alsologtostderr is set)
|
No need to introduce a I first did a 10 minute test on glog configs for logtostderr and alsologtostderr Results of test --> if alsologtostderr is true, it will write to logs and stderr regardless of logtostderr. (If logtostderr is not specified, set false, set true, set before alsologtostderr is set, set after alsologtostderr is set) Verified that the logs are written to the default /tmp directory as specified |
Oops
|
Pulled fresh and passed manual tests with a log dir and without a log dir |
|
Only if the custom path doesn't exist. Since you asked for the ability to
log, I figured you might want to be able to specify a custom log path.
I guess it could be done within go.
…On Nov 1, 2017 3:00 PM, "Ayush Sobti" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Dockerfile
<#20 (comment)>:
> @@ -5,4 +5,5 @@ RUN if (dpkg -l | grep -cq tzdata); then \
echo "Installing tzdata to avoid go panic caused by missing timezone data"; \
apt-get update && apt-get install tzdata -y --no-install-recommends apt-utils; \
fi
+RUN mkdir -p /path/to/custom/log
Do we need this command in the Dockerfile?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACV_i3B8mDWxbHb6nTOjWK4sn6qsfNuMks5syL_EgaJpZM4QNg8q>
.
|
Dockerfile
Outdated
| @@ -5,4 +5,5 @@ RUN if (dpkg -l | grep -cq tzdata); then \ | |||
| echo "Installing tzdata to avoid go panic caused by missing timezone data"; \ | |||
| apt-get update && apt-get install tzdata -y --no-install-recommends apt-utils; \ | |||
| fi | |||
| RUN mkdir -p /path/to/custom/log | |||
There was a problem hiding this comment.
Do we need this command in the Dockerfile?
There was a problem hiding this comment.
Corrected with programmatic creation
examples/deployment.yaml
Outdated
| @@ -15,8 +15,7 @@ | |||
| - name: kube-monkey | |||
| command: | |||
| - "/kube-monkey" | |||
| args: | |||
| - "-v=5" | |||
| args: ["-v=5", "-log_dir=/path/to/custom/log"] | |||
There was a problem hiding this comment.
Since this is meant to be an example as close to something that can be re-used, lets make the log location more standard. say, /var/log/kube-monkey.
main.go
Outdated
| glog.Info("Starting kube-monkey with logging level: ", flag.Lookup("v").Value) | ||
|
|
||
|
|
||
| if _, err := os.Stat(flag.Lookup("log_dir").Value.String()); !os.IsNotExist(err) { |
There was a problem hiding this comment.
If a custom log dir is provided:
- Check if that dir exists
- If not, try to create it
- If that fails, panic and exit program
main.go
Outdated
| // Check commandline options or "flags" for glog parameters | ||
| // to be picked up by the glog module | ||
| flag.Usage = glogUsage | ||
| flag.Parse() | ||
|
|
||
| if _, err := os.Stat(flag.Lookup("log_dir").Value.String()); os.IsNotExist(err) { | ||
| if (os.MkdirAll(flag.Lookup("log_dir").Value.String(), os.ModePerm) != nil) { | ||
| glog.Errorf("Failed to open custom log directory; defaulting to /tmp! Error: %v", flag.Lookup("log_dir").Value, err) |
There was a problem hiding this comment.
You're logging an error here but you were successfully able to create the log_dir
There was a problem hiding this comment.
You mean change to Infof rather than Errorf right.
I was thinking in the Dockerfile framework :) when I was working with it
There was a problem hiding this comment.
Ah, nevermind, I misread that if-condition
|
Voila! It now tries to create the directory programmatically. I verified the new version works and all the logs show up under the custom path. I was worried about the race condition/getting glog to use the newly created directory, but after testing I found out that if the path is created before |
main.go
Outdated
| if (os.MkdirAll(flag.Lookup("log_dir").Value.String(), os.ModePerm) != nil) { | ||
| glog.Errorf("Failed to open custom log directory; defaulting to /tmp! Error: %v", flag.Lookup("log_dir").Value, err) | ||
| } else { | ||
| glog.Errorf("Failed to open custom log directory; attempting to create custom directory! Error: %v", flag.Lookup("log_dir").Value, err) |
There was a problem hiding this comment.
Yeah, lets make this (Line 30) Infof, and since at this point the directory has already been created, can you change the log statement to reflect that? Something like:
glog.Infof("Created custom log directory as ...")
There was a problem hiding this comment.
Did it!
Line 30 is an error though. If kube-monkey doesn't have the permissions to create the directory or if it fails for some other reason, then it will be an error.
main.go
Outdated
| @@ -27,7 +27,7 @@ func initLogging() { | |||
| if (os.MkdirAll(flag.Lookup("log_dir").Value.String(), os.ModePerm) != nil) { | |||
| glog.Errorf("Failed to open custom log directory; defaulting to /tmp! Error: %v", flag.Lookup("log_dir").Value, err) | |||
| } else { | |||
| glog.Errorf("Failed to open custom log directory; attempting to create custom directory! Error: %v", flag.Lookup("log_dir").Value, err) | |||
| glog.V(3).Infof("Failed to open custom log directory; attempting to create custom directory! Error: %v", flag.Lookup("log_dir").Value, err) | |||
There was a problem hiding this comment.
Heh, sorry for being so nit-picky, but on second thoughts, I think its best we not log anything at all in the case that the directory does not exist and we create it successfully. This is basically a routine task and should not be logged. If it is logged, it should be at debug level.
There was a problem hiding this comment.
Uhh I don't think glog for golang has dlog. You can specify a very high verbose level aka 8 or something
There was a problem hiding this comment.
If we don't create it successfully MkdirAll should return an error... which I didn't capture to use for the error log
examples/deployment.yaml
Outdated
| @@ -15,6 +15,7 @@ | |||
| - name: kube-monkey | |||
| command: | |||
| - "/kube-monkey" | |||
| args: ["-v=5", "-log_dir=/path/to/custom/log"] | |||
There was a problem hiding this comment.
One last nit-pick :)
Can we make this "-log_dir=/var/log/kube-monkey".
I want to make sure if someone grabs the example and tries to run it as is, they have some sane defaults.
|
Perfect! Thank you for making all the changes. This PR looks good. I'll run a couple of quick tests in a bit to make sure I can still compile, build etc and then merge this. |
Aergonus
force-pushed
the
glogify
branch
2 times, most recently
from
185f13c to
c3aa6bc
Compare
Where applicable, I changed fmt to use glog instead.
I also redirected glog to stderr so that
kubectl logs kube-monkeywould work.In order to use glog, glide had to be updated with
glide install --strip-vendorsince client-go includes glog in its vendor imports.Also updated the examples to show how to pass the v level of logging in the command line deployment
I also refactored a bit of duplicate code in kubemonkey.