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

es_out: Support Upstream Servers configuration #2653

Closed
wants to merge 0 commits into from

Conversation

sirwio
Copy link

@sirwio sirwio commented Oct 7, 2020

Supporting Upstream Servers configuration for the Elasticsearch output plugin
Adding support for Upstream Servers for the Elasticsearch ouput plugin.
Implmentation mimics the support for Upstream Servers already present
in the Forward output plugin.

There is no additional node configuration keys required by the plugin
for the Upstream Servers configuration.

The change will require the documentation to be updated e.g. to mention that the out_es
plugin does have an additional key Upstream that shall provide the absolute path for the
Upstream configuration file. Documentation need also be added to the Upstream Servers
documentation to mention that the Elasticsearch output plugin is now supported.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

Documentation

  • Documentation required for this feature
    Update Elasticsearch output plugin documenation with Upstreams key information
    Update Upstreams Servers documentation to mention support for Elasticsearch is available.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@sirwio
Copy link
Author

sirwio commented Oct 7, 2020

Sample fluent bit configuration file:

fluent-bit.conf
-------------------
SERVICE]
  Log_Level debug 

[INPUT]
  Name tcp
  
[OUTPUT]
  Name es 
  Match * 
  Host localhost 
  Port 9200
  tls on 
  tls.verify off 
  HTTP_User admin 
  HTTP_Passwd admin
  Index oehs
  Upstream /fbit/upstream_es.conf

The new configuration required is the key Upstream.

  Upstream /fbit/upstream_es.conf

@sirwio
Copy link
Author

sirwio commented Oct 7, 2020

Valgrind debug output without Upstream servers configured:
debug-log-without-upstream.txt

@sirwio
Copy link
Author

sirwio commented Oct 7, 2020

Upstream Servers configuration file:

upstream.conf
-----------------
[UPSTREAM]
  name es-round-robbin 

[NODE]
  name       es1 
  host       localhost 
  port       9200 
  tls on
  tls.verify off
  tls.debug 1

[NODE]
  name       es2 
  host       localhost 
  port       9201 

@sirwio
Copy link
Author

sirwio commented Oct 7, 2020

Valgrind debug output with Upstream servers configured:
debug-log-with-upstream.txt

@sirwio
Copy link
Author

sirwio commented Oct 7, 2020

This MR is based upon the work made by claralui in MR #1560

@sirwio
Copy link
Author

sirwio commented Oct 12, 2020

@edsiper In review comment on claralui's MR #1560 you made the review below:

f this represents a target elasticsearch node I will propose struct flb_elasticsearch_node

Link to comment on MR #1560
The configuration is this case not for the node specifically but general for the plugin itself. It might be possible to override it for each node but in present implementation that is not done.
Also if the name would be change for this plugin then the same name change should perhaps be done for the forward plugin as well and done in a different MR addressing both plugins.

@sirwio sirwio force-pushed the master branch 3 times, most recently from d41329c to 9062cea Compare November 18, 2020 12:58
@edsiper
Copy link
Member

edsiper commented Nov 19, 2020

assigning to @fujimotos for review

plugins/out_es/es.c Outdated Show resolved Hide resolved
flb_output_set_context(ins, ctx);

Copy link
Member

Choose a reason for hiding this comment

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

L523 has a trailing whitespace. Remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by removing trailing spaces.

@fujimotos
Copy link
Member

@edsiper @sirwio I tested this patch and the basic functionality was working.

I left a few inline review comments to the diff. I'm okay with merging this patch
once these things are resolved.

The configuration is this case not for the node specifically but general for the plugin itself. It might be possible to override it for each node but in present implementation that is not done.

This sirwio's comment essentially means that Buffer_Size in the following example
is ignored as of the current patch set (because that part is left unimplemented).

[UPSTREAM]
    name       forward-balancing

[NODE]
    name       node-1
    host       127.0.0.1
    port       9200
    buffer_size -1

So this patch has a missing piece/caveat. That being said, I think it is still reasonable
to merge this patch since it provides a useful (albeit incomplete) functionality to users.

@sirwio
Copy link
Author

sirwio commented Nov 19, 2020

@edsiper @sirwio I tested this patch and the basic functionality was working.

I left a few inline review comments to the diff. I'm okay with merging this patch
once these things are resolved.

The configuration is this case not for the node specifically but general for the plugin itself. It might be possible to override it for each node but in present implementation that is not done.

This sirwio's comment essentially means that Buffer_Size in the following example
is ignored as of the current patch set (because that part is left unimplemented).

[UPSTREAM]
    name       forward-balancing

[NODE]
    name       node-1
    host       127.0.0.1
    port       9200
    buffer_size -1

So this patch has a missing piece/caveat. That being said, I think it is still reasonable
to merge this patch since it provides a useful (albeit incomplete) functionality to users.

Yes the idea was to get the patch approved with this incomplete functionality. In my current use cases I have no need to have configurations that differ between the elastic search nodes. I can however imagine that such could be required for some users.

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

Successfully merging this pull request may close these issues.

3 participants