Added support for write-timeout support to fluentd plugin. by imumesh18 · Pull Request #35716 · moby/moby

imumesh18 · 2017-12-06T09:32:28Z

Signed-off-by: dungeonmaster18 [email protected]

- What I did

I will be adding write-timeout support for fleuntd plugin
I have replaced the dependency in vendor.conf from commit id with vesion tag which are related fluentd plugin.

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: dungeonmaster18 <[email protected]>

imumesh18 · 2017-12-06T12:44:36Z

Hey @thaJeztah and @cpuguy83 . I have added support for write timeout. Please review it whenever you have time. Also I have replaced the commit id with version tag in vendor.conf for all the libraries used by fluentd plugin.

P.S Once you guys think this is good just ping me I will start the work on docs and auto complete command for bash and zsh. Also I will squash the commits.

Thanks

thaJeztah · 2017-12-06T20:11:20Z

daemon/logger/fluentd/fluentd.go

+		wtd, err := time.ParseDuration(info.Config[writeTimeoutKey])
+		if err != nil {
+			return nil, err
+		}


Should we check for negative durations and produce an error?

Also, we need to check what status-code is returned by the API in case of an invalid value (haven't checked yet) (make sure it doesn't return a 5xx error)

We should also check for extremely long timeout values to prevent daemon waiting on the write.

So @anusha-ragunathan how long do you thing it should wait. I was thinking of something in minutes.

@thaJeztah Actually I am completely new to these things so I am kind of learning how can I handle server error(5xx). It will take time but I will push the changes by next week. Actually I am bit busy with my college exams 🙈

@dungeonmaster18 I think logger has ring buffer for this case. @cpuguy83

As far I know ring buffer is nothing but the circular buffer with fixed size. Correct me if I am wrong @ripcurld0

@dungeonmaster18 : regarding setting an upper limit on the timeout, lets skip that. Users should be aware of what they are setting for the timeout value. It's hard to predict a good max value. Also, I checked other timeouts. Other than disallowing negative values, there's no other check.

For the status-code; The API uses this code https://github.com/moby/moby/blob/master/api/server/httputils/errors.go#L33-L73 (and code from the errdefs package: https://github.com/moby/moby/tree/master/api/errdefs) to convert errors from the daemon into the correct HTTP status codes. You can try creating an API request with an incorrect value for the timeout, and check what status code is returned.

thaJeztah · 2017-12-06T20:13:52Z

ping @anusha-ragunathan PTAL

cpuguy83 · 2017-12-06T21:19:47Z

daemon/logger/fluentd/fluentd.go

 	defaultMaxRetries = math.MaxInt32

+	// Write() will not time out
+	defaultWriteTimeout = time.Duration(0)


We should still set a default timeout. The main thing we don't want to happen is to allow a dead logging backend lock up the daemon in some way.

How about 300ns?

I'd do something much higher, we don't want to drop logs.
30s timeout should be fine.

sounds good to me.

imumesh18 · 2017-12-16T16:02:22Z

PTAL @thaJeztah @anusha-ragunathan @cpuguy83.

P.S should I address more 5xx errors. Right now I am only checking for 501 502 503. Would like to know your views on this?

cpuguy83 · 2017-12-16T16:17:44Z

daemon/logger/fluentd/fluentd.go

I'm not sure I understand this error handling. time.ParseDuration shouldn't be making an HTTP request.

yup it's wrong I realized later 🙈 .

I am bit confused with the http status code. Where should I be checking it.

Probably https://github.com/moby/moby/pull/35716/files#diff-4e76640044eca60a4a45cd8631049282R108 needs a properly typed error (of type InvalidParameter)

How about InvalidParameter: write-timeout must be a positive integer @cpuguy83

It's not the error message that matters so much as the error type, which needs to implement this interface: https://github.com/moby/moby/blob/master/api/errdefs/defs.go#L9

btw, this can just use errdefs.InvalidParameter(errors.New("some error")) now.

