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

Add support for writing stacks with SIGUSR1 #1835

Merged
merged 3 commits into from
Oct 11, 2018

Conversation

mrunalp
Copy link
Member

@mrunalp mrunalp commented Oct 8, 2018

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2018
@mrunalp
Copy link
Member Author

mrunalp commented Oct 8, 2018

/test all

@mrunalp
Copy link
Member Author

mrunalp commented Oct 8, 2018

/test kata-containers

@mrunalp
Copy link
Member Author

mrunalp commented Oct 9, 2018

/test e2e_rhel

@mrunalp
Copy link
Member Author

mrunalp commented Oct 9, 2018

/test kata-containers

@runcom
Copy link
Member

runcom commented Oct 9, 2018

Kata still complaining :( @sboeuf

cmd/crio/main.go Outdated
go func() {
for s := range sig {
switch s {
case syscall.SIGUSR1:
logrus.Debugf("Caught SIGUSR1")
if err := utils.WriteGoroutineStacks(os.Stderr); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

actually, hold on, let's do what docker does, I remember logging to stderr was a pain because of journald stripping stuff, and that got changed to log to a file eventually.

It's actually super easy and we can use:

https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/trap.go#L61
https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/daemon/debugtrap_unix.go

Copy link
Member

@runcom runcom left a comment

Choose a reason for hiding this comment

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

let's log to file instead of stderr

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
Copy link
Member Author

mrunalp commented Oct 9, 2018

@runcom Updated.

@mrunalp
Copy link
Member Author

mrunalp commented Oct 9, 2018

/test all

@sboeuf
Copy link

sboeuf commented Oct 11, 2018

/test kata-containers

@sboeuf
Copy link

sboeuf commented Oct 11, 2018

@runcom @mrunalp sorry with the inconsistent Kata CI here... We have some problems with the type of VM being assigned to our CI, meaning that sometimes they don't have the expected nested support, and we simply cannot run Kata on them.

@mrunalp
Copy link
Member Author

mrunalp commented Oct 11, 2018

@sboeuf okay, thanks for the info!

@mrunalp
Copy link
Member Author

mrunalp commented Oct 11, 2018

/test kata-containers

1 similar comment
@runcom
Copy link
Member

runcom commented Oct 11, 2018

/test kata-containers

@runcom
Copy link
Member

runcom commented Oct 11, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2018
@k8s-ci-robot k8s-ci-robot merged commit f219ebb into cri-o:release-1.11 Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants