fluentd logger: support all options besides Unix sockets #19439

pcarrier · 2016-01-19T08:44:53Z

Mostly useful for #19438.

wenchma · 2016-01-19T09:01:33Z

daemon/logger/fluentd/fluentd.go


 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/logger/loggerutils"
 	"github.com/fluent/fluent-logger-golang/fluent"
+	"github.com/fsouza/go-dockerclient/external/github.com/docker/docker/pkg/units"


this import is strange, you made a lot of detours, and the pkg doesn't exist

Sorry, my IDE did something weird. Fixed.

calavera · 2016-01-21T16:20:32Z

Small change, otherwise LGTM

pcarrier · 2016-01-23T01:12:40Z

@calavera addressed, thanks.

icecrime · 2016-01-26T06:18:04Z

Ping @LK4D4!

calavera · 2016-01-29T19:53:11Z

This needs a rebase 😭

pcarrier · 2016-01-30T00:05:17Z

AFAICT this is because of 3cf82ff, which tries to solve the same problem in what I'd say isn't as good a solution. My solution would be to revert it.

cpuguy83 · 2016-01-30T02:14:30Z

vendor/src/github.com/fluent/fluent-logger-golang/fluent/fluent.go

-	f.conn, err = net.DialTimeout("tcp", f.Config.FluentHost+":"+strconv.Itoa(f.Config.FluentPort), f.Config.Timeout)
+	switch f.Config.FluentNetwork {
+	case "tcp":
+		f.conn, err = net.DialTimeout(f.Config.FluentNetwork, f.Config.FluentHost+":"+strconv.Itoa(f.Config.FluentPort), f.Config.Timeout)


Can we move this host/port vs sock path into a helper function?
This way connect is just a single case and can potentially support other formats easily as well (e.g. "unixgram")

For that matter, seems like we should just store host/port as a single field anyway.

This is a third-party library. I suppose we don't want to fork it.

oh damn, didn't notice we were in vendor here.

pcarrier · 2016-02-02T18:10:21Z

@calavera @icecrime @LK4D4 what next?

cpuguy83 · 2016-02-02T18:15:34Z

daemon/logger/fluentd/fluentd.go

+
+	defaultHost        = "127.0.0.1"
+	defaultPort        = 24224
+	defaultBufferLimit = 8 * 1024 * 1024


I'd personally prefer to keep this @ a 1MB buffer.
Consider you have 100 containers (which is not much) each with an 8MB buffer

This would be changing the current buffer size. Happy to adjust, if you confirm we don't want stable "defaults" (technically it wasn't a default, it was hardcoded...).

Also note that the dehaviour when exceeding the buffer size is pretty damn aggressive: the whole buffer is discarded. Again, third-party library.

It looks better to keep it as 1MB. Users can change it if they want.

To be clear, the last docker release is at 8MB AFAICT. I'm OK going with 1MB if we want, but that would be a change AFAICT.

That is already discussed on #18041.
Please respect previous commits.

Feb 2, 2016 · 2016-02-02T21:21:38Z

ping @tagomoris

tagomoris · 2016-02-03T04:33:58Z

daemon/logger/fluentd/fluentd.go

@@ -114,8 +162,6 @@ func ValidateLogOpt(cfg map[string]string) error {
 		case "tag":
 		case "labels":
 		case "env":
-		case "fail-on-startup-error":
-		case "buffer-limit":


Is it ok to just remove these options, without adding options newly added? (fluentd-buffer-limit, fluentd-retry-limit and others).

tagomoris · 2016-02-03T04:43:43Z

I added some comments.
And I've just released fluent-logger-golang v1.1.0 with async connect and unix domain support options. It may be better version to be vendored.

Feb 10, 2016 · 2016-02-10T21:27:31Z

@pcarrier feel free to update your PR.

thaJeztah · 2016-02-15T14:19:17Z

ping @pcarrier can you address the comments? Trying to move things forward here

pcarrier · 2016-02-15T16:20:58Z

On it.

thaJeztah · 2016-02-15T16:40:33Z

ping @cpuguy83 @LK4D4 PR's was updated, PTAL

@pcarrier will update the fluentd.md file with the new options during docs-review

(Thanks for updating @pcarrier!)

icecrime · 2016-02-29T03:30:13Z

test this please

icecrime · 2016-02-29T03:30:28Z

Ping @SvenDowideit.

pcarrier · 2016-03-19T19:46:11Z

Sorry for the slow response, been busy with other tasks. On it.

This reverts commit 3cf82ff. Signed-off-by: Pierre Carrier <[email protected]>

This version supports async connections. Signed-off-by: Pierre Carrier <[email protected]>

GordonTheTurtle · 2016-03-20T16:31:08Z

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "pcarrier/19438-async-connections-to-fluentd" [email protected]:pcarrier/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~4
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

pcarrier · 2016-03-20T16:34:07Z

I don't understand.

% git log origin/master..pcarrier/19438-async-connections-to-fluentd
commit 7a29366b075f3b3cd7bc955a08afea2a6918c79c
Author: Pierre Carrier <[email protected]>
Date:   2016-03-20 16:30:46 +0000

    docs: fluentd-async-connect option

    Signed-off-by: Pierre Carrier <[email protected]>

commit d48eab75110f580022ff6af308241b4ce19957d1
Author: Pierre Carrier <[email protected]>
Date:   2016-01-24 05:51:03 -0800

    fluentd logger: support all options besides Unix sockets

    Mostly useful for docker/docker#19438.

    Signed-off-by: Pierre Carrier <[email protected]>

commit 9c2a0739fb9c5e3636425d669f0ab860ea1bfbae
Author: Pierre Carrier <[email protected]>
Date:   2016-01-19 00:07:13 -0800

    bump fluent-logger-golang version

    This version supports async connections.

    Signed-off-by: Pierre Carrier <[email protected]>

commit d89dae6e4becc16e80e3781ac68b9fbb855947b3
Author: Pierre Carrier <[email protected]>
Date:   2016-01-29 16:05:46 -0800

    Revert "Added flag to ignore fluentd connect error on container start"

    This reverts commit 3cf82ff1ab14e1ddd2b629524e894ac359168388.

    Signed-off-by: Pierre Carrier <[email protected]>

icecrime · 2016-03-21T00:06:41Z

Sorry @pcarrier, the commit do indeed look signed, I don't understand why the CI job is complaining (cc @thaJeztah). I guess we could ignore?

thaJeztah · 2016-03-21T09:35:22Z

docs LGTM, can you squash the last two commits?

thaJeztah · 2016-03-21T09:38:20Z

windows hitting flaky test #20922, restarting

Mostly useful for #19438. Signed-off-by: Pierre Carrier <[email protected]>

pcarrier · 2016-03-21T10:03:45Z

@thaJeztah done :)

thaJeztah · 2016-03-21T10:05:57Z

cool, thanks, and looks like the "dco" check now also passed

LGTM

ping @vdemeester PTAL

vdemeester · 2016-03-21T10:34:11Z

LGTM 🐰

thaJeztah · 2016-03-21T10:52:41Z

Hm documentation fail looks to be a known issue in Hugo, because;

Found: 1119 files
Found: 0 errors

thaJeztah · 2016-03-21T10:52:48Z

retest this please

thaJeztah · 2016-03-21T12:31:21Z

gccgo is hitting #21365 😢 restarting

…ons-to-fluentd fluentd logger: support all options besides Unix sockets

GordonTheTurtle added the status/0-triage label Jan 19, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Jan 19, 2016

pcarrier changed the title ~~fluentd lgoger: support all options besides Unix sockets~~ fluentd logger: support all options besides Unix sockets Jan 19, 2016

wenchma reviewed Jan 19, 2016
View reviewed changes

calavera added status/2-code-review and removed status/1-design-review labels Jan 21, 2016

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jan 30, 2016

cpuguy83 reviewed Jan 30, 2016
View reviewed changes

cpuguy83 reviewed Feb 2, 2016
View reviewed changes

tagomoris reviewed Feb 3, 2016
View reviewed changes

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Feb 15, 2016

icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 29, 2016

calavera added the impact/cli label Mar 17, 2016

Pierre Carrier added 2 commits March 20, 2016 16:22

Revert "Added flag to ignore fluentd connect error on container start"

d89dae6

This reverts commit 3cf82ff. Signed-off-by: Pierre Carrier <[email protected]>

bump fluent-logger-golang version

9c2a073

This version supports async connections. Signed-off-by: Pierre Carrier <[email protected]>

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 20, 2016

fluentd logger: support all options besides Unix sockets

13086f3

Mostly useful for #19438. Signed-off-by: Pierre Carrier <[email protected]>

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 21, 2016

vdemeester added status/2-code-review status/4-merge and removed status/3-docs-review status/2-code-review labels Mar 21, 2016

thaJeztah added this to the 1.11.0 milestone Mar 21, 2016

vdemeester added a commit that referenced this pull request Mar 21, 2016

Merge pull request #19439 from pcarrier/pcarrier/19438-async-connecti…

d82ad12

…ons-to-fluentd fluentd logger: support all options besides Unix sockets

vdemeester merged commit d82ad12 into moby:master Mar 21, 2016

albers mentioned this pull request Mar 25, 2016

bash completion for new fluentd log driver options #21515

Merged

sdurrheimer mentioned this pull request Mar 25, 2016

Add zsh completion for new fluentd log driver options #21516

Merged

This was referenced Apr 20, 2016

Support async connections to fluent servers #19438

Closed

Unable to start containers when syslog endpoint unavailable #22129

Closed

fluentd logger: support all options besides Unix sockets #19439

fluentd logger: support all options besides Unix sockets #19439

Conversation

pcarrier commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calavera commented Jan 21, 2016

pcarrier commented Jan 23, 2016

icecrime commented Jan 26, 2016

calavera commented Jan 29, 2016

pcarrier commented Jan 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcarrier commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tagomoris commented Feb 3, 2016

thaJeztah commented Feb 15, 2016

pcarrier commented Feb 15, 2016

thaJeztah commented Feb 15, 2016

icecrime commented Feb 29, 2016

icecrime commented Feb 29, 2016

pcarrier commented Mar 19, 2016

GordonTheTurtle commented Mar 20, 2016

pcarrier commented Mar 20, 2016

icecrime commented Mar 21, 2016

thaJeztah commented Mar 21, 2016

thaJeztah commented Mar 21, 2016

pcarrier commented Mar 21, 2016

thaJeztah commented Mar 21, 2016

vdemeester commented Mar 21, 2016

thaJeztah commented Mar 21, 2016

thaJeztah commented Mar 21, 2016

thaJeztah commented Mar 21, 2016