Hash initial server selection without save last server #18

Merged
merged 1 commit into from Oct 3, 2015

Conversation

Projects
None yet
2 participants
@mvrogowski

We use pen to load balancing RADIUS server that handles standard and PEAP requests.

The standard RADIUS request is a simple transaction that works fine with any load balancing algorithm. But the PEAP transaction is more complex: it needs the TLS tunnel to encrypt the traffic. To perform a PEAP transaction, it need to do more than one request to server. To get success, we need to ensure that all transaction packets will sent to same server. The only way to do it, is using the hash algorithm.

When hash algorithm is enabled, it redirects all request to same server, making one server hammered while others are on idle state.

To fix our problem, I added a new initial server selection algorithm called "Hash no server" that adds the source port to hash algorithm and don't save the last server the client used.

Could you analyze my solution? If it is a good one, I will be thankful if you add it to your repository. :)

I hope I have contributed to your project.

Thank's for your attention!

@UlricE

This comment has been minimized.

Show comment
Hide comment
@UlricE

UlricE Sep 29, 2015

Owner

Will have a look as soon as I get access to a computer.

Owner

UlricE commented Sep 29, 2015

Will have a look as soon as I get access to a computer.

UlricE added a commit that referenced this pull request Oct 3, 2015

Merge pull request #18 from mvrogowski/master
Hash initial server selection without save last server

@UlricE UlricE merged commit 5e33606 into UlricE:master Oct 3, 2015

@mvrogowski

This comment has been minimized.

Show comment
Hide comment
@mvrogowski

mvrogowski Oct 4, 2015

Could you add a new tar.gz release to http://siag.nu/pub/pen/ with this mods, please?

Could you add a new tar.gz release to http://siag.nu/pub/pen/ with this mods, please?

@UlricE

This comment has been minimized.

Show comment
Hide comment
@UlricE

UlricE Oct 6, 2015

Owner

Figuring out how to add this without breaking backward compatibility.

Owner

UlricE commented Oct 6, 2015

Figuring out how to add this without breaking backward compatibility.

@UlricE

This comment has been minimized.

Show comment
Hide comment
@UlricE

UlricE Oct 6, 2015

Owner

About pull request #18:

One of the points of the hash algorithm is that it is deterministic:

  1. In a load-balanced pair of load balancers, the server selection is done the same way regardless of where the client ends up.
  2. A client will always be sent to the same server if it connects from the same IP address.

PR 18 maintains property 1 but not 2, so breaks the old hash algorithm.

The DSR code has its own hash algorithm which includes the port when the -r (roundrobin) option is used. It would be a good idea to use the same semantics for the non-DSR case.

Using -r will forego reusing the old server (see initial_server).

With these changes (see below), -hr will do the same thing as -N while maintaining backwards compatibility.

Have a look and if it makes sense I'll put this in 0.31.0.

diff --git a/client.c b/client.c
index cbdc327..79bd25a 100644
--- a/client.c
+++ b/client.c
@@ -84,10 +84,6 @@ int store_client(struct sockaddr_storage *cli)
        clients[i].addr = *cli;
        clients[i].connects++;

-       /* don't remember server */
-       if(server_alg & ALG_HASH_NO_SERVER) {
-               clients[i].server = NO_SERVER;
-       }

        DEBUG(2, "Client %s has index %d", pen_ntoa(cli), i);

diff --git a/pen.c b/pen.c
index 7f66433..eb346e7 100644
--- a/pen.c
+++ b/pen.c
@@ -2415,9 +2415,6 @@ static int options(int argc, char **argv)
                        }
                        break;
 #endif  /* HAVE_LIBSSL */
-               case 'N':
-                       server_alg |= ALG_HASH_NO_SERVER;
-                       break;
                case '?':
                default:
                        usage();
diff --git a/server.c b/server.c
index b9c016a..0238ef6 100644
--- a/server.c
+++ b/server.c
@@ -51,7 +51,11 @@ static int pen_hash(struct sockaddr_storage *a)
        switch (a->ss_family) {
        case AF_INET:
                si = (struct sockaddr_in *)a;
-               hash = (si->sin_addr.s_addr ^ si->sin_port) % nservers;
+               if (server_alg & ALG_ROUNDROBIN) {
+                       hash = (si->sin_addr.s_addr ^ si->sin_port) % nservers;
+               } else {
+                       hash = si->sin_addr.s_addr % nservers;
+               }

                DEBUG(2, "Hash: %d", hash);

@@ -206,7 +210,7 @@ int initial_server(int conn)
        }
        if (server_alg & ALG_PRIO) return server_by_prio();
        if (server_alg & ALG_WEIGHT) return server_by_weight();
