Don't stop the agent when journald is stopped or crashes by remicalixte · Pull Request #2416 · DataDog/datadog-agent

remicalixte · 2018-10-04T20:37:36Z

What does this PR do?

By default systemd redirect the stdout to journald. When journald is stopped or crashes we receive a SIGPIPE signal. Go ignore SIGPIPE signals unless it is when stdout or stdout is closed, in this case the agent is stopped. We don't want that so we intercept the SIGPIPE signals and just discard them using the new --ignore-sigpipe flag

Other small fix, don't display the absolute path of the source files when using syslog:
before:

Oct 04 20:41:03 ubuntu-xenial agent[22041]: INFO | (/go/src/github.com/DataDog/datadog-agent/.omnibus/src/datadog-agent/src/github.com/DataDog/datadog-agent/pkg/collector/runner/runner.go:258) | Running check memory

after:

Oct 04 20:42:06 ubuntu-xenial agent[22501]: INFO | (runner.go:258) | Running check memory

Motivation

Fix #1555

remicalixte · 2018-10-05T17:15:25Z

This issue also affects the trace agent, we should fix it for this one too. The process agent is not affected since it doesn't output anything to stdout but it might be safer to do the same thing for it too

nmuesch · 2018-10-09T12:43:53Z

Hey! Thanks for this @remicalixte It seems I'm currently hitting this issue in one of my environments. In my case it seems like its the Trace Agent thats experiencing the issue. Would it be possible to extend this to Trace and Process as well?

palazzem · 2018-10-10T13:21:45Z

On APM side, we're waiting for your review before merging DataDog/datadog-trace-agent#486

arbll

LGTM. Just a question by curiosity: why don't just ignore SIGPIPE by default ?

gbbr · 2018-10-11T12:11:05Z

Good question. Do we need the flag?

remicalixte · 2018-10-11T14:17:01Z

I wanted to change the default behavior in the fewest cases possible. With the flag we don't ignore SIGPIPE when we run the datadog-agent status command for example, and stopping when we receive a SIGPIPE is the behavior of many unix command line programs. There is a bit of info there: https://golang.org/pkg/os/signal/#pkg-overview.

olivielpeau

just a question, disregard if it's not relevant

arbll · 2018-10-11T16:16:09Z

The current implementation is scoped to run & start so it would not interfere with the other commands. I think the question is if there is a case where we want to exit a running agent when we get a SIGPIPE for stdout or stderr. I would answer no but I might be missing something.

Regardless both options are fine for me

remicalixte · 2018-10-11T16:47:27Z

Oh yes you're right. I guess the flag is not really necessary then...

remicalixte · 2018-10-11T17:32:32Z

The process agent is already ignoring all signals for example. I think it's fine to always ignore SIGPIPEs

olivielpeau

Looks good, just a few nits :)

olivielpeau · 2018-10-11T18:28:43Z

 		}
 	}()

+	// By default systemd redirect the stdout to journald. When journald is stopped or crashes we receive a SIGPIPE signal.


nit: redirects

olivielpeau · 2018-10-11T18:31:07Z

 	}()

+	// By default systemd redirect the stdout to journald. When journald is stopped or crashes we receive a SIGPIPE signal.
+	// Go ignore SIGPIPE signals unless it is when stdout or stdout is closed, in this case the agent is stopped. We don't want that


nit: ignores. Also, it's not very clear what we don't want that refers to, maybe this would be clearer: We never want the agent to stop upon receiving SIGPIPE, so [...]

olivielpeau · 2018-10-11T18:34:05Z

 		<format id="common" format="%%Date(%s) | %%LEVEL | (%%File:%%Line in %%FuncShort) | %%Msg%%n"/>
 		<format id="syslog-json" format="%%CustomSyslogHeader(20,`+strconv.FormatBool(rfc)+`){&quot;level&quot;:&quot;%%LEVEL&quot;,&quot;relfile&quot;:&quot;%%RelFile&quot;,&quot;line&quot;:&quot;%%Line&quot;,&quot;msg&quot;:&quot;%%Msg&quot;}%%n"/>
-		<format id="syslog-common" format="%%CustomSyslogHeader(20,`+strconv.FormatBool(rfc)+`) %%LEVEL | (%%RelFile:%%Line) | %%Msg%%n" />
+		<format id="syslog-common" format="%%CustomSyslogHeader(20,`+strconv.FormatBool(rfc)+`) %%LEVEL | (%%File:%%Line) | %%Msg%%n" />


to give slightly more context, we use (%%File:%%Line in %%FuncShort) for agent.log (for ex: somefile.go:23 in myFunction), I think we should normalize to the latter unless there's a good reason not to

Yes I agree

olivielpeau

👌👌

remicalixte requested a review from a team as a code owner October 4, 2018 20:37

remicalixte added the [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. label Oct 4, 2018

remicalixte added this to the 6.6.0 milestone Oct 4, 2018

remicalixte changed the title ~~Don't stop the agent when journald is stopped or crash~~ Don't stop the agent when journald is stopped or crashes Oct 4, 2018

remicalixte added the kind/bug label Oct 5, 2018

remicalixte mentioned this pull request Oct 9, 2018

cmd/trace-agent: ignore SIGPIPE signal DataDog/datadog-trace-agent#486

Merged

remicalixte force-pushed the remicalixte/fix-journald branch from d6fbcf0 to 3fa214a Compare October 10, 2018 14:05

arbll previously approved these changes Oct 11, 2018

View reviewed changes

olivielpeau reviewed Oct 11, 2018

View reviewed changes

Comment thread cmd/agent/app/run.go Outdated

remicalixte dismissed arbll’s stale review via e769708 October 11, 2018 17:38

remicalixte force-pushed the remicalixte/fix-journald branch from 3fa214a to e769708 Compare October 11, 2018 17:38

remicalixte added 3 commits October 11, 2018 13:51

Reduce line length when using syslog

d5fb4c3

Intercept SIGPIPE on unix

07e3535

Add release notes

0be6e0b

remicalixte force-pushed the remicalixte/fix-journald branch from e769708 to 0be6e0b Compare October 11, 2018 17:52

olivielpeau previously approved these changes Oct 11, 2018

View reviewed changes

remicalixte added 2 commits October 11, 2018 14:49

Grammar issues

894ef76

Normalize logs output

064c450

remicalixte dismissed olivielpeau’s stale review via 064c450 October 11, 2018 18:49

olivielpeau approved these changes Oct 11, 2018

View reviewed changes

remicalixte merged commit d3e291a into master Oct 11, 2018

remicalixte deleted the remicalixte/fix-journald branch October 11, 2018 19:05

Uh oh!

Conversation

remicalixte commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Uh oh!

remicalixte commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmuesch commented Oct 9, 2018

Uh oh!

palazzem commented Oct 10, 2018

Uh oh!

arbll left a comment

Choose a reason for hiding this comment

Uh oh!

gbbr commented Oct 11, 2018

Uh oh!

remicalixte commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivielpeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arbll commented Oct 11, 2018

Uh oh!

remicalixte commented Oct 11, 2018

Uh oh!

remicalixte commented Oct 11, 2018

Uh oh!

olivielpeau left a comment

Choose a reason for hiding this comment

Uh oh!

olivielpeau Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

olivielpeau Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

olivielpeau Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

remicalixte Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

olivielpeau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

remicalixte commented Oct 4, 2018 •

edited

Loading

remicalixte commented Oct 5, 2018 •

edited

Loading

remicalixte commented Oct 11, 2018 •

edited

Loading