diff mbox series

[ovs-dev,2/2] bridge: Only an inactivity_probe of 0 should turn off inactivity probes.

Message ID 20210610224934.2200271-2-blp@ovn.org
State New
Headers show
Series [ovs-dev,1/2] fail-open: Only fail open if we've been disconnected for at least 1 s. | expand

Commit Message

Ben Pfaff June 10, 2021, 10:49 p.m. UTC
The documentation for inactivity_probe says this:

       inactivity_probe: optional integer
              Maximum  number  of milliseconds of idle time on connec‐
              tion to controller before sending  an  inactivity  probe
              message.  If  Open vSwitch does not communicate with the
              controller for the specified number of seconds, it  will
              send a probe. If a response is not received for the same
              additional amount of time, Open vSwitch assumes the con‐
              nection  has  been broken and attempts to reconnect. De‐
              fault is implementation-specific. A value of 0  disables
              inactivity probes.

This means that a value of 0 should disable inactivity probes and any
other value should be in milliseconds.  The code in bridge.c was
actually interpreting it as any value between 0 and 999 disabling
inactivity probes.  That was surprising when I accidentally configured
it to 5 or to 10, not remembering that it was in milliseconds, and
disabled them entirely.  This fixes the problem.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 vswitchd/bridge.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ilya Maximets June 14, 2021, 7:08 p.m. UTC | #1
On 6/11/21 12:49 AM, Ben Pfaff wrote:
> The documentation for inactivity_probe says this:
> 
>        inactivity_probe: optional integer
>               Maximum  number  of milliseconds of idle time on connec‐
>               tion to controller before sending  an  inactivity  probe
>               message.  If  Open vSwitch does not communicate with the
>               controller for the specified number of seconds, it  will
>               send a probe. If a response is not received for the same
>               additional amount of time, Open vSwitch assumes the con‐
>               nection  has  been broken and attempts to reconnect. De‐
>               fault is implementation-specific. A value of 0  disables
>               inactivity probes.
> 
> This means that a value of 0 should disable inactivity probes and any
> other value should be in milliseconds.  The code in bridge.c was
> actually interpreting it as any value between 0 and 999 disabling
> inactivity probes.  That was surprising when I accidentally configured
> it to 5 or to 10, not remembering that it was in milliseconds, and
> disabled them entirely.  This fixes the problem.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  vswitchd/bridge.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 5ed7e8234354..9c1ee3cf06fe 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -3965,8 +3965,10 @@ bridge_configure_remotes(struct bridge *br,
>          *oc = (struct ofproto_controller) {
>              .type = get_controller_ofconn_type(c->target, c->type),
>              .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8,
> -            .probe_interval = (c->inactivity_probe
> -                               ? *c->inactivity_probe / 1000 : 5),
> +            .probe_interval = (!c->inactivity_probe ? 5
> +                               : !*c->inactivity_probe ? 0
> +                               : *c->inactivity_probe < 1000 ? 1
> +                               : *c->inactivity_probe / 1000),
>              .band = ((!c->connection_mode
>                        || !strcmp(c->connection_mode, "in-band"))
>                       && !disable_in_band
> 

This change looks fine in terms that it fixes disabling of the
inactivity probe, so:

  Acked-by: Ilya Maximets <i.maximets@ovn.org>

OTOH, it's unclear why the DB schema allows configuration in
milliseconds if the implementation doesn't honor the configured
value rounding it up or down.
diff mbox series

Patch

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 5ed7e8234354..9c1ee3cf06fe 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3965,8 +3965,10 @@  bridge_configure_remotes(struct bridge *br,
         *oc = (struct ofproto_controller) {
             .type = get_controller_ofconn_type(c->target, c->type),
             .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8,
-            .probe_interval = (c->inactivity_probe
-                               ? *c->inactivity_probe / 1000 : 5),
+            .probe_interval = (!c->inactivity_probe ? 5
+                               : !*c->inactivity_probe ? 0
+                               : *c->inactivity_probe < 1000 ? 1
+                               : *c->inactivity_probe / 1000),
             .band = ((!c->connection_mode
                       || !strcmp(c->connection_mode, "in-band"))
                      && !disable_in_band