Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log shim panic logs in containerd. #951

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

ambarve
Copy link
Contributor

@ambarve ambarve commented Feb 23, 2021

Currently hcsshim writes the shim panic logs in a file named panic.log inside the sandbox
directory. However, those logs are never logged in containerd and they get lost when the
sandbox container is removed. This change allows the shim to log these panic logs to
containerd before deleting them.

Signed-off-by: Amit Barve [email protected]

@ambarve ambarve requested a review from a team as a code owner February 23, 2021 20:14
@dcantah
Copy link
Contributor

dcantah commented Feb 23, 2021

Do we no longer need a containerd PR here?

@ambarve
Copy link
Contributor Author

ambarve commented Feb 23, 2021

Nope. Whatever is written on the stderr of the shim binary delete instance will be logged by containerd automatically.

@dcantah
Copy link
Contributor

dcantah commented Feb 23, 2021

Sweet ok!

@kevpar
Copy link
Member

kevpar commented Feb 24, 2021

Can you make sure you have tested by inserting panic at various points in the code, and made sure the stack is correctly picked up by delete and re-logged via ETW? We should be able to see an event from the runhcs ETW provider with the panic stack.

@ambarve ambarve force-pushed the shim_panic_logs branch 2 times, most recently from 8140e5b to 2b171cb Compare February 25, 2021 00:01
Currently hcsshim writes the shim panic logs in a file named panic.log inside the sandbox
directory. However, those logs are never logged in containerd and they get lost when the
sandbox container is removed. This change allows the shim to log these panic logs to
containerd before deleting them.

Signed-off-by: Amit Barve <[email protected]>
@ambarve
Copy link
Contributor Author

ambarve commented Feb 26, 2021

@kevpar I have tested this by adding panic at different locations in hcsshim code and verified that the panic log shows up in both ETW and containerd events. Can you please take another look?

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If testing is good then LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants