-
Notifications
You must be signed in to change notification settings - Fork 229
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
Remove Duplicate Cluster parsers in Fluent-bit config. #853
Remove Duplicate Cluster parsers in Fluent-bit config. #853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change removes the functionality of being able to use a clusterParser via a FluentBitConfig CR.
I think somehow the clusterParsers were being wrongfully selected, only those ClusterParsers are to be selected that have a labelSelector defined in FluentBitConfig CR. Somehow when no labelSelector is defined all the ClusterParsers are being selected, which shouldn't happen. That needs to be fixed.
Just so that nothing gets lost in translation, the intended behaviour was to create a copy of a clusterParser in We'd want to keep the functionality of being able to consume clusterParsers via namespaced Filters, a better logic than forming dups can also be proposed. |
With this change Also, ClusterParser can be used via a FluentBitConfig CR. When we are processing namespaces FB Configmap, we are just not processing the parsers now. as we have are doing for filters. May there is some specific use-case you are suggesting that could fail with this change. let me know, I can test that. |
Create a ClusterParser, now try to use this parser using a namespaced filter. If this works then everything is fine, otherwise this is broken. It's been a while since I wrote this feature so we can think of a better approach to maintain the functionality if this doesn't work with your change. |
Just thinking, does namespaced parser have any significance? can't we have just parsers(no namespace level parsers). parsers alone can't be used for log processing. It would be always part of a filter & Filter can be namespace level and cluster level. Thoughts? |
Signed-off-by: karan k <karan.k@oracle.com>
343e50c
to
0e867b1
Compare
@adiforluls As of now, I have just added simple fix to remove the duplicate cluster parsers. It will help to get rid of parser not registered error. But we should re-think about this parser namespace architecture. Honestly, It's not making sense to me to introduce namespace parsers in Fluent-operator. Although Namespace filters are making sense as we are introducing the namespace logging, But namespace parsers are something, we don't need to categorised at cluster and namespace level. |
@karan56625 The change looks okay but I'll review more carefully tomorrow hopefully and approve then. For your other questions, I'll look to answer them tomorrow but I think there's some context missing, for that I suggest you to go through the original problem statement. #580 Overall, the gist is to let namespaced users create k8s resources that wraps FluentBit configs, previously only a ClusterAdmin would be able to create cluster level Filter/Parser/Output. Now imagine that I chose to exclude namespaced parser from the design, as a namespaced user/tenant/admin, for your namespaced Filter, you'd then need a ClusterAdmin to create ClusterParser resource for you for your filter to work. For just one namespace this doesn't look like a problem, but imagine this scenario for 100s of namespaces (namespace as a service). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes duplicate parser warnings so it's a fix. There's still a low prio bug where ClusterParsers are being selected inspite of not having a labelSelector in FluentbitConfig CR right? Could you open a backlog issue for that, I'd like to get to it when I have time.
Also cc @benjaminhuo this can be merged unless you have some changes you'd like to request |
yeah, If we create one clusterParser, it is getting replicate for each
Filed issue to track that #857. |
To cover that use-case, we are creating duplicate parser which are doing the same job. Suppose if we have 100 namespace config and one cluster parser. So there will be 101 entry for the same cluster in parsers.conf that seems redundant. Even if we fix it, and 94 namespace configMap are referring to one cluster parser, there will be 95 entries in parsers.conf. This is for just one cluster parser. suppose if namespaced config is referring to many cluster parsers. |
@benjaminhuo if you are fine with this simple fix to get rid of error message in fluentbit logs, can I seek your approval and request you to merge this if there is no issue. |
Okay I'll answer questions one by one.
|
I'll take a look at this PR. |
What this PR does / why we need it:
If we have multiple namespace FB config, don't Create cluster parser for each namespace config. There should be one single cluster parser for all namespace.
Which issue(s) this PR fixes:
Fixes #852
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: