Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/fluent-bit, stable/fluentd] Move source to a new charts repo #21235

Closed
naseemkullah opened this issue Mar 4, 2020 · 23 comments
Closed

Comments

@naseemkullah
Copy link
Collaborator

Hello,

@scottrigby and I are in the process of preparing these charts new home.

Are you all on board and would like to continue maintaining the charts? Please let us know :)

fluent-bit:
@kfox1111
@hectorj2f
@Towmeykaw

fluentd:
@rendhalver
@Miouge1
@hectorj2f

@Miouge1
Copy link
Collaborator

Miouge1 commented Mar 4, 2020

@naseemkullah sounds good.

One of the nice things that the helm/charts repo provides is the CI (helm lint, DCO checker, version bump checker, E2E tests etc...). Do you have plans for some automated CI in the new repo?

@naseemkullah
Copy link
Collaborator Author

@naseemkullah sounds good.

Do you have plans for some automated CI in the new repo?

Indeed we do! We will be using the latest and greatest in github actions for our CI. See https://github.com/helm/charts-repo-actions-demo/ for an example of what we'll be doing.

@Towmeykaw
Copy link
Collaborator

@naseemkullah 👍

@naseemkullah
Copy link
Collaborator Author

@rendhalver @kfox1111 @hectorj2f Please advise as to wether or not you are on board with this! If so you will be retained as maintainers. :) Cheers.

@naseemkullah
Copy link
Collaborator Author

Hey @Miouge1 it seems like the fluentd chart doesnt use an official fluentd image, im not sure we could move it as is, if there were a chart that basically packaged https://github.com/fluent/fluentd-kubernetes-daemonset that could probably work.

@naseemkullah
Copy link
Collaborator Author

Hello @Towmeykaw @kfox1111 @hectorj2f ,

The config values + templating is really unwieldy, if we could achieve something simpler and more flexible (without having to bubble up all possible config options) but in a breaking manner, i think this would be good to do during the moving of the chart. thoughts? Or should we address this like any breaking PR either here or post-move?

@naseemkullah
Copy link
Collaborator Author

naseemkullah commented Mar 20, 2020

I'm thinking something along the lines of:

values file:

config:
  ## Ref: https://docs.fluentbit.io/manual/service
  service:
    Flush: 1
    Daemon: Off
    Log_Level:  info
    Parsers_File: parsers.conf
    Parsers_File: parsers_custom.conf
    HTTP_Server:  On
    HTTP_Listen:  0.0.0.0
    HTTP_Port:    2020

  ## Ref: https://docs.fluentbit.io/manual/input
  inputs:
    - Name: tail
      Path: /var/log/containers/*.log
      Parser: docker
      Tag: kube.*
    - Name: systemd
      Tag: host.*
      Systemd_Filter: _SYSTEMD_UNIT=kubelet.service

  ## Ref: https://docs.fluentbit.io/manual/filter
  filters:
    - Name: kubernetes
      Match: kube.*
      Merge_Log: On
      K8S-Logging.Parser:  On
      K8S-Logging.Exclude: On

  ## Ref: https://docs.fluentbit.io/manual/output
  outputs:
    - Name: es
      Match: "*"
      Host: elasticsearch-master.logging.svc
      Logstash_Format: On

  ## Ref: https://docs.fluentbit.io/manual/parser
  customParsers:
    - Name: docker_no_time
      Format: json
      Time_Keep: Off
      Time_Key: time
      Time_Format: "%Y-%m-%dT%H:%M:%S.%L"

confgmap:

data:
  custom-parsers.conf: |
    {{- range $parser := .Values.config.customParsers }}
    [PARSER]
      {{- range $key, $value := index $parser }}
        {{ $key }} {{ $value }}
      {{- end }}
    {{- end }}

  fluent-bit.conf: |
    [SERVICE]
      {{- range $key, $value := index .Values.config.service }}
        {{ $key }} {{ $value }}
      {{- end }}

    {{- range $input := .Values.config.inputs }}

    [INPUT]
      {{- range $key, $value := index $input }}
        {{ $key }} {{ $value }}
      {{- end }}
    {{- end }}

    {{- range $filter := .Values.config.filters }}

    [FILTER]
      {{- range $key, $value := index $filter }}
        {{ $key }} {{ $value }}
      {{- end }}
    {{- end }}

    {{- range $output := .Values.config.outputs }}

    [OUTPUT]
      {{- range $key, $value := index $output }}
        {{ $key }} {{ $value }}
      {{- end }}
    {{- end }}

It will make the chart way cleaner, mind you if people want to mount secrets and such, they will have to provide that as extraSecrets/extraVolumes and what not. That's to say no more logic based on if backend.foo.bar ....thoughts @kfox1111 @hectorj2f @Towmeykaw ?

@naseemkullah
Copy link
Collaborator Author

I've went ahead and created a fluent-bit chart from scratch https://github.com/fluent/helm-charts/tree/master/charts/fluent-bit based off learnings with the chart in this repo. The config options were getting too unwieldy, the new chart imho has a better approach as per my above comment.

@Towmeykaw @kfox1111 @hectorj2f Please let me know if you would like to help maintain this new one 🙏

@Towmeykaw
Copy link
Collaborator

@naseemkullah looks a lot cleaner, I will have a closer look at it but it looks nice. I would love to help maintaining the chart.

@naseemkullah
Copy link
Collaborator Author

Great to hear @Towmeykaw , please open a PR to add yourself as co-maintainer :)

Also I just merged fluent/helm-charts@65860a0 which allows for maximum flexibility for volumes (as this chart has) and env vars.

That should be all that's needed to accomodate all plugins! Hopefully we can keep the chart nice and clean :D

@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 23, 2020
@stale
Copy link

stale bot commented May 7, 2020

This issue is being automatically closed due to inactivity.

@stale stale bot closed this as completed May 7, 2020
@sannadi99
Copy link

@naseemkullah and @scottrigby is https://fluent.github.io/helm-charts fluentd chart has the capability to use config maps. Right now I don't see a way to send fluent.conf at runtime through config map(values.yaml) like we do it in https://kubernetes-charts.storage.googleapis.com/ fluentd-elasticsearch chart. Are we planning to add that capability soon?

@scottrigby scottrigby removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2020
@scottrigby scottrigby reopened this Sep 1, 2020
@scottrigby scottrigby added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Sep 1, 2020
@scottrigby
Copy link
Member

scottrigby commented Sep 27, 2020

@naseemkullah I think https://github.com/fluent/helm-charts was a good move - thanks for all your wok there! End users who are able to update to a cleaner experience can take advantage of the fluent/fluent-bit chart now (it's already being deployed by users for the past few months). If I understand correctly, the fluent/fluentd chart still needs work. This is a great way for the community to get involved there, for those who are able to contribute 😄 It's in the fluent community's hands now.

For end users who are not yet able to update to the new experience before 13 Nov (when the incubator and stable repos will be archived and their chart release history garbage collected), where should we point users to?

Either way, I think end users will have the best outcome if a conversation/decision/collaboration gets underway sooner than later. What do you all think?

@scottrigby scottrigby changed the title [stable/fluent-bit, stable/fluentd] Move charts to fluent/helm-charts [stable/fluent-bit, stable/fluentd] Move source to a new charts repo Sep 28, 2020
@carrodher
Copy link
Collaborator

From the Bitnami POV, there are only three requirements to add a new chart to the catalog:

As the bitnami/fluentd chart is already present in our catalog, those requirements are already fulfilled and the existing chart is fully integrated into the test & release pipeline. As the stable/fluentd chart needs to be deprecated sooner rather than later if you want to select the bitnami chart as the continuation of the stable one we can work on:

  • Creating a migration path for users upgrading from stable/fluentd to bitnami/fluentd
  • Adding the features present in the stable chart that are not present in the bitnami one

@davidkarlsen
Copy link
Member

davidkarlsen commented Oct 16, 2020

Can we try to conclude on the path forward for fluentd?

I think the bitnami chart for fluentd can do mostly the same as the existing stable one does, but it could maybe allow for a "wildcardish" configuration like here: https://github.com/helm/charts/blob/master/stable/fluentd/templates/configmap.yaml#L11.

We moved from the stable to bitnami one at work, and when I look in the flux repo, we apply the configmap outside of helm, using the option to point to an existing cm:
https://github.com/bitnami/charts/blob/master/bitnami/fluentd/values.yaml#L81

The experience when moving would be on par with such a feature I think. WDYT @carrodher ?

@carrodher
Copy link
Collaborator

Hi @davidkarlsen, we are happy to collaborate in a smooth transition from stable/fluentd to bitnami/fluentd, we'll compare both charts adding any missing feature, paying special attention to the cofigMap configuration.

Do you think there is something else we need to take into account in the bitnami/fluentd chart? Something else we need to do apart from trying to implement any missing feature?

@davidkarlsen
Copy link
Member

I can't see anything else missing than the configurability.

@alemorcuq
Copy link
Contributor

alemorcuq commented Nov 4, 2020

I've been checking both charts and these are the main differences that I could spot:

  • The bitnami/fluentd chart supports a forwarder/aggregator architecture as described here, while the stable/fluentd chart just deploys either a Deployment or a StatefulSet. This simpler architecture can be achieved in bitnami/fluentd by deploying the chart using only the aggregator.

  • The stable/fluentd chart allows the installation of plugins at container initialization. Bitnami doesn't support this and it would be nice to have it supported.

  • The stable/fluentd chart can autoscale itself using a HPA. This could be nice to have in an aggregator-only architecture, but in a forwarder/aggregator architecture it won't work as the forwarders won't be aware of new pods.

  • In the stable/fluentd chart you can configure Fluentd through the values.yaml, while in the Bitnami chart you have to specify an external config map to configure it. Supporting this in Bitnami would be very nice and powerful.

  • stable/fluentd has an Ingress and Bitnami doesn't. This may be a nice feature to have, but I'm not sure if it should be added to the forwarders, the aggregators, or both?

From this list, I'd say the most important items are the "wildcardish" configuration (it will ease the configuration and restart pods on configuration changes as it would be managed by Helm) and the plugin installation at container initialization (although this may require some changes to the container or to the security context of the pods, specially the aggregator pods as they don't run as root by default).


I think that a nice transition between both charts is already possible as shown by @davidkarlsen, but there are a couple of areas (highlighted above) that can be improved. What are your thoughts on this @scottrigby @carrodher @davidkarlsen @Miouge1 ?

@alemorcuq
Copy link
Contributor

I've created a PR to start implementing these missing features. Feel free to add your thoughts there.

@davidkarlsen
Copy link
Member

@alemorcuq IMHO the plugin installation is not so favourable as it is more fragile compared to just building a ready made image containing the plugins required. But of course - it can be implemented and people can just choose not to use it.

+1 to HPA

+1 to the configuration - which is what I requested too.

I don't care too much about ingress - but it can be optional too.

@alemorcuq - awesome - let me know if you need any help.

@carrodher
Copy link
Collaborator

Hi all, I would like to let you know that the above PR (bitnami/charts#4215) has been merged in the bitnami repository, at this moment the above-mentioned features are already implemented in the bitnami/fluentd chart (starting on 3.4.0)

@bridgetkromhout
Copy link
Member

The fluent-related charts in this repo are deprecated and Artifact Hub lists the official fluent repo: https://artifacthub.io/packages/search?repo=fluent&sort=relevance&page=1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants