diff mbox

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

Message ID CADtzDCme775hZPe8TpKex2HVZbH8N1iWjchYUy3P7o-3CNVQ2g@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Han Zhou May 2, 2017, 12:31 a.m. UTC 
On Mon, May 1, 2017 at 11:19 AM, Ben Pfaff <blp@ovn.org> 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 <zhouhan@gmail.com>
> > ---
> >
> > 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(&cfg->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--
     ovsdb_idl_set_probe_interval(ctx->ovnsb_idl, interval);
 }

Comments

Ben Pfaff May 2, 2017, 3:18 p.m. UTC | #1 
On Mon, May 01, 2017 at 05:31:54PM -0700, Han Zhou wrote:
> On Mon, May 1, 2017 at 11:19 AM, Ben Pfaff <blp@ovn.org> 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 <zhouhan@gmail.com>
> > > ---
> > >
> > > 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(&cfg->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.
Han Zhou May 2, 2017, 8:26 p.m. UTC | #2 
On Tue, May 2, 2017 at 8:18 AM, Ben Pfaff <blp@ovn.org> 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
diff mbox

Patch

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(&ovnsb_idl_loop),
         };

-        update_probe_interval(&ctx);
+        update_probe_interval(&ctx, 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(&cfg->external_ids,
-                                   "ovn-remote-probe-interval",
-                                   DEFAULT_PROBE_INTERVAL_MSEC)
-                    : DEFAULT_PROBE_INTERVAL_MSEC);
+    int interval = -1;
+    if (cfg) {
+        interval = smap_get_int(&cfg->external_ids,
+                                "ovn-remote-probe-interval",
+                                -1);
+    }
+    if (interval == -1) {
+        interval = stream_or_pstream_needs_probes(ovnsb_remote)
+                   ? DEFAULT_PROBE_INTERVAL_MSEC
+                   : 0;
+    }
+