Signed-off-by: dungeonmaster18 <[email protected]>

anusha-ragunathan · 2018-04-12T17:07:28Z

daemon/logger/fluentd/fluentd.go

+	writeTimeout := defaultWriteTimeout
+	i, _ := strconv.Atoi(info.Config[writeTimeoutKey])
+	if info.Config[writeTimeoutKey] != "" && i >= 0 {
+		wtd, err := time.ParseDuration(info.Config[writeTimeoutKey])


You can directly assign to writeTimeout

anusha-ragunathan · 2018-04-12T17:16:58Z

Needs follow up PRs for:

docs update in https://github.com/docker/docker.github.io/blob/1b343beca4aaab8b183eefa89867b6bf64505be5/config/containers/logging/fluentd.md#options
auto completion update required in https://github.com/docker/cli/blob/6d7c23a6b46f9f0b48ecc43c4c7a37a55dc2660e/contrib/completion/bash/docker (also for zsh)

thaJeztah · 2018-04-23T15:44:33Z

ping @dungeonmaster18 could you have a look at the review comments? Looks like the change to use the errdefs #35716 (comment) is still current

olljanat · 2018-12-22T10:13:37Z

@imumesh18 CI build fails to:

18:38:29 daemon/logger/fluentd/fluentd.go:52:37:warning: unnecessary conversion (unconvert)
18:38:30 Build step 'Execute shell' marked build as failure

And there is reviews above which needs to be addressed

akerouanton · 2019-04-08T09:27:51Z

Looks like this PR is stall for quite some time now. Would you mind if I resubmit it with review comments fixed? We actually got hit by a bug fixed here: as mentioned by cpuguy83 in this comment, having no default timeout value leads to daemon lock up whenever fluentd times out, thus producing the same symptoms as described in #38092 (non-blocking mode fixes the issue through).

Updated dependency with version tag

0b5e38a

Signed-off-by: dungeonmaster18 <[email protected]>

GordonTheTurtle added the status/0-triage label Dec 6, 2017

imumesh18 changed the title ~~Add support for write-timeout support to fluentd plugin.~~ Added support for write-timeout support to fluentd plugin. Dec 6, 2017

Added support for write timeout for fluentd plugin

7233a50

Signed-off-by: dungeonmaster18 <[email protected]>

thaJeztah added area/logging kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/1-design-review and removed status/0-triage labels Dec 6, 2017

thaJeztah reviewed Dec 6, 2017

View reviewed changes

cpuguy83 requested changes Dec 6, 2017

View reviewed changes

cpuguy83 requested changes Dec 16, 2017

View reviewed changes

Addressed comments!

989a19e

Signed-off-by: dungeonmaster18 <[email protected]>

GordonTheTurtle assigned aluzzardi Dec 21, 2017

vdemeester unassigned aluzzardi Feb 16, 2018

GordonTheTurtle assigned MHBauer Feb 17, 2018

anusha-ragunathan reviewed Apr 12, 2018

View reviewed changes

derek bot added the rebuild/* label Dec 21, 2018

GordonTheTurtle removed the rebuild/* label Dec 21, 2018

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

akerouanton mentioned this pull request Nov 9, 2021

vendor: github.com/fluent/fluent-logger-golang v1.8.0 #42979

Merged

imumesh18 closed this Apr 25, 2025

imumesh18 deleted the write-timeout-config branch April 25, 2025 11:51

Conversation

imumesh18 commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imumesh18 commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imumesh18 commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anusha-ragunathan commented Apr 12, 2018

Uh oh!

thaJeztah commented Apr 23, 2018

Uh oh!

olljanat commented Dec 22, 2018

Uh oh!

akerouanton commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

imumesh18 commented Dec 6, 2017 •

edited

Loading

imumesh18 commented Dec 6, 2017 •

edited

Loading

akerouanton commented Apr 8, 2019 •

edited

Loading