Change os.Stderr reassign for Windows service#7023
Conversation
|
This is intended to resolve #6990 |
jterry75
left a comment
There was a problem hiding this comment.
Obviously not part of this CR, but should the file be handled by some sort of log rotate utility like we do for CRI instead of a flat file handle? Then even if it grows over time the caller has the ability to remove the older ones. Even in this case they would hit the same issue just not on the panic.log file now it would be on the actual log file right?
|
@jterry75 Right, I think that's the ideal but like you kind of alluded to I just wanted to get shim logs out of panic.log for now with this change |
|
Np sounds good |
|
@kevpar ping |
|
ping @kevpar |
|
Sorry, been meaning to get to this one... I created a table to summarize the behavior here, both before and after the change: I think we could do with a comment in service_windows.go explaining overall what happens with output when run as a service (e.g. what's in this table), since it seems kind of unintuitive. One thing I'm wondering: When run as a service I don't think stderr goes anywhere useful by default. Would it make this code easier to read to explicitly set Otherwise I think this seems reasonable. This will keep [1] Of course, if any other code in containerd specifically queries |
Will add
Sure that's a good idea, better to be explicit. Edit: Well to be clear, we can't assign os.Stderr = io.Discard directly as os.Stderr is an os.File and io.Discard is just some Writer. We could open os.DevNull which just points to NUL on windows and assign it to os.Stderr I guess |
c952620 to
af22760
Compare
|
@kevpar Updated! PTAL |
Previously we were reassigning os.Stderr to the panic.log file we create when getting asked to run Containerd as a Windows service. The panic.log file was used as a means to easily collect panic stacks as Windows services don't have regular standard IO, and the usual recommendation is to either write to the event log or just to a file in the case of running as a service. One place where this panic.log flow was biting us was with shim logging, which is forwarded from the shim and copied to os.Stderr directly which was causing shim logs to get forwarded to this panic.log file instead of just panics. We expose an additional `--log-file` flag if you ask to run a windows service which is the main way you'd get Containerd logs, and with this change all of the shim logging which would today end up in panic.log will now also go to this log file. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
af22760 to
ee0f2e9
Compare
|
Linux CI failures are definitely unrelated and the node-e2e failed on some preamble for fetching package lists |
|
/retest Edit: Nice they passed |
Previously we were reassigning os.Stderr to the panic.log file we create
when getting asked to run Containerd as a Windows service. The panic.log
file was used as a means to easily collect panic stacks as Windows
services don't have regular standard IO, and the usual recommendation
is to either write to the event log or just to a file in the case of
running as a service.
One place where this panic.log flow was biting us was with shim logging,
which is forwarded from the shim and copied to os.Stderr directly which was
causing shim logs to get forwarded to this panic.log file instead of just
panics. We expose an additional
--log-fileflag if you ask to run awindows service which is the main way you'd get Containerd logs, and with
this change all of the shim logging which would today end up in panic.log
will now also go to this log file.