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

feat: support yaml config file #1208

Merged
merged 16 commits into from
Jun 26, 2024
Merged

feat: support yaml config file #1208

merged 16 commits into from
Jun 26, 2024

Conversation

cw-Guo
Copy link
Collaborator

@cw-Guo cw-Guo commented Jun 14, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1008

Does this PR introduced a user-facing change?

1. fluent-bit supports yaml config mode
2. fluent-bit supports intput processors and output processors

Additional documentation, usage docs, etc.:


@cw-Guo cw-Guo marked this pull request as draft June 14, 2024 05:38
@benjaminhuo
Copy link
Member

@cw-Guo Thanks, that's great to have yaml config files

cc @wanjunlei @wenchajun @Gentleelephant @adiforluls

@cw-Guo cw-Guo marked this pull request as ready for review June 14, 2024 05:53
@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 14, 2024

@benjaminhuo This is ready for review. But the test cases are limited.

The idea behind this implementation can be found in the origin issue# 1008

To support processor, a map[string]interface{} is used to avoid defining the structure for the processors. Because the available processors in the official documents are very limited and are subjected to change.

We might add a new CRD for processors in the furture. (In practice, any filter can be a processor, without errors but this usage is not documented in the offical documents.)

A few LoadAsYaml functions haven't been implemented, I will try to implement them in the following few days.

@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 14, 2024

To run it, a new image for fluent-bit need to be built.

