Re: [ovs-dev] [PATCH v2] Avoid update probe interval to non-zero for unix socket.

[Han Zhou]

On Tue, May 2, 2017 at 8:18 AM, Ben Pfaff  wrote:
>
> Thanks for the testing and the fix.  Can you please re-send this as a
> full patch?  The version that made it to the list got wordwrapped.
>

ok, I just re-sent the full patch:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/331827.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Avoid update probe interval to non-zero for unix socket.

[Ben Pfaff]

On Mon, May 01, 2017 at 05:31:54PM -0700, Han Zhou wrote:
> On Mon, May 1, 2017 at 11:19 AM, Ben Pfaff  wrote:
> >
> > On Tue, Apr 18, 2017 at 11:12:14AM -0700, Han Zhou wrote:
> > > In commit c1bfdd9d it disables probe when not needed, but commit
> > > 715038b6 updates ovn-controller probe interval for OVNSB DB
> > > periodically according to ovn-remote-probe-interval config, and sets
> > > it to DEFAULT_PROBE_INTERVAL_MSEC if not configured, even if the
> > > connection type is unix socket which doesn't need probe.
> > >
> > > This fix avoids probe interval update if not needed (always set to 0).
> > >
> > > Signed-off-by: Han Zhou 
> > > ---
> > >
> > > Notes:
> > > v1->v2: fix commit id mentioned in commit message.
> >
> > Thanks for reporting and fixing this bug.  I agree there's a bug here,
> > but I'd rather honor the user's explicit request to set a nonzero probe
> > interval, if there is such a request in ovsdb.
> >
> > How about this?  I have not tested it, beyond verifying that it
> > compiles.
> >
> > Thanks,
> >
> > Ben.
> >
> 
> Yes, this idea is better. But the code has a problem.
> 
> > @@ -933,6 +934,8 @@ update_probe_interval(struct controller_ctx *ctx)
> >  ? smap_get_int(>external_ids,
> > "ovn-remote-probe-interval",
> > DEFAULT_PROBE_INTERVAL_MSEC)
> > -: DEFAULT_PROBE_INTERVAL_MSEC);
> > +: stream_or_pstream_needs_probes(ovnsb_remote)
> > +? DEFAULT_PROBE_INTERVAL_MSEC
> > +: 0);
> 
> Here is the fix (tested):

Thanks for the testing and the fix.  Can you please re-send this as a
full patch?  The version that made it to the list got wordwrapped.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Avoid update probe interval to non-zero for unix socket.

[Han Zhou]

On Mon, May 1, 2017 at 11:19 AM, Ben Pfaff  wrote:
>
> On Tue, Apr 18, 2017 at 11:12:14AM -0700, Han Zhou wrote:
> > In commit c1bfdd9d it disables probe when not needed, but commit
> > 715038b6 updates ovn-controller probe interval for OVNSB DB
> > periodically according to ovn-remote-probe-interval config, and sets
> > it to DEFAULT_PROBE_INTERVAL_MSEC if not configured, even if the
> > connection type is unix socket which doesn't need probe.
> >
> > This fix avoids probe interval update if not needed (always set to 0).
> >
> > Signed-off-by: Han Zhou 
> > ---
> >
> > Notes:
> > v1->v2: fix commit id mentioned in commit message.
>
> Thanks for reporting and fixing this bug.  I agree there's a bug here,
> but I'd rather honor the user's explicit request to set a nonzero probe
> interval, if there is such a request in ovsdb.
>
> How about this?  I have not tested it, beyond verifying that it
> compiles.
>
> Thanks,
>
> Ben.
>

Yes, this idea is better. But the code has a problem.

> @@ -933,6 +934,8 @@ update_probe_interval(struct controller_ctx *ctx)
>  ? smap_get_int(>external_ids,
> "ovn-remote-probe-interval",
> DEFAULT_PROBE_INTERVAL_MSEC)
> -: DEFAULT_PROBE_INTERVAL_MSEC);
> +: stream_or_pstream_needs_probes(ovnsb_remote)
> +? DEFAULT_PROBE_INTERVAL_MSEC
> +: 0);

Here is the fix (tested):

--8<--cut here-->8--
diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
index e00f57a..d7423ab 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -64,7 +64,8 @@ static unixctl_cb_func inject_pkt;
 #define DEFAULT_BRIDGE_NAME "br-int"
 #define DEFAULT_PROBE_INTERVAL_MSEC 5000

-static void update_probe_interval(struct controller_ctx *);
+static void update_probe_interval(struct controller_ctx *,
+  const char *ovnsb_remote);
 static void parse_options(int argc, char *argv[]);
 OVS_NO_RETURN static void usage(void);

@@ -594,7 +595,7 @@ main(int argc, char *argv[])
 .ovnsb_idl_txn = ovsdb_idl_loop_run(_idl_loop),
 };

-update_probe_interval();
+update_probe_interval(, ovnsb_remote);

 update_ssl_config(ctx.ovs_idl);

@@ -925,14 +926,21 @@ inject_pkt(struct unixctl_conn *conn, int argc
OVS_UNUSED,
 /* Get the desired SB probe timer from the OVS database and configure it
into
  * the SB database. */
 static void
-update_probe_interval(struct controller_ctx *ctx)
+update_probe_interval(struct controller_ctx *ctx, const char *ovnsb_remote)
 {
 const struct ovsrec_open_vswitch *cfg
 = ovsrec_open_vswitch_first(ctx->ovs_idl);
-int interval = (cfg
-? smap_get_int(>external_ids,
-   "ovn-remote-probe-interval",
-   DEFAULT_PROBE_INTERVAL_MSEC)
-: DEFAULT_PROBE_INTERVAL_MSEC);
+int interval = -1;
+if (cfg) {
+interval = smap_get_int(>external_ids,
+"ovn-remote-probe-interval",
+-1);
+}
+if (interval == -1) {
+interval = stream_or_pstream_needs_probes(ovnsb_remote)
+   ? DEFAULT_PROBE_INTERVAL_MSEC
+   : 0;
+}
+
 ovsdb_idl_set_probe_interval(ctx->ovnsb_idl, interval);
 }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Avoid update probe interval to non-zero for unix socket.

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:12:14AM -0700, Han Zhou wrote:
> In commit c1bfdd9d it disables probe when not needed, but commit
> 715038b6 updates ovn-controller probe interval for OVNSB DB
> periodically according to ovn-remote-probe-interval config, and sets
> it to DEFAULT_PROBE_INTERVAL_MSEC if not configured, even if the
> connection type is unix socket which doesn't need probe.
> 
> This fix avoids probe interval update if not needed (always set to 0).
> 
> Signed-off-by: Han Zhou 
> ---
> 
> Notes:
> v1->v2: fix commit id mentioned in commit message.

Thanks for reporting and fixing this bug.  I agree there's a bug here,
but I'd rather honor the user's explicit request to set a nonzero probe
interval, if there is such a request in ovsdb.

How about this?  I have not tested it, beyond verifying that it
compiles.

Thanks,

Ben.

--8<--cut here-->8--

From: Ben Pfaff 
Date: Mon, 1 May 2017 11:18:20 -0700
Subject: [PATCH] ovn-controller: Disable probes by default for unix sockets.

Normally the OVS JSON-RPC library does not probe idle connections across
Unix domain sockets, since the kernel can tell OVS whether the connections
are truly connected without probes, but ovn-controller carelessly
overrode that.

(This should not be an issue in typical OVN deployments, because the OVN SB
database is normally accessed via TCP or SSL.)

CC: Nirapada Ghosh 
Fixes: 715038b6b222 ("ovn-controller: reload configured SB probe timer")
Reported-by: Han Zhou 
Signed-off-by: Ben Pfaff 
---
 ovn/controller/ovn-controller.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index e00f57ad5516..4e433d2b6378 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -64,7 +64,8 @@ static unixctl_cb_func inject_pkt;
 #define DEFAULT_BRIDGE_NAME "br-int"
 #define DEFAULT_PROBE_INTERVAL_MSEC 5000
 
-static void update_probe_interval(struct controller_ctx *);
+static void update_probe_interval(struct controller_ctx *,
+  const char *ovnsb_remote);
 static void parse_options(int argc, char *argv[]);
 OVS_NO_RETURN static void usage(void);
 
@@ -594,7 +595,7 @@ main(int argc, char *argv[])
 .ovnsb_idl_txn = ovsdb_idl_loop_run(_idl_loop),
 };
 
-update_probe_interval();
+update_probe_interval(, ovnsb_remote);
 
 update_ssl_config(ctx.ovs_idl);
 
@@ -925,7 +926,7 @@ inject_pkt(struct unixctl_conn *conn, int argc OVS_UNUSED,
 /* Get the desired SB probe timer from the OVS database and configure it into
  * the SB database. */
 static void
-update_probe_interval(struct controller_ctx *ctx)
+update_probe_interval(struct controller_ctx *ctx, const char *ovnsb_remote)
 {
 const struct ovsrec_open_vswitch *cfg
 = ovsrec_open_vswitch_first(ctx->ovs_idl);
@@ -933,6 +934,8 @@ update_probe_interval(struct controller_ctx *ctx)
 ? smap_get_int(>external_ids,
"ovn-remote-probe-interval",
DEFAULT_PROBE_INTERVAL_MSEC)
-: DEFAULT_PROBE_INTERVAL_MSEC);
+: stream_or_pstream_needs_probes(ovnsb_remote)
+? DEFAULT_PROBE_INTERVAL_MSEC
+: 0);
 ovsdb_idl_set_probe_interval(ctx->ovnsb_idl, interval);
 }
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] Avoid update probe interval to non-zero for unix socket.

[Han Zhou]

In commit c1bfdd9d it disables probe when not needed, but commit
715038b6 updates ovn-controller probe interval for OVNSB DB
periodically according to ovn-remote-probe-interval config, and sets
it to DEFAULT_PROBE_INTERVAL_MSEC if not configured, even if the
connection type is unix socket which doesn't need probe.

This fix avoids probe interval update if not needed (always set to 0).

Signed-off-by: Han Zhou 
---

Notes:
v1->v2: fix commit id mentioned in commit message.

 lib/reconnect.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/reconnect.c b/lib/reconnect.c
index 471fb7f..6b52481 100644
--- a/lib/reconnect.c
+++ b/lib/reconnect.c
@@ -16,6 +16,7 @@
 
 #include 
 #include "reconnect.h"
+#include "stream.h"
 
 #include 
 
@@ -243,6 +244,9 @@ reconnect_set_backoff(struct reconnect *fsm, int 
min_backoff, int max_backoff)
 void
 reconnect_set_probe_interval(struct reconnect *fsm, int probe_interval)
 {
+if (!stream_or_pstream_needs_probes(fsm->name)) {
+probe_interval = 0;
+}
 fsm->probe_interval = probe_interval ? MAX(1000, probe_interval) : 0;
 }
 
-- 
2.1.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev