Moves runc logs to per container id #139
Conversation
|
@jterry75, |
| BeforeEach(func() { | ||
| expectedPath = "/var/run/gcsrunc/log.log" | ||
| expectedPath = "/tmp/gcs/" + id + "runc.log" |
There was a problem hiding this comment.
You'll probably notice this when the tests are done, but I think there should be a slash before runc.log here.
There was a problem hiding this comment.
Ha. Yea your right. I wish I could run these locally
service/gcs/runtime/runc/utils.go
Outdated
| func (r *runcRuntime) getLogPath() string { | ||
| return filepath.Join(containerFilesDir, "log.log") | ||
| func (r *runcRuntime) getLogPath(id string) string { | ||
| return filepath.Join("/tmp/gcs", id, "runc.log") |
There was a problem hiding this comment.
nit: It would be nice to have "/tmp/gcs" in some constant somewhere, either by having an actual constant or using getStorageRootPath()
|
doesn't this require changes to https://github.com/Microsoft/opengcs/blob/master/client/process.go#L139 as well? |
|
@rn - good point |
|
My reasoning for this change was that it would be good to have runc logs separated out by container because when debugging a failure, you'd probably want to localize your search to individual containers at a time rather than every piece of logging done by runc for all containers. This is less relevant now, and more useful when we support multiple containers per uvm. Also, this change allows all logs to be coalesced a bit under @jhowardmsft, I know you were also enthusiastic about this change :). You know a lot more about this from the docker end than I do, so I'm not sure what your thoughts are. |
|
If I'm understanding the comments above correctly, it should be a straightforward change to the debug code to just cat the right log. Remember the daemon only runs a single container in a utility VM (even though we support >1 through HCS). |
|
@jhowardmsft - The issue is that we don't actually know the container id in the debug code. If we move it from a global location we could certainly |
|
I'm already one step ahead of you :) Spoke in person..... |
|
Added an extra call to make the container dir for the tests but in most cases this won't do anything. Now the tests are passing and the PR should be good for final review. |
| @@ -136,7 +136,8 @@ func (config *Config) DebugGCS() { | |||
| cmd += debugCommand("ls -l /tmp/gcs/*") | |||
| cmd += debugCommand("cat /tmp/gcs/*/config.json") | |||
| cmd += debugCommand("ls -lR /var/run/gcsrunc") | |||
There was a problem hiding this comment.
This should probably change too now.... (I don't think /var/run/gcs* is used anymore)
There was a problem hiding this comment.
It still is used. We write the pid file there for create container/process
There was a problem hiding this comment.
It isn't used for logs anymore, though.
There was a problem hiding this comment.
I agree but isn't it usefully to get an output of all the pid files? IE: all the running process in runc?
There was a problem hiding this comment.
Sure. The more the merrier :)
There was a problem hiding this comment.
Alternatively I can move the use of this dir to be /tmp/gcs/(id)/ too?
There was a problem hiding this comment.
If we don't care about the runc wrapper being a generic wrapper that can be used outside of this project, I think it makes sense to merge it as much as possible with the GCS. Also, I guess in addition to pid files there are also process.json files under /var/run/gcsrunc, which could definitely be useful for logging.
There was a problem hiding this comment.
@beweedon - Want to open an issue for merging the two then? I don't see any use cases for a runc wrapper library. Its primarily just for us and how we use runc
|
@soccerGB - anything left for me to do here? |
Resolves: #130