@cw-Guo cw-Guo marked this pull request as draft June 14, 2024 06:18
// ConfigFileFormat defines the format of the config file, default is "classic",
// available options are "classic" and "yaml"
// +kubebuilder:validation:Enum:=classic;yaml
ConfigFileFormat *string `json:"configFileFormat,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

What would be the default configuration format?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the default will be the classic mode.

@adiforluls
Copy link
Member

I left a comment in the proposal about an edge case. And would like to see some test cases here if possible (unit tests at-least).

@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 17, 2024

I left a comment in the proposal about an edge case. And would like to see some test cases here if possible (unit tests at-least).

i will add more tests. For custom plugin, i will also try to incooperate it but i am afraid it will be hard.

@adiforluls
Copy link
Member

I left a comment in the proposal about an edge case. And would like to see some test cases here if possible (unit tests at-least).

i will add more tests. For custom plugin, i will also try to incooperate it but i am afraid it will be hard.

Thoughts:

  • Old users who have custom plugin config will be on classic style by default, so no confusion here
  • A new user comes along, adds a custom plugin config in classic style and selects yaml style in the fbc CR. IMO this should result in operator throwing an error, if the user selects yaml style config, then they should provide yaml style configs in custom plugin and vice versa.

This is just my opinion, I'd let other maintainers pitch in as well on the PR.

@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 17, 2024

@adiforluls I can probably add one more field in the custom plugin to take in the unsupported plugins, just like the current processor implementation.
https://github.com/fluent/fluent-operator/pull/1208/files#diff-487bd681e6fe01d15ade2e5a8650963714441e9282d9d6898c8c3ce1ab09aa43R76

@cw-Guo cw-Guo marked this pull request as ready for review June 18, 2024 05:13
@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 18, 2024

@benjaminhuo @adiforluls @wanjunlei @wenchajun @Gentleelephant
May I have your reviews?

@benjaminhuo
Copy link
Member

@benjaminhuo @adiforluls @wanjunlei @wenchajun @Gentleelephant May I have your reviews?

Sure, we'll review this. Thanks for the contribution.
cc @wanjunlei @wenchajun @Gentleelephant

adiforluls
adiforluls previously approved these changes Jun 19, 2024
Copy link
Member

@adiforluls adiforluls left a comment

Choose a reason for hiding this comment

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

Given it touches a lot of files like fluent-watcher binaries, I'd definitely want other reviewers to go through the change as well, but in terms of overall configuration and cases LGTM.

chengweiguo and others added 13 commits June 20, 2024 21:21
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
Signed-off-by: juicer <cwguoz@gmail.com>
apis/fluentbit/v1alpha2/clusterfilter_types.go Outdated Show resolved Hide resolved
apis/fluentbit/v1alpha2/plugins/custom/custom_types.go Outdated Show resolved Hide resolved
apis/fluentbit/v1alpha2/clusterfluentbitconfig_types.go Outdated Show resolved Hide resolved
apis/fluentbit/v1alpha2/clusteroutput_types.go Outdated Show resolved Hide resolved
apis/fluentbit/v1alpha2/plugins/params/kvs.go Outdated Show resolved Hide resolved
cmd/fluent-watcher/fluentbit/main.go Outdated Show resolved Hide resolved
@wanjunlei
Copy link
Collaborator

I have some questions and suggestions.

  • This PR does not involve namespace CRD. Will this be implemented in the future?
  • In the long run, we don't need to maintain two sets of configuration files. We will remove the support for classic configuration files in future versions. So when the fluentbit version is greater than 2.0.0, can we consider using the YAML configuration file as the default configuration file?
  • It is not a good idea to splice yaml files. I suggest defining a structure to generate yaml config file.

Just some suggestions, not blocking this pr.

Signed-off-by: juicer <cwguoz@gmail.com>
@cw-Guo
Copy link
Collaborator Author

cw-Guo commented Jun 25, 2024

@wanjunlei ❤️ Thanks for the review and suggestions. I have fixed the import orders.

This PR does not involve namespace CRD. Will this be implemented in the future?

I am not too sure about how the namespace CRDs work. I didn't use it in my previous experience. But I did add support for:

  1. Filter,
  2. Output

They are passed in to fluentbitconfig_controller.go. From my understanding, this will be enough but please correct me if i understand it wronly.

In the long run, we don't need to maintain two sets of configuration files... So when the fluentbit version is greater than 2.0.0, can we consider using the YAML configuration file as the default configuration file?

It is not a good idea to splice yaml files. I suggest defining a structure to generate yaml config file.

I agree that we don't want to maintain two sets of configuration files and I am also against splicing yaml files. But I also hasitate to make the (hard) decision for the users. Avoiding breaking changes is the start point of my design. I am afraid only supporting yaml config file format would possibly make some users abandon the project.

If the community has decided to only support the yaml config file format, I am happy to see that a refractor for fluent-operator or a newer api version.

@wanjunlei
Copy link
Collaborator

@wanjunlei ❤️ Thanks for the review and suggestions. I have fixed the import orders.

This PR does not involve namespace CRD. Will this be implemented in the future?

I am not too sure about how the namespace CRDs work. I didn't use it in my previous experience. But I did add support for:

  1. Filter,
  2. Output

They are passed in to fluentbitconfig_controller.go. From my understanding, this will be enough but please correct me if i understand it wronly.

In the long run, we don't need to maintain two sets of configuration files... So when the fluentbit version is greater than 2.0.0, can we consider using the YAML configuration file as the default configuration file?

It is not a good idea to splice yaml files. I suggest defining a structure to generate yaml config file.

I agree that we don't want to maintain two sets of configuration files and I am also against splicing yaml files. But I also hasitate to make the (hard) decision for the users. Avoiding breaking changes is the start point of my design. I am afraid only supporting yaml config file format would possibly make some users abandon the project.

If the community has decided to only support the yaml config file format, I am happy to see that a refractor for fluent-operator or a newer api version.

OK, got it. The namespace CRD just needs to change the FIlter and Output.

@benjaminhuo
Copy link
Member

I agree that we don't want to maintain two sets of configuration files and I am also against splicing yaml files. But I also hasitate to make the (hard) decision for the users. Avoiding breaking changes is the start point of my design. I am afraid only supporting yaml config file format would possibly make some users abandon the project.

If the community has decided to only support the yaml config file format

The old config file format might exist for some time for the yaml format to mature and catch up.
There are quite a lot of things to catch up.

@benjaminhuo benjaminhuo merged commit d8e2060 into fluent:master Jun 26, 2024
9 of 10 checks passed
@benjaminhuo
Copy link
Member

benjaminhuo commented Jun 26, 2024

Thanks @cw-Guo very much for such a big enhancement! I've invited you as fluent-operator maintainer.
Looking forward to more contributions for the yaml format support!
@patrick-stephens @agup006 would you help to invite @cw-Guo to the fluent org?

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.

feature request: Add support for fluent-bit yaml configuration file mode
4 participants