Exponentially backoff on repeated connect retries

- When the number of retries per remote exceeds a limit (hard coded to 5), double the restart pause interval for each additional retry per remote.
- Trigger a SIGHUP to reset the retry count when the pause interval exceeds 1024 times the base value of restart pause.

The base value of restart pause is set using --connect-retry (5 seconds by default). If it is set to zero no slow-down will happen.

Signed-off-by Selva Nair <[email protected]...>

---
 src/openvpn/init.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

src/openvpn/init.c +1 -0

index 498d36f..45e8878 100644
@@ -1955,6 +1955,7 @@ static void
 socket_restart_pause (struct context *c)
 {
   int sec = 2;
+  int backoff = 0;
 
   switch (c->options.ce.proto)
     {

@@ -1977,6 +1978,17 @@ socket_restart_pause (struct context *c)
     sec = 10;
 #endif
 

And why sighup? Shouldn't just limiting sec be enought. SIGHUP has many side effects. I don't think we should trigger these for connection time.

With default setting this results in a SIGHUP after some hours and also the last intervals with 5120s are too big for default settings. I would aim for something lower like five or ten minutes.

Quoted 7 lines

And why sighup? Shouldn't just limiting sec be enought. SIGHUP has many side effects. I don't think we should trigger these for connection time.

With default setting this results in a SIGHUP after some hours and also the last intervals with 5120s are too big for default settings. I would aim for something lower like five or ten minutes.

I think a bit about that and I think a good way to specify exponential back is something like

connect-retry 5 1800, which would mean "scale back up to 1800s" and maybe default that option to connect-retry 5 300. That also allows specifying larger connect-retry values without backoff. E.g. connect-retry 60 60

Quoted 7 lines · On Sat, Jul 2, 2016 at 11:32 AM, Arne Schwabe <[email protected]...…

And why sighup? Shouldn't just limiting sec be enought. SIGHUP has many side effects. I don't think we should trigger these for connection time.

With default setting this results in a SIGHUP after some hours and also the last intervals with 5120s are too big for default settings. I would aim for something lower like five or ten minutes.

I think a bit about that and I think a good way to specify exponential back is something like

connect-retry 5 1800, which would mean "scale back up to 1800s" and maybe default that option to connect-retry 5 300. That also allows specifying larger connect-retry values without backoff. E.g. connect-retry 60 60

Good idea.

Agreed, triggering SIGHUP is not ideal. The only reason for it was to reset the counters so that the backoff time doesn't get stuck at the highest value for ever. If we reduce the max backoff interval that's less of a concern.

One motivation for this patch is to avoid logs filling up. For a client stuck in a restart loop with verb=3 and 5 minutes max pause, the logs grow by about 1 M per day -- may be not too bad.

Selva

Quoted 13 lines

connect-retry 5 1800, which would mean "scale back up to 1800s" and maybe default that option to connect-retry 5 300. That also allows specifying larger connect-retry values without backoff. E.g. connect-retry 60 60

Good idea.

Agreed, triggering SIGHUP is not ideal. The only reason for it was to reset the counters so that the backoff time doesn't get stuck at the highest value for ever. If we reduce the max backoff interval that's less of a concern.

Quoted 5 lines

One motivation for this patch is to avoid logs filling up. For a client stuck in a restart loop with verb=3 and 5 minutes max pause, the logs grow by about 1 M per day -- may be not too bad.

Selva

Do you want to propose a new patch or should I do it. I would propose still a exponential backoff but starting a 2s. so we get 2s 4s 8s ... or as default conect-retry 2 256. It would then just stay at the 256s interval until a sucessful connection is established. That simplifies the logic and gives predictable (from a user perspective) results.

Iirc the logic of connect-retry was changed by me in master so changing the logic again should not be a problem

- When the number of retries per remote exceeds a limit (hard coded to 5), double the restart pause interval for each additional retry per remote.
- Trigger a SIGHUP to reset the retry count when the pause interval exceeds 1024 times the base value of restart pause. (removed in v2 of the patch)

The base value of restart pause is set using --connect-retry (5 seconds by default).

v2 changes (based on suggestions from Arne Schwabe <[email protected]...>)

- Do not throw SIGHUP.
- Add an optional argument to "--connect-retry n [m]" where 'm' specifies the max value of restart pause interval (default 300 sec).
E.g., "--connect-retry 5 1800" will cause the restart pause to scale up starting at 5 until it exceeds 1800 seconds at which point it gets capped at 1800.
- If n == m no slow down will occur.
- While at it, fix typos and clarify the description of connect-retry-max in the man page and Changes.rst

Signed-off-by Selva Nair <[email protected]...>

---
 Changes.rst           |    8 ++++++--
 doc/openvpn.8         |   19 ++++++++++++-------
 src/openvpn/init.c    |    8 ++++++++
 src/openvpn/options.c |   13 ++++++++++---
 src/openvpn/options.h |    1 +
 5 files changed, 37 insertions(+), 12 deletions(-)

Changes.rst +0 -0

index d12cdad..59722e2 100644

Changes.rst +1 -0

+
 Maintainer-visible changes
 --------------------------
 - OpenVPN no longer supports building with crypto support, but without TLS

doc/openvpn.8 +12 -7

index 64cc934..3ca6f50 100644
@@ -462,22 +462,27 @@ application-level UDP protocols, or tunneling protocols which don't
 possess a built-in reliability layer.
 .\"*********************************************************
 .TP
-.B \-\-connect\-retry n
+.B \-\-connect\-retry n [max]
 Wait
 .B n
-seconds  between connection attempts (default=5).
+seconds  between connection attempts (default=5). Repeated reconnection
+attempts are slowed down after 5 retries per remote by doubling the wait
+time after each unsuccessful attempt. The optional argument
+.B max
+specifies the maximum value of wait time in seconds at which it gets
+capped (default=300).
 .\"*********************************************************
 .TP
 .B \-\-connect\-retry\-max n
 .B n
-specifies the number of times all
+specifies the number of times each
 .B \-\-remote
-respectively
+or
 .B <connection>
-statements are tried. Specifiying
+entry is tried. Specifying
 .B n
-as one would try each entry exactly once. A sucessful connection
-resets the counter. (default=umlimited).
+as one would try each entry exactly once. A successful connection
+resets the counter. (default=unlimited).
 .\"*********************************************************
 .TP
 .B \-\-show\-proxy\-settings

src/openvpn/init.c +1 -0

index 498d36f..247a526 100644
@@ -1955,6 +1955,7 @@ static void
 socket_restart_pause (struct context *c)
 {
   int sec = 2;
+  int backoff = 0;
 
   switch (c->options.ce.proto)
     {

- sndbuf and recvbuf default now to OS default instead of 64k

Review User-visible Changes · line 120

Quoted 3 lines

(quoted diff) +0 -0

@@ -120,6 +120,10 @@ User-visible Changes
 - --http-proxy-retry and --sock-proxy-retry have been removed. Proxy

connections

Review User-visible Changes · line 121

Quoted 1 line

(quoted diff) +0 -0

     will now behave like regular connection entries and generate a USR1

on failure.

Review User-visible Changes · line 122

Quoted 2 lines

(quoted diff) +1 -0

+- --connect-retry gets a optional third argument that specifies the

maximum

Review User-visible Changes · line 123

Quoted 1 line

(quoted diff) +1 -0

+  time in seconds to wait between reconnection attempts when an

exponential

Changes.rst User-visible Changes · line 125

Quoted 1 line

Changes.rst +1 -0

+  backoff is triggered due to repeated retries. Default = 300 seconds.

Shouldn't that be 2nd argument?

+ backoff = (c->options.unsuccessful_attempts /
c->options.connection_list->len) - 4;

src/openvpn/init.c socket_restart_pause (struct context *c) · line 1983

Quoted 1 line

src/openvpn/init.c +1 -0

+  if (backoff > 0 && sec < 1<<16)

the 1<<16 seems a bit random here. Could you at least add a comment of why? I don't see any valid reason here since sec is limited between 1 and connect_retry_seconds_max by the next two lines anyway.

src/openvpn/init.c socket_restart_pause (struct context *c) · line 1984

Quoted 1 line

src/openvpn/init.c +1 -0

+    sec = max_int (sec, 1) << min_int (backoff, 15);

The 15 here is the same. Both lines seem to only make sense with 16 bit integers.

+ if (sec > c->options.ce.connect_retry_seconds_max)

src/openvpn/init.c socket_restart_pause (struct context *c) · line 1986

Quoted 1 line

src/openvpn/init.c +1 -0

+    sec = c->options.ce.connect_retry_seconds_max;

Oh, my suggestion made the max time time configurable per connection entry. We should address that in a seperate patch. I think connect-retry as a whole can be made a non connection config entry. (connect-retry traditionally was the time between tcp connects for a tcp connection which looped on its own before the dual stack patch if anyone wonders why it was connection specific).

Something like this is needed to protect against overflow of the 32 bit int (signed int in this case). Think of the user specifying sec = 2^17. Any shift by more than 14 will make sec = 0. A more sophisticated logic that counts the bits in sec before shifting is possible, but this looked simple.. Now that a v3 is needed, will change to an explicit check on overflow.

Quoted 7 lines

Something like this is needed to protect against overflow of the 32 bit int (signed int in this case). Think of the user specifying sec = 2^17. Any shift by more than 14 will make sec = 0. A more sophisticated logic that counts the bits in sec before shifting is possible, but this looked simple.. Now that a v3 is needed, will change to an explicit check on overflow.

That do that check in options.c and complain there. :)

+ /* Slow down reconnection after 5 retries per remote */

Quoted 1 line

+ backoff = (c->options.unsuccessful_attempts /

c->options.connection_list->len) - 4;

src/openvpn/init.c socket_restart_pause (struct context *c)

Quoted 6 lines

src/openvpn/init.c +5 -0

+  if (backoff > 0 && sec < 1<<16)
+    sec = max_int (sec, 1) << min_int (backoff, 15);
+  if (sec > c->options.ce.connect_retry_seconds_max)
+    sec = c->options.ce.connect_retry_seconds_max;
+
   if (c->persist.restart_sleep_seconds > 0 &&

c->persist.restart_sleep_seconds > sec)

Quoted 3 lines

     sec = c->persist.restart_sleep_seconds;

else if (c->persist.restart_sleep_seconds == -1)

I think this also needs an excempt for TCP_SERVER case. I think moving the TCP_SERVER if below the exponential logic will work. Otherwise connecting just 10 times is an effect dos for a tcp server.

Or do you have tcp-client in mind? In case of tcp-client, before this patch, "--connect-retry 0" will hammer the server with repeated retries. That will be avoided as max sec defaults to 300 sec, unless the user specifies "--connect-retry 0 0". Shall we require that the second arg to connect-retry should be greater than some minimum, say, at least 10 sec?

Quoted 6 lines

Or do you have tcp-client in mind? In case of tcp-client, before this patch, "--connect-retry 0" will hammer the server with repeated retries. That will be avoided as max sec defaults to 300 sec, unless the user specifies "--connect-retry 0 0". Shall we require that the second arg to connect-retry should be greater than some minimum, say, at least 10 sec?

No, the TCP-SERVER line there is for p2p mode but with tcp-server. In p2p mode with p2p the code will also execute that socket-restart code after the other side will disconnect.

Quoted 10 lines · Arne Schwabe

Quoted 6 lines

Or do you have tcp-client in mind? In case of tcp-client, before this patch, "--connect-retry 0" will hammer the server with repeated retries. That will be avoided as max sec defaults to 300 sec, unless the user specifies "--connect-retry 0 0". Shall we require that the second arg to connect-retry should be greater than some minimum, say, at least 10 sec?

No, the TCP-SERVER line there is for p2p mode but with tcp-server. In p2p mode with p2p the code will also execute that socket-restart code after the other side will disconnect.

In the p2p case the tcp server will just do a SIGUSR1 restart and wait for the client, isn't it -- at least that's what my tests showed on Linux. With "--connect-retry 0", the pause time for tcp-server before restart will be "1, 1, 1, 1, 1, 2, 4, 8 .." seconds until it reaches the max value. If I move the "if for TCP_SERVER" to below the exponential backoff, the server restart pause interval will stay fixed at 1 sec with no backoff. Is that what is preferred? Or am I missing something?

- When the number of retries per remote exceeds a limit (hard coded to 5), double the restart pause interval for each additional retry per remote.
- Trigger a SIGHUP to reset the retry count when the pause interval exceeds 1024 times the base value of restart pause. (removed in v2 of the patch)

The base value of restart pause is set using --connect-retry (5 seconds by default).

v2 changes (based on suggestions from Arne Schwabe <[email protected]...>)

- Do not throw SIGHUP.
- Add an optional argument to "--connect-retry n [m]" where 'm' specifies the max value of restart pause interval (default 300 sec).
E.g., "--connect-retry 5 1800" will cause the restart pause to scale up starting at 5 until it exceeds 1800 seconds at which point it gets capped at 1800.
- If n == m no slow down will occur.
- While at it, fix typos and clarify the description of connect-retry-max in the man page and Changes.rst

Signed-off-by Selva Nair <[email protected]...>

---
 Changes.rst           |    8 ++++++--
 doc/openvpn.8         |   19 ++++++++++++-------
 src/openvpn/init.c    |    8 ++++++++
 src/openvpn/options.c |   13 ++++++++++---
 src/openvpn/options.h |    1 +
 5 files changed, 37 insertions(+), 12 deletions(-)

Changes.rst +6 -2

index d12cdad..59722e2 100644
@@ -75,8 +75,8 @@ User-visible Changes
   In --static mode connect-timeout specifies the timeout for TCP and
   proxy connection establishment
 
-- connect-retry now specifies the maximum number of unsucessfully
-  trying all remote/connection entries before exiting.
+- connect-retry-max now specifies the maximum number of unsuccessful
+  attempts of each remote/connection entry before exiting.
 
 - sndbuf and recvbuf default now to OS default instead of 64k
 

@@ -120,6 +120,10 @@ User-visible Changes
 - --http-proxy-retry and --sock-proxy-retry have been removed. Proxy connections
     will now behave like regular connection entries and generate a USR1 on failure.
 
+- --connect-retry gets a optional third argument that specifies the maximum
+  time in seconds to wait between reconnection attempts when an exponential
+  backoff is triggered due to repeated retries. Default = 300 seconds.
+
 Maintainer-visible changes
 --------------------------
 - OpenVPN no longer supports building with crypto support, but without TLS

doc/openvpn.8 +12 -7

index 64cc934..3ca6f50 100644
@@ -462,22 +462,27 @@ application-level UDP protocols, or tunneling protocols which don't
 possess a built-in reliability layer.
 .\"*********************************************************
 .TP
-.B \-\-connect\-retry n
+.B \-\-connect\-retry n [max]
 Wait
 .B n
-seconds  between connection attempts (default=5).
+seconds  between connection attempts (default=5). Repeated reconnection
+attempts are slowed down after 5 retries per remote by doubling the wait
+time after each unsuccessful attempt. The optional argument
+.B max
+specifies the maximum value of wait time in seconds at which it gets
+capped (default=300).
 .\"*********************************************************
 .TP
 .B \-\-connect\-retry\-max n
 .B n
-specifies the number of times all
+specifies the number of times each
 .B \-\-remote
-respectively
+or
 .B <connection>
-statements are tried. Specifiying
+entry is tried. Specifying
 .B n
-as one would try each entry exactly once. A sucessful connection
-resets the counter. (default=umlimited).
+as one would try each entry exactly once. A successful connection
+resets the counter. (default=unlimited).
 .\"*********************************************************
 .TP
 .B \-\-show\-proxy\-settings

src/openvpn/init.c +1 -0

index 498d36f..247a526 100644
@@ -1955,6 +1955,7 @@ static void
 socket_restart_pause (struct context *c)
 {
   int sec = 2;
+  int backoff = 0;
 
   switch (c->options.ce.proto)
     {

@@ -1977,6 +1978,13 @@ socket_restart_pause (struct context *c)
     sec = 10;
 #endif
 

- When the number of retries per remote exceeds a limit (hard coded to 5), double the restart pause interval for each additional retry per remote.