-       if (server_alg & ALG_HASH || server_alg & ALG_HASH_NO_SERVER) return pen_hash(&clients[conns[conn].client].addr);
+       if (server_alg & ALG_HASH) return pen_hash(&clients[conns[conn].client].addr);
        return server_by_roundrobin();
 }

diff --git a/server.h b/server.h
index 53a67d8..b89d0aa 100644
--- a/server.h
+++ b/server.h
@@ -11,7 +11,6 @@
 #define ALG_PRIO 8
 #define ALG_HASH 16
 #define ALG_STUBBORN 32
-#define ALG_HASH_NO_SERVER 64

 #define EMERGENCY_SERVER (-1)
 #define ABUSE_SERVER (-2)

Owner

UlricE commented Oct 6, 2015

About pull request #18:

One of the points of the hash algorithm is that it is deterministic:

  1. In a load-balanced pair of load balancers, the server selection is done the same way regardless of where the client ends up.
  2. A client will always be sent to the same server if it connects from the same IP address.

PR 18 maintains property 1 but not 2, so breaks the old hash algorithm.

The DSR code has its own hash algorithm which includes the port when the -r (roundrobin) option is used. It would be a good idea to use the same semantics for the non-DSR case.

Using -r will forego reusing the old server (see initial_server).

With these changes (see below), -hr will do the same thing as -N while maintaining backwards compatibility.

Have a look and if it makes sense I'll put this in 0.31.0.

diff --git a/client.c b/client.c
index cbdc327..79bd25a 100644
--- a/client.c
+++ b/client.c
@@ -84,10 +84,6 @@ int store_client(struct sockaddr_storage *cli)
        clients[i].addr = *cli;
        clients[i].connects++;

-       /* don't remember server */
-       if(server_alg & ALG_HASH_NO_SERVER) {
-               clients[i].server = NO_SERVER;
-       }

        DEBUG(2, "Client %s has index %d", pen_ntoa(cli), i);

diff --git a/pen.c b/pen.c
index 7f66433..eb346e7 100644
--- a/pen.c
+++ b/pen.c
@@ -2415,9 +2415,6 @@ static int options(int argc, char **argv)
                        }
                        break;
 #endif  /* HAVE_LIBSSL */
-               case 'N':
-                       server_alg |= ALG_HASH_NO_SERVER;
-                       break;
                case '?':
                default:
                        usage();
diff --git a/server.c b/server.c
index b9c016a..0238ef6 100644
--- a/server.c
+++ b/server.c
@@ -51,7 +51,11 @@ static int pen_hash(struct sockaddr_storage *a)
        switch (a->ss_family) {
        case AF_INET:
                si = (struct sockaddr_in *)a;
-               hash = (si->sin_addr.s_addr ^ si->sin_port) % nservers;
+               if (server_alg & ALG_ROUNDROBIN) {
+                       hash = (si->sin_addr.s_addr ^ si->sin_port) % nservers;
+               } else {
+                       hash = si->sin_addr.s_addr % nservers;
+               }

                DEBUG(2, "Hash: %d", hash);

@@ -206,7 +210,7 @@ int initial_server(int conn)
        }
        if (server_alg & ALG_PRIO) return server_by_prio();
        if (server_alg & ALG_WEIGHT) return server_by_weight();
-       if (server_alg & ALG_HASH || server_alg & ALG_HASH_NO_SERVER) return pen_hash(&clients[conns[conn].client].addr);
+       if (server_alg & ALG_HASH) return pen_hash(&clients[conns[conn].client].addr);
        return server_by_roundrobin();
 }

diff --git a/server.h b/server.h
index 53a67d8..b89d0aa 100644
--- a/server.h
+++ b/server.h
@@ -11,7 +11,6 @@
 #define ALG_PRIO 8
 #define ALG_HASH 16
 #define ALG_STUBBORN 32
-#define ALG_HASH_NO_SERVER 64

 #define EMERGENCY_SERVER (-1)
 #define ABUSE_SERVER (-2)

@mvrogowski

This comment has been minimized.

Show comment
Hide comment
@mvrogowski

mvrogowski Oct 7, 2015

I will run some tests and reply as soon it's done.

I will run some tests and reply as soon it's done.

@mvrogowski

This comment has been minimized.

Show comment
Hide comment
@mvrogowski

mvrogowski Oct 9, 2015

I tested your solution and all works very well!

Please, tell me when you add to 0.31 and make it public.

Thank's, Ulric!

I tested your solution and all works very well!

Please, tell me when you add to 0.31 and make it public.

Thank's, Ulric!

@UlricE

This comment has been minimized.

Show comment
Hide comment
@UlricE

UlricE Oct 12, 2015

Owner

Pushed and released. 👍

Owner

UlricE commented Oct 12, 2015

Pushed and released. 👍

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