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

[Han Zhou]

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>
> > ---
> >   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
> >
>