Simple rr-network load balancer #1706

illesm · 2017-10-04T18:32:42Z

It implements a simple round-robin load balancing between multiple destination.
Balancing is based on MSEC hashing where destinations share the same configuration
except the destination address.
The main purpose to make load balance configuration simple.

It can be used as a simple network destination and all network parameters are
recognized, the destination servers must be configured as a space separated list
of host using the "targets" option:

destination mydest {
    load-balancer(
            targets(server1 server2 server2) # mandatory option

            # additional network options can be set as well:
            transport(udp)
            port(3456)
    )
};

NOTE: as failover-server is not supported in this version it is left out. Otherwise
nodes are used for fail-over servers to maintain availability.

The idea and original configuration comes from Ferenc Sipos.
Big kudos for the inspiration and for his persistence on pushing me to add
this function on our flight from New York to Memphis, TN! Thx!

Also extended confgen module:
confgen: enable passing arguments to confgen external program as env variables

Options can be set when invoking confgen external program. These options are
passed as environment variables. Note that env variables are overwritten and
cleared after the external execution.

This comes handy as this way configuration can be generated not only dynamically,
but also driven from the configuration.

@module confgen context(destination) name(mygen) exec("gen.sh")

And later that can be used as:
mygen(
    option1(param1)
    option2("my other option")
)

This functionality probably addresses #1210 mostly.

kira-syslogng · 2017-10-04T18:33:02Z

This user does not have permission to start the build. Can one of the admins verify this patch and start the build? (admin please type: ok to test)

presidento · 2017-10-04T18:54:26Z

@kira-syslogng ok to test.

presidento · 2017-10-04T18:59:00Z

The idea is clever.
It is not a problem that we can loss the order of the incoming logs? As I remember, a previous idea was to do the load balancing based on the source host to keep the sequence of logs coming from a device. But I am not sure.

kira-syslogng · 2017-10-04T19:07:16Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

illesm · 2017-10-04T19:13:06Z

In many LB scenarios you will end up loosing sequence, but of course you use some slng assigned sequence number to reproduce it. Anyhow this is a simple solution that is useful for scenarios where strict sequencing is not important, but you have lot of logs and need to just distribute it.

You can of course use "sticky host" load-balancing, but that might be a limitation where one host produces too many logs. Also in case of larger deployments with multiple nodes producing logs the strict sequence is probably not that important.

kira-syslogng · 2017-10-04T19:35:22Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

bazsi · 2017-10-05T07:02:07Z

Random set of thoughts:

I like confgen passing the environment variables to the process, however setenv/unsetenv is not very nice, but probably not very problematic either. The usual way is to create a custom environ aray, and use that in execve(), however there's no "e" variant from popen.
The persist-id should probably be set for each network() destination to be a unique value, as otherwise the queues may not be separated.
the ordering issue can be fixed by using $HOST instead of MSEC as the routing key. It may not be that balanced as MSEC, but then it is a tradeoff by the customer. What if the template to produce the routing key can be specified? Then one could use $(sha1 $HOST) and route based on the first character.
with the latest register-generators-as-plugins refactor this could become a C implementation, thus no need for an external program which causes headaches with SELinux.

Kokan · 2017-10-05T04:46:36Z

scl/loadbalancer/gen-loadbalancer.sh

@@ -1,4 +1,25 @@
 #!/bin/sh
+#############################################################################
+# Copyright (c) 2017 Balabit


Could you please squash the style check and copyright commits ?

I can, but why?
it does two different things on two different modules.

Sorry I meant not those two commits into one :)

You have currently 4 commits, from which 2 of them just for style check and copyright.
It would be nice to squash this modification into the original two commits (those are well separated).

The confgen: fixed coding style error and confgen: enable passing arguments to confgen external program as env into one, also the loadbalancer: added missing copyright headers and loadbalancer: added network load balancer destination into one.

I hope it make more sense like this :)

fair, i will do it :)

Kokan · 2017-10-05T05:41:49Z

modules/confgen/confgen-plugin.c

+static void
+confgen_set_args_as_env(gpointer k, gpointer v, gpointer user_data)
+{
+  gchar buf[1024];


What happens if the 1024 is not enough ?

we truncate the variable name, but it is fairly unlikely to have such a long option name

It is fair enough.

Kokan · 2017-10-05T05:44:18Z

modules/confgen/confgen-plugin.c

+{
+  gchar buf[1024];
+  g_snprintf(buf, sizeof(buf), "confgen_%s", (gchar *)k);
+  setenv(buf, (gchar *)v, 1);


What would happen if setenv is not successful ?
If the environment variable exists, do we really want to override it ?

yes we want to overwrite it

we can handle errors, but it is basically an out of memory case only, which is bad anyhow

EINVAL name is NULL, points to a string of length 0, or contains an '=' character. ENOMEM Insufficient memory to add a new variable to the environment.

Kokan · 2017-10-05T05:47:16Z

modules/confgen/confgen-plugin.c

+static void
+confgen_unset_args_from_env(gpointer k, gpointer v, gpointer user_data)
+{
+  gchar buf[1024];


What happens if the 1024 is not enough ?

we truncate the variable name, but it is fairly unlikely to have such a long option name

It is fair enough.

Kokan · 2017-10-05T05:58:25Z

modules/confgen/confgen-plugin.c


+  cfg_args_foreach(args, confgen_set_args_as_env, NULL);


Note: At least on documentation level it should be considered that in the name of the args the -s are replaced with _.

Kokan · 2017-10-05T06:11:22Z

scl/loadbalancer/gen-loadbalancer.sh

+    echo '    filter {'
+    echo -n '    "'
+    echo -n $target_id
+    echo -n '" == "$(% ${R_MSEC} '


Could we benefit from allowing other number source ? For example $SEQNUM could also work.

yes, it could work.
@bazsi also suggested using HOST, but that is bit tricky to create a filter as there is not module for string, of course we can use comparison always. Though it would not balance well with many logs coming from one host. Probably that could be made configurable.

Either MSEC or SEQNUM could work, however we wanted something simple.

I would leave it as it is now, and if someone wants to enhance it they can send PR.

Kokan · 2017-10-05T07:07:26Z

scl/loadbalancer/gen-loadbalancer.sh

+    echo -n $target
+    echo -n '" '
+    echo -n $TARGET_PARAMS
+#echo -n 'failover-servers("'


Could you remove this commented part of the script ?

i intentionaly left it here as there is failover support in PE6, but not here, so anyone wanting to port it would find it useful.
Of couse it can be removed.

I would rather not include it here.

Kokan · 2017-10-05T07:11:30Z

scl/loadbalancer/gen-loadbalancer.sh

+target_cnt=$(echo $TARGETS | wc -w)
+target_id=0
+echo 'channel {'
+for target in $TARGETS


The MSEC max value is a limit of the number of the targets. In this case the script do not really need to create the extra channels.

I do not understand this limit, but if you ommit the channel definition the final flag would stop the msg processing not just for this but for other destination.
what do i miss?

If there are MSEC + n target, those n targets won't be chosen. Of course it won't cause any issue, but it enlarges the configuration unnecessarily.
It would be a minor tweak not even creating them.

check the code, it wont happen, unless they configure more then 2^128 servers destination :)
we do the modulo of the # of targets on MSEC and MSEC is a large number, even in 1 sec there are 1000 different value, so it should be ok up to a dozen target for sure.

I think @Kokan just wanted to clarify that MSEC is not a large integer, it's a number between 0 and 999.
That is the reason why you may not want to generate more than 1000 channels/destinations.

I don't have any problems with the current implementation, we just wanted to let you know about this. :)

I do not think that this would actually scale to more then a dozen (and also does not seem to be realistic) so we should be fine wit this :)

Have you tried this out in real life? Or any kind of emulation with high traffic ? It would be nice to see how balanced it gets.

Feri used a less efficient version (using regexp) in production, ask him for details.

Kokan · 2017-10-05T07:14:18Z

scl/loadbalancer/plugin.conf

+@module confgen context(destination) name(genlb) exec("`scl-root`/loadbalancer/gen-loadbalancer.sh")
+
+
+block destination load-balancer(targets()) {


I would include somehow in the name, that it is currently for network destination only, and not a full-fledged general load balancer.

network-load-balancer is ok or maybe network-rr?

I would prefer network-load-balancer.

@module

…variables Options can be set when invoking confgen external program. These options are passed as environment variables. Note that env variables are overwritten and cleared after the external execution. Also note that in option names dash(-) is converted to underscore (_). This comes handy as this way configuration can be generated not only dynamically, but also driven from the configuration. @module confgen context(destination) name(mygen) exec("gen.sh") And later that can be used as: mygen( option1(param1) option2("my other option") )

It implements a simple round-robin load balancing between multiple destination. Balancing is based on MSEC hashing where destinations share the same configuration except the destination address. The main purpose to make load balance configuration simple. It can be used as a simple network destination and all network parameters are recognized, the destination servers must be configured as a space separated list of host using the "targets" option: destination mydest { network-load-balancer( targets(server1 server2 server2) # mandatory option # additional network options can be set as well: transport(udp) port(3456) ) }; NOTE: as failover-server is not supported in this version it is left out. Otherwise nodes are used for fail-over servers to maintain availability. The idea and original configuration comes from Ferenc Sipos. Big kudos for the inspiration and for his persistence on pushing me to add this function on our flight from New York to Memphis, TN! Thx!

kira-syslogng · 2017-10-06T01:02:13Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

illesm · 2017-10-06T13:29:39Z

I fixed what I considered important, so feel free to merge it or enhance it further if you want

MrAnno added the needs-review label Oct 4, 2017

Kokan self-requested a review October 5, 2017 04:39

Kokan added the hacktoberfest label Oct 5, 2017

Kokan suggested changes Oct 5, 2017

View reviewed changes

presidento added the enhancement label Oct 5, 2017

illesm added 2 commits October 6, 2017 02:49

illesm force-pushed the load-balancer branch from 8a20d15 to 193e862 Compare October 6, 2017 00:49

Kokan approved these changes Oct 10, 2017

View reviewed changes

Kokan merged commit 980b3be into syslog-ng:master Oct 10, 2017

Kokan removed the needs-review label Oct 10, 2017

presidento mentioned this pull request Oct 10, 2017

Feature: native network load balancing #1210

Open

		@module confgen context(destination) name(genlb) exec("`scl-root`/loadbalancer/gen-loadbalancer.sh")


		block destination load-balancer(targets()) {

Simple rr-network load balancer #1706

Simple rr-network load balancer #1706

Conversation

illesm commented Oct 4, 2017

kira-syslogng commented Oct 4, 2017

presidento commented Oct 4, 2017

presidento commented Oct 4, 2017

kira-syslogng commented Oct 4, 2017

illesm commented Oct 4, 2017

kira-syslogng commented Oct 4, 2017

bazsi commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAnno Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kira-syslogng commented Oct 6, 2017

illesm commented Oct 6, 2017

MrAnno Oct 6, 2017 •

edited