Fluentd: add fluentd-async, fluentd-request-ack, and deprecate fluentd-async-connect #39086

thaJeztah · 2019-04-16T11:01:34Z

~~Follow up to #39074 (first two commits are from that PR; I'll rebase once that's merged)~~ (rebased)

closes #40332

Some minor refactoring; the driver now not only validates if options are supported, but also if their value can be parsed (producing an error earlier, instead of when instantiating a driver)
Add fluentd-async option, deprecate fluentd-async-connect. This ia a rename of the old option. The old option remains functional, but when used will log a deprecation warning in fluentd. If both the new, and old option is set, a "conflicting options" error is produced. This change is not versioned, and affects all API versions.
Add fluentd-request-ack option. This adds a new fluentd-request-ack logging option for the Fluentd logging driver. If enabled, the server will respond with an acknowledgement. This option improves the reliability of the message transmission. This change is not versioned, and affects all API versions.

thaJeztah · 2019-04-16T11:06:22Z

Ping @tagomoris @cpuguy83 PTAL

@tagomoris I noticed two more options that are in the driver, but not exposed through the docker API;

fluent/fluent-logger-golang@70eca0b (Async send fluent/fluent-logger-golang#56); New configuration directive MaxRetryWait. Looking at that option, it would likely only make sense as a daemon option; not sure if it's worth adding though (as we already have fluentd-retry-wait and that option would only limit/restrict its maximum value
Add JSON marshaler fluent/fluent-logger-golang#27 Add JSON marshaler (Add MarshalAsJSON option and marshal as forward protocol adopted JSON). Looks like that option was around already, but also not exposed through the Docker API; is that problem still current? (i.e., are there still cases where msgpack cannot be used?). If that's really addressing a corner-case, perhaps we should not add that option (but open to input).

codecov · 2019-04-16T13:42:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d96f61c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39086   +/-   ##
=========================================
  Coverage          ?      37%           
=========================================
  Files             ?      612           
  Lines             ?    45390           
  Branches          ?        0           
=========================================
  Hits              ?    16795           
  Misses            ?    26308           
  Partials          ?     2287

tagomoris

Basically LGTM. I commented on a point for usability.

tagomoris · 2019-04-17T01:00:41Z

daemon/logger/fluentd/fluentd.go

+	}
+
+	if cfg[asyncKey] != "" && cfg[asyncConnectKey] != "" {
+		return config, errors.Errorf("conflicting options: cannot specify both '%s' and '%s", asyncKey, asyncConnectKey)


This error msg doesn't show the pair of keys now conflicting (just shows values of these keys). It should show conflicting keys and these values for users.

It's best if it can show that the one is now deprecated and the other is recommended.

thaJeztah · 2019-07-14T20:38:39Z

ping @kolyshkin @cpuguy83 PTAL

thaJeztah · 2019-12-31T16:34:51Z

rebased ping @kolyshkin @cpuguy83 @tiborvass PTAL

daemon/logger/fluentd/fluentd.go

thaJeztah · 2020-01-02T11:23:06Z

@cpuguy83 updated; PTAL

cpuguy83

LGTM

This option was deperecated in the upstream fluentd logging driver v1.4.0, and while we documented it as deprecated in the API changelog, there was no mention yet in the deprecated docs. relates to: - fluent/fluent-logger-golang#56 - moby/moby#39086 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This option was deperecated in the upstream fluentd logging driver v1.4.0, and while we documented it as deprecated in the API changelog, there was no mention yet in the deprecated docs. relates to: - fluent/fluent-logger-golang#56 - moby/moby#39086 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 95b0c43) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Docker's fluentd log driver supports passing an "async" option. The "fluentd-async-connect" option was deprecated in docker v20.10 and removed in v28.0. Reference: https://docs.docker.com/engine/deprecated/#fluentd-async-connect-log-opt Functionally, both options are equivalent, see: moby/moby#39086.

Docker's fluentd log driver supports passing an "async" option. The "fluentd-async-connect" option was deprecated in Docker v20.10.0 and completely removed in v28.0.0. Reference: https://docs.docker.com/engine/deprecated/#fluentd-async-connect-log-opt Functionally, both options are equivalent, see: moby/moby#39086. Since Docker's refactor of the fluentd async option was not versioned and affects all API versions, we need to determine the appropriate option using the Docker Server version, in order to be backwards compatible.

thaJeztah added area/logging area/api status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/docs impact/changelog labels Apr 16, 2019

GordonTheTurtle added the status/0-triage label Apr 16, 2019

thaJeztah added impact/deprecation and removed status/0-triage labels Apr 16, 2019

thaJeztah mentioned this pull request Apr 16, 2019

Bump fluent/fluent-logger-golang v1.4.0 #39074

Merged

tagomoris approved these changes Apr 17, 2019

View reviewed changes

GordonTheTurtle assigned duglin May 1, 2019

thaJeztah force-pushed the add_fluentd_options branch from 12d8fca to 29e019a Compare July 16, 2019 14:22

thaJeztah added the rebuild/z label Jul 16, 2019

GordonTheTurtle removed the rebuild/z label Jul 16, 2019

thaJeztah mentioned this pull request Dec 31, 2019

Add RequestAck property to fluentd logger #40332

Closed

thaJeztah force-pushed the add_fluentd_options branch from 29e019a to 1b4631c Compare December 31, 2019 16:34

thaJeztah unassigned duglin Dec 31, 2019

cpuguy83 requested changes Dec 31, 2019

View reviewed changes

daemon/logger/fluentd/fluentd.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the add_fluentd_options branch 2 times, most recently from 3e519d0 to 89b98f7 Compare January 2, 2020 11:22

thaJeztah force-pushed the add_fluentd_options branch from 89b98f7 to 1166e83 Compare January 2, 2020 11:25

cpuguy83 approved these changes Jan 2, 2020

View reviewed changes

thaJeztah added docs/revisit impact/documentation labels Jan 3, 2020

AkihiroSuda merged commit 853e123 into moby:master Feb 11, 2020

thaJeztah deleted the add_fluentd_options branch February 11, 2020 10:50

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

minamijoyo mentioned this pull request Oct 5, 2020

input forward: plugin doesn't close TCP port when it caught SIGTERM fluent/fluent-bit#2610

Closed

albers mentioned this pull request Jan 5, 2021

Update bash completion for fluentd --log-options docker/cli#2913

Merged

thaJeztah mentioned this pull request Nov 12, 2021

Clarification needed in "Fluentd logging driver" documentation docker/docs#13835

Closed

thaJeztah mentioned this pull request Apr 10, 2022

Replace deprecated option --fluent-async-connect with --fluent-async docker/docs#12413

Merged

thaJeztah mentioned this pull request Apr 10, 2022

docs: deprecated: add entry for "fluent-async-connect" log-opt docker/cli#3550

Merged

thaJeztah mentioned this pull request Apr 10, 2022

[20.10 backport] docs: deprecated: add entry for "fluent-async-connect" log-opt docker/cli#3551

Merged

coadler mentioned this pull request Apr 1, 2025

Incompatible with Docker v28 (API 1.48) aws/amazon-ecs-agent#4555

Open

singholt mentioned this pull request Apr 3, 2025

Determine the fluentd async option based on Docker server version aws/amazon-ecs-agent#4558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fluentd: add fluentd-async, fluentd-request-ack, and deprecate fluentd-async-connect #39086

Fluentd: add fluentd-async, fluentd-request-ack, and deprecate fluentd-async-connect #39086

thaJeztah commented Apr 16, 2019 •

edited

Loading

thaJeztah commented Apr 16, 2019

codecov bot commented Apr 16, 2019 •

edited

Loading

tagomoris left a comment

tagomoris Apr 17, 2019

tagomoris Apr 17, 2019

thaJeztah commented Jul 14, 2019

thaJeztah commented Dec 31, 2019

thaJeztah commented Jan 2, 2020

cpuguy83 left a comment

Fluentd: add fluentd-async, fluentd-request-ack, and deprecate fluentd-async-connect #39086

Fluentd: add fluentd-async, fluentd-request-ack, and deprecate fluentd-async-connect #39086

Conversation

thaJeztah commented Apr 16, 2019 • edited Loading

thaJeztah commented Apr 16, 2019

codecov bot commented Apr 16, 2019 • edited Loading

Codecov Report

tagomoris left a comment

Choose a reason for hiding this comment

tagomoris Apr 17, 2019

Choose a reason for hiding this comment

tagomoris Apr 17, 2019

Choose a reason for hiding this comment

thaJeztah commented Jul 14, 2019

thaJeztah commented Dec 31, 2019

thaJeztah commented Jan 2, 2020

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 16, 2019 •

edited

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading