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

[1.0.0] expose override raw conf for container log pipeline #556

Merged
merged 2 commits into from Apr 3, 2020

Conversation

samjsong
Copy link
Contributor

@samjsong samjsong commented Apr 3, 2020

Description

This is similar to the way that Fluent-bit exposes their config. They have the option to set configs, but also to override entire sections if needed.

Here are two of the use-cases we want to solve with this new fluentd.logs.containers.overrideRawConfig:

  1. If the user wants to change config that requires some form of indentation. examples include adding a new label to split the pipeline based on metadata (metadata based ingest budgets?), or forking to S3 (need to use the copy plugin in the output)
  2. If the user wants to add a new tag to enrich custom logs with metadata. In that case, they could update the tags in this pipeline from <filter containers.**> to <filter containers.** custom.**>, which will allow for the custom logs with the custom tag to also go through this pipeline. In the case that their log files are named in a different format than the default docker logs files, they can also update the relevant regexes to accommodate that.

The idea here is to enable users to customize the part of the fluentd pipeline that is most conducive to customizations, with the clear indication that by doing so, they are overriding the default pipeline and that therefore not every custom change can be tested and supported. We'll document examples on how to achieve certain behavior (like the above), but anything else will be at the user's discretion.

(also I added the enabled flag for systemd and kubelet logs as a feature request from a previous github issue)

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

Copy link
Contributor

@vsinghal13 vsinghal13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sumo-drosiek
Copy link
Contributor

Looks ok, but we need to document this properly. I see some threats:

  • helm templates won't be working for overrided configurations
  • customer have to set buffers and so on manually
  • customer can accidentally match/filter records dedicated to other part of configuration. eg **
  • fluentd.logs.containers.extraFilterPluginConf won't be working

@@ -1,3 +1,4 @@
{{ if .Values.fluentd.logs.kubelet.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -29,6 +30,8 @@
</buffer>
</match>
</label>
{{- end}}
{{ if .Values.fluentd.logs.systemd.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add it to 1.0.0 :)

@perk-sumo perk-sumo added this to the v1.0 milestone Apr 3, 2020
@sumo-drosiek sumo-drosiek self-requested a review April 3, 2020 07:51
@samjsong samjsong merged commit bcc6115 into master Apr 3, 2020
@samjsong samjsong deleted the ssong-expose-container-log-conf branch April 3, 2020 16:01
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.

None yet

4 participants