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 dlogf facility for nsexec logging #2020

Closed

Conversation

BooleanCat
Copy link

@BooleanCat BooleanCat commented Mar 19, 2019

Whilst trying to debug a hang in runc exec, we (Garden, Cloud Foundry) were struggling to find useful information to help us with discovering where a hang was occuring.

We noticed that nsexec does not log or have the ability to log any details about what's going on. We propose adding a dlogf (like the printf family of functions) that will log to some fd defined as an ENV by libcontainer, and can be specified as a command line parameter to the runc binary.

We're probably going to patch this into our runc build script (at least temporarily whilst we figure out our hang), but thought this feature might be useful more generally. We deliberately haven't added any use of the dlogf function except an example to show its use as what we want to log is open for discussion and if you're into this idea then we'd love input on what should be logged.

(Our original issue that we're trying to debug is documented here: cloudfoundry/garden-runc-release#121)

Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io>

Signed-off-by: Tom Godkin <tgodkin@pivotal.io>
@cyphar
Copy link
Member

cyphar commented Mar 19, 2019

For the record, I'm super happy to see something like this -- it's something I've been thinking of working on for at least a year or two but have never had the time to sit down and actually do it.

There are plenty of things that right off the bat would be useful to log -- errors all should be logged (bail should be changed so that it actually logs the error rather than printing it to a useless stderr fd`).

All of that being said, I have a few comments:

  • I think that the logs should go to stderr by default not a separate file (or rather, they should be combined with the logrus output of the runc binary -- so that we have only one logging facility in runc). If we also need to add a way to output logrus logs to a file, we can do that too.

  • In line with the above, I would like it if we had leveled logs, so that we can output debug information as well as error-level information from nsenter. But these can be added over time.

I believe @marcov played around with something like this a few months ago, let me see if I can dig it up again.

@yulianedyalkova
Copy link
Contributor

Thanks, @cyphar! Any luck on finding @marcov's work? We are really keen on contributing this feature as we believe it would help us troubleshoot the issue that we face.
cc @danail-branekov

@yulianedyalkova
Copy link
Contributor

We found a runc fork that seems to be the work you mentioned: https://github.com/marcov/runc/commits/child-logging

Do you remember why it did not get merged? Is there something that we need to keep in mind if we want to continue working on it? Are there any known issues that we need to address?

@marcov
Copy link
Contributor

marcov commented Mar 25, 2019

hi @yulianedyalkova, yes that link is the work @cyphar was referring to, there's also a PR for that: #1861.

It's my fault if I never followed up the initial review and if the PR stayed like that until now. That's in part the reason why it never got merged.

For me it would be OK to resume working on that if there's any interest on it and if it's in line to what you need. But I am also totally fine if you prefer pushing forward this PR.

@cyphar
Copy link
Member

cyphar commented Apr 23, 2019

Closing this in favour of #2034.

@cyphar cyphar closed this Apr 23, 2019
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.

4 participants