[ovs-dev] [PATCH 2/3] ovn-controller.c: Move the position of handling OVN-SB related settings.

Han Zhou hzhou at ovn.org
Fri Jan 17 22:31:00 UTC 2020


On Fri, Jan 17, 2020 at 5:14 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Wed, Jan 15, 2020 at 5:34 PM Flavio Fernandes <flavio at flaviof.com>
wrote:
> >
> >
> >
> > > On Jan 14, 2020, at 5:34 PM, Han Zhou <hzhou at ovn.org> wrote:
> > >
> > > On Tue, Jan 14, 2020 at 1:24 PM Mark Michelson <mmichels at redhat.com>
wrote:
> > >>
> > >> The commit message doesn't make much sense to me. The external-ids
are
> > >> set outside of ovn-controller, so the concept of them being handled
in
> > >> "the same iteration" or "the next one" only works if ovn-controller
is
> > >> setting them at some point in the loop.
> > >>
> > > Maybe the commit message is not clear enough. Let me explain with more
> > > details.
> > > In each iteration, the OVS IDL's data is updated AFTER the line:
> > >    struct ovsdb_idl_txn *ovs_idl_txn =
ovsdb_idl_loop_run(&ovs_idl_loop);
> > >
> > > Without this patch, it (the lines that are moved) applies the settings
> > > before reading the new settings. So if a change is made to
external-ids,
> > > e.g. ovn-remote-db, the loop will be waked up because of the OVSDB RPC
> > > messages, but it won't apply any of the new settings. The new
settings will
> > > be applied only if there is another event coming to wake the loop,
e.g.
> > > probe interval timeout. In my testing I saw the change took effect
after 5
> > > seconds when probe interval timed out. If probe was disabled, it would
> > > never got applied unless a new change is made. I suspect the problem
> > > reported here [0] may due to the same reason.
> > >
> > > [0]
> > >
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html
> > >
> > >> Couldn't this have a negative effect on the first iteration of the
loop?
> > >> If we don't set SSL parameters or the sb remote first, then we will
have
> > >> errors when attempting to connect to the southbound database.
> > >>
> > >
> > > At the first iteration, it just make sure the OVS IDL data is
refreshed
> > > before setting the SSL parameters. We are still setting the
parameters. The
> > > patch doesn't skip anything.
> > >
> > >> On 1/13/20 5:52 PM, Han Zhou wrote:
> > >>> Move the logic of handling OVN-SB related setting in external-ids
> > >>> after the ovs_idl_loop run, so that any change in the external-ids
> > >>> settings can take effect in the same iteration, without waiting for
> > >>> the next one.
> > >>>
> > >>> Signed-off-by: Han Zhou <hzhou at ovn.org>
>
> Acked-by: Numan Siddique <numans at ovn.org>
>
> Numan
>
> > >>> ---
> > >>>  controller/ovn-controller.c | 8 ++++----
> > >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/controller/ovn-controller.c
b/controller/ovn-controller.c
> > >>> index 3d843f7..abb1b4c 100644
> > >>> --- a/controller/ovn-controller.c
> > >>> +++ b/controller/ovn-controller.c
> > >>> @@ -2012,10 +2012,6 @@ main(int argc, char *argv[])
> > >>>      while (!exiting) {
> > >>>          engine_init_run();
> > >>>
> > >>> -        update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
> > >>> -        update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
> > >>> -
> > >
ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
> > >>> -
> > >>>          struct ovsdb_idl_txn *ovs_idl_txn =
> > > ovsdb_idl_loop_run(&ovs_idl_loop);
> > >>>          unsigned int new_ovs_cond_seqno
> > >>>              = ovsdb_idl_get_condition_seqno(ovs_idl_loop.idl);
> > >>> @@ -2027,6 +2023,10 @@ main(int argc, char *argv[])
> > >>>              ovs_cond_seqno = new_ovs_cond_seqno;
> > >>>          }
> > >>>
> > >>> +        update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
> > >>> +        update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
> > >>> +
> > >
ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
> > >>> +
> > >>>          struct ovsdb_idl_txn *ovnsb_idl_txn
> > >>>              = ovsdb_idl_loop_run(&ovnsb_idl_loop);
> > >>>          unsigned int new_ovnsb_cond_seqno
> > >>>
> > >>
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> > This change addresses the issue of setting the tunnel as Lars
discovered.
> > I needs to be back merged to 2.12 as well.
> >
> >
> > Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html
<https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html
>
> > Tested-by: Flavio Fernandes <flavio at flaviof.com>
> >
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

Thanks Flavio and Numan.
Mark, could you confirm if your concern was addressed as well?

Thanks,
Han


More information about the dev mailing list