diff mbox series

[ovs-dev,v2] ovn-controller: Fix nb_cfg update with monitor_cond_change in flight.

Message ID 1605631820-28801-1-git-send-email-dceara@redhat.com
State New
Headers show
Series [ovs-dev,v2] ovn-controller: Fix nb_cfg update with monitor_cond_change in flight. | expand

Commit Message

Dumitru Ceara Nov. 17, 2020, 4:50 p.m. UTC
It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
value to ofctrl_put() if there are pending changes to conditional
monitoring clauses (local or in flight).  It might be that after the
monitor condition is acked by the SB, records that were added to the SB
before SB_Global.nb_cfg was set are now sent as updates to
ovn-controller.  These should be first installed in OVS before
ovn-controller reports that it caught up with the current
SB_Global.nb_cfg value.

Also, ofctrl_put should not advance cur_cfg if there are flow updates in
flight.

Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---
v2:
- use new IDL *set_condition() return value.
- fix ofctrl_put to not advance cur_cfg if there are flow updates in
  flight.
---
 controller/ofctrl.c         |  2 +-
 controller/ovn-controller.c | 75 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 54 insertions(+), 23 deletions(-)

Comments

Ben Pfaff Nov. 19, 2020, 4:15 a.m. UTC | #1
On Tue, Nov 17, 2020 at 05:50:20PM +0100, Dumitru Ceara wrote:
> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
> value to ofctrl_put() if there are pending changes to conditional
> monitoring clauses (local or in flight).  It might be that after the
> monitor condition is acked by the SB, records that were added to the SB
> before SB_Global.nb_cfg was set are now sent as updates to
> ovn-controller.  These should be first installed in OVS before
> ovn-controller reports that it caught up with the current
> SB_Global.nb_cfg value.
> 
> Also, ofctrl_put should not advance cur_cfg if there are flow updates in
> flight.
> 
> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> 
> ---
> v2:
> - use new IDL *set_condition() return value.
> - fix ofctrl_put to not advance cur_cfg if there are flow updates in
>   flight.

ovn-controller has changed (advanced?) to the point that I have trouble
understanding the code now.  I'm going to assume that you understand it
pretty well, but please allow me to ask a question here.  Will this
always manage to come up to date with some version of the sb, or is
there a chance that it never will report a consistent value if the sb
keeps changing quickly?  I wasn't able to figure that out with a quick
look.
Dumitru Ceara Nov. 19, 2020, 10:50 a.m. UTC | #2
On 11/19/20 5:15 AM, Ben Pfaff wrote:
> On Tue, Nov 17, 2020 at 05:50:20PM +0100, Dumitru Ceara wrote:
>> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
>> value to ofctrl_put() if there are pending changes to conditional
>> monitoring clauses (local or in flight).  It might be that after the
>> monitor condition is acked by the SB, records that were added to the SB
>> before SB_Global.nb_cfg was set are now sent as updates to
>> ovn-controller.  These should be first installed in OVS before
>> ovn-controller reports that it caught up with the current
>> SB_Global.nb_cfg value.
>>
>> Also, ofctrl_put should not advance cur_cfg if there are flow updates in
>> flight.
>>
>> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.")
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>>
>> ---
>> v2:
>> - use new IDL *set_condition() return value.
>> - fix ofctrl_put to not advance cur_cfg if there are flow updates in
>>   flight.
> 
> ovn-controller has changed (advanced?) to the point that I have trouble

I would say "became more complex"..

> understanding the code now.  I'm going to assume that you understand it
> pretty well, but please allow me to ask a question here.  Will this

I'm doing my best but there are so many dependencies and side effects in
the code..

> always manage to come up to date with some version of the sb, or is
> there a chance that it never will report a consistent value if the sb
> keeps changing quickly?  I wasn't able to figure that out with a quick
> look.
> 

Now, back to your question:

TLDR: If ovn-controller has to continuously change its monitor
conditions I *think* there is a chance that it will never report that is
caught up with a specific SB seqno.  However, I tried various scenarios
(with quick SB updates or claiming/releasing OVS interfaces in quick
succession) and I didn't manage to make "ovn-nbctl --wait=hv sync" block
indefinitely.

Moreover, I think the question is if it's correct for ovn-controller to
report it has caught up if there are unseen SB flows that correspond to
its currently requested but not acked monitor condition?

Long version:

The main processing loop in ovn-controller looks something like this:

a. In the beginning of the loop monitor_cond_change requests (due to the
previous iteration) are sent when ovsdb_idl_loop_run(&ovnsb_idl_loop) is
executed.  monitor_cond_change replies for older requests are also
handled here.

b. Local datapaths (along with a lot of other stuff) are updated inside
the incremental processing engine in engine_run().

c. ovn-controller requests OVS to update flows using as barrier seqno
the last nb_cfg value read when no monitor_cond_change was pending.

d. Then monitor conditions are changed (if local datapaths have changed)
in update_sb_monitors().

e. Get the last seqno confirmed by OVS (ofctrl_get_cur_cfg()) and store
it in chassis_private.nb_cfg.

If in every iteration at step "b" above local datapaths change (due to
SB changes or OVS interfaces being claimed/released) then in the next
iteration we'll be requesting a cond change at step "a".

If at some point at step "a" SB_Global.nb_cfg gets updated, this won't
get propagated to ofctrl in step "c" until monitor conditions stop
changing for at least one iteration.

However, in practice:
- If SB changes cause monitor_cond_change that means port_bindings are
being added/deleted/updated but that can happen, I *think*, only with
ovn-northd intervention but ovn-northd should be already waiting for
ovn-controller to catch up "ovn-nbctl --wait=hv sync".
- If OVS ports are continuously claimed/released I think it's safe to
assume that ovn-controller is never really up to date with the contents
of the SB.

Another issue i see now is that steps "c" and "d" should probably be
swapped.  I can do that in a v3 if you're OK with the new semantics of
nb_cfg.

P.S.: Sorry for the long email.

Regards,
Dumitru
Ben Pfaff Nov. 19, 2020, 5:59 p.m. UTC | #3
On Thu, Nov 19, 2020 at 11:50:57AM +0100, Dumitru Ceara wrote:
> On 11/19/20 5:15 AM, Ben Pfaff wrote:
> > On Tue, Nov 17, 2020 at 05:50:20PM +0100, Dumitru Ceara wrote:
> >> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
> >> value to ofctrl_put() if there are pending changes to conditional
> >> monitoring clauses (local or in flight).  It might be that after the
> >> monitor condition is acked by the SB, records that were added to the SB
> >> before SB_Global.nb_cfg was set are now sent as updates to
> >> ovn-controller.  These should be first installed in OVS before
> >> ovn-controller reports that it caught up with the current
> >> SB_Global.nb_cfg value.
> >>
> >> Also, ofctrl_put should not advance cur_cfg if there are flow updates in
> >> flight.

...

> > ovn-controller has changed (advanced?) to the point that I have trouble
> > understanding the code now.  I'm going to assume that you understand it
> > pretty well, but please allow me to ask a question here.  Will this
> 
> I'm doing my best but there are so many dependencies and side effects in
> the code..

DDlog might actually help with that, eventually.  It should allow us to
program in the problem domain, rather than writing a lot of code to
manage dependencies.

> > always manage to come up to date with some version of the sb, or is
> > there a chance that it never will report a consistent value if the sb
> > keeps changing quickly?  I wasn't able to figure that out with a quick
> > look.
> > 
> 
> Now, back to your question:
> 
> TLDR: If ovn-controller has to continuously change its monitor
> conditions I *think* there is a chance that it will never report that is
> caught up with a specific SB seqno.  However, I tried various scenarios
> (with quick SB updates or claiming/releasing OVS interfaces in quick
> succession) and I didn't manage to make "ovn-nbctl --wait=hv sync" block
> indefinitely.

That's good, empirically.

> Moreover, I think the question is if it's correct for ovn-controller to
> report it has caught up if there are unseen SB flows that correspond to
> its currently requested but not acked monitor condition?

I think the answer is "no", which means that the next question is, how
do we implement it properly?  I don't have an immediate answer.  The
database interfacing code is rather complex these days.
Ben Pfaff Nov. 19, 2020, 6 p.m. UTC | #4
On Thu, Nov 19, 2020 at 09:59:04AM -0800, Ben Pfaff wrote:
> On Thu, Nov 19, 2020 at 11:50:57AM +0100, Dumitru Ceara wrote:
> > On 11/19/20 5:15 AM, Ben Pfaff wrote:
> > > On Tue, Nov 17, 2020 at 05:50:20PM +0100, Dumitru Ceara wrote:
> > >> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
> > >> value to ofctrl_put() if there are pending changes to conditional
> > >> monitoring clauses (local or in flight).  It might be that after the
> > >> monitor condition is acked by the SB, records that were added to the SB
> > >> before SB_Global.nb_cfg was set are now sent as updates to
> > >> ovn-controller.  These should be first installed in OVS before
> > >> ovn-controller reports that it caught up with the current
> > >> SB_Global.nb_cfg value.
> > >>
> > >> Also, ofctrl_put should not advance cur_cfg if there are flow updates in
> > >> flight.
> 
> ...
> 
> > > ovn-controller has changed (advanced?) to the point that I have trouble
> > > understanding the code now.  I'm going to assume that you understand it
> > > pretty well, but please allow me to ask a question here.  Will this
> > 
> > I'm doing my best but there are so many dependencies and side effects in
> > the code..
> 
> DDlog might actually help with that, eventually.  It should allow us to
> program in the problem domain, rather than writing a lot of code to
> manage dependencies.
> 
> > > always manage to come up to date with some version of the sb, or is
> > > there a chance that it never will report a consistent value if the sb
> > > keeps changing quickly?  I wasn't able to figure that out with a quick
> > > look.
> > > 
> > 
> > Now, back to your question:
> > 
> > TLDR: If ovn-controller has to continuously change its monitor
> > conditions I *think* there is a chance that it will never report that is
> > caught up with a specific SB seqno.  However, I tried various scenarios
> > (with quick SB updates or claiming/releasing OVS interfaces in quick
> > succession) and I didn't manage to make "ovn-nbctl --wait=hv sync" block
> > indefinitely.
> 
> That's good, empirically.
> 
> > Moreover, I think the question is if it's correct for ovn-controller to
> > report it has caught up if there are unseen SB flows that correspond to
> > its currently requested but not acked monitor condition?
> 
> I think the answer is "no", which means that the next question is, how
> do we implement it properly?  I don't have an immediate answer.  The
> database interfacing code is rather complex these days.

Oh, let me give this patch a conditional "ack".   If you are pretty
confident that it makes things better, then it has my blessing:
Acked-by: Ben Pfaff <blp@ovn.org>
Dumitru Ceara Nov. 20, 2020, 5:43 p.m. UTC | #5
On 11/19/20 7:00 PM, Ben Pfaff wrote:
> On Thu, Nov 19, 2020 at 09:59:04AM -0800, Ben Pfaff wrote:
>> On Thu, Nov 19, 2020 at 11:50:57AM +0100, Dumitru Ceara wrote:
>>> On 11/19/20 5:15 AM, Ben Pfaff wrote:
>>>> On Tue, Nov 17, 2020 at 05:50:20PM +0100, Dumitru Ceara wrote:
>>>>> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg
>>>>> value to ofctrl_put() if there are pending changes to conditional
>>>>> monitoring clauses (local or in flight).  It might be that after the
>>>>> monitor condition is acked by the SB, records that were added to the SB
>>>>> before SB_Global.nb_cfg was set are now sent as updates to
>>>>> ovn-controller.  These should be first installed in OVS before
>>>>> ovn-controller reports that it caught up with the current
>>>>> SB_Global.nb_cfg value.
>>>>>
>>>>> Also, ofctrl_put should not advance cur_cfg if there are flow updates in
>>>>> flight.
>>
>> ...
>>
>>>> ovn-controller has changed (advanced?) to the point that I have trouble
>>>> understanding the code now.  I'm going to assume that you understand it
>>>> pretty well, but please allow me to ask a question here.  Will this
>>>
>>> I'm doing my best but there are so many dependencies and side effects in
>>> the code..
>>
>> DDlog might actually help with that, eventually.  It should allow us to
>> program in the problem domain, rather than writing a lot of code to
>> manage dependencies.
>>

I tend to agree to this.  Although it still seems like a long way until
ovn-controller would use DDlog.

>>>> always manage to come up to date with some version of the sb, or is
>>>> there a chance that it never will report a consistent value if the sb
>>>> keeps changing quickly?  I wasn't able to figure that out with a quick
>>>> look.
>>>>
>>>
>>> Now, back to your question:
>>>
>>> TLDR: If ovn-controller has to continuously change its monitor
>>> conditions I *think* there is a chance that it will never report that is
>>> caught up with a specific SB seqno.  However, I tried various scenarios
>>> (with quick SB updates or claiming/releasing OVS interfaces in quick
>>> succession) and I didn't manage to make "ovn-nbctl --wait=hv sync" block
>>> indefinitely.
>>
>> That's good, empirically.
>>
>>> Moreover, I think the question is if it's correct for ovn-controller to
>>> report it has caught up if there are unseen SB flows that correspond to
>>> its currently requested but not acked monitor condition?
>>
>> I think the answer is "no", which means that the next question is, how
>> do we implement it properly?  I don't have an immediate answer.  The
>> database interfacing code is rather complex these days.

So that means that even if in the theoretical situation I described
above ovn-controller never reports that it caught up until all
conditions are acked that's not a real problem.  Which will be the new
behavior if this patch is accepted.

> 
> Oh, let me give this patch a conditional "ack".   If you are pretty
> confident that it makes things better, then it has my blessing:
> Acked-by: Ben Pfaff <blp@ovn.org>
> 

Thanks!  I'll send a new version next week.
diff mbox series

Patch

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 79529d1..1e07c5d 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -2033,7 +2033,7 @@  ofctrl_put(struct ovn_desired_flow_table *flow_table,
         need_put = true;
     } else if (nb_cfg != old_nb_cfg) {
         /* nb_cfg changed since last ofctrl_put() call */
-        if (cur_cfg == old_nb_cfg) {
+        if (cur_cfg == old_nb_cfg && ovs_list_is_empty(&flow_updates)) {
             /* we were up-to-date already, so just update with the
              * new nb_cfg */
             cur_cfg = nb_cfg;
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index a06cae3..90fa81e 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -130,7 +130,7 @@  get_bridge(const struct ovsrec_bridge_table *bridge_table, const char *br_name)
     return NULL;
 }
 
-static void
+static unsigned int
 update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
                    const struct sbrec_chassis *chassis,
                    const struct sset *local_ifaces,
@@ -236,16 +236,24 @@  update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
         }
     }
 
-out:
-    sbrec_port_binding_set_condition(ovnsb_idl, &pb);
-    sbrec_logical_flow_set_condition(ovnsb_idl, &lf);
-    sbrec_mac_binding_set_condition(ovnsb_idl, &mb);
-    sbrec_multicast_group_set_condition(ovnsb_idl, &mg);
-    sbrec_dns_set_condition(ovnsb_idl, &dns);
-    sbrec_controller_event_set_condition(ovnsb_idl, &ce);
-    sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast);
-    sbrec_igmp_group_set_condition(ovnsb_idl, &igmp);
-    sbrec_chassis_private_set_condition(ovnsb_idl, &chprv);
+out:;
+    unsigned int cond_seqnos[] = {
+        sbrec_port_binding_set_condition(ovnsb_idl, &pb),
+        sbrec_logical_flow_set_condition(ovnsb_idl, &lf),
+        sbrec_mac_binding_set_condition(ovnsb_idl, &mb),
+        sbrec_multicast_group_set_condition(ovnsb_idl, &mg),
+        sbrec_dns_set_condition(ovnsb_idl, &dns),
+        sbrec_controller_event_set_condition(ovnsb_idl, &ce),
+        sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast),
+        sbrec_igmp_group_set_condition(ovnsb_idl, &igmp),
+        sbrec_chassis_private_set_condition(ovnsb_idl, &chprv),
+    };
+
+    unsigned int expected_cond_seqno = 0;
+    for (size_t i = 0; i < ARRAY_SIZE(cond_seqnos); i++) {
+        expected_cond_seqno = MAX(expected_cond_seqno, cond_seqnos[i]);
+    }
+
     ovsdb_idl_condition_destroy(&pb);
     ovsdb_idl_condition_destroy(&lf);
     ovsdb_idl_condition_destroy(&mb);
@@ -255,6 +263,7 @@  out:
     ovsdb_idl_condition_destroy(&ip_mcast);
     ovsdb_idl_condition_destroy(&igmp);
     ovsdb_idl_condition_destroy(&chprv);
+    return expected_cond_seqno;
 }
 
 static const char *
@@ -488,7 +497,7 @@  get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl)
 static void
 update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
              bool *monitor_all_p, bool *reset_ovnsb_idl_min_index,
-             bool *enable_lflow_cache)
+             bool *enable_lflow_cache, unsigned int *sb_cond_seqno)
 {
     const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl);
     if (!cfg) {
@@ -513,7 +522,11 @@  update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
          * Otherwise, don't call it here, because there would be unnecessary
          * extra cost. Instead, it is called after the engine execution only
          * when it is necessary. */
-        update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true);
+        unsigned int next_cond_seqno =
+            update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true);
+        if (sb_cond_seqno) {
+            *sb_cond_seqno = next_cond_seqno;
+        }
     }
     if (monitor_all_p) {
         *monitor_all_p = monitor_all;
@@ -726,11 +739,23 @@  restore_ct_zones(const struct ovsrec_bridge_table *bridge_table,
 }
 
 static int64_t
-get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table)
+get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table,
+           unsigned int cond_seqno, unsigned int expected_cond_seqno)
 {
+    static int64_t nb_cfg = 0;
+
+    /* Delay getting nb_cfg if there are monitor condition changes
+     * in flight.  It might be that those changes would instruct the
+     * server to send updates that happened before SB_Global.nb_cfg.
+     */
+    if (cond_seqno != expected_cond_seqno) {
+        return nb_cfg;
+    }
+
     const struct sbrec_sb_global *sb
         = sbrec_sb_global_table_first(sb_global_table);
-    return sb ? sb->nb_cfg : 0;
+    nb_cfg = sb ? sb->nb_cfg : 0;
+    return nb_cfg;
 }
 
 static const char *
@@ -2423,6 +2448,7 @@  main(int argc, char *argv[])
 
     unsigned int ovs_cond_seqno = UINT_MAX;
     unsigned int ovnsb_cond_seqno = UINT_MAX;
+    unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
 
     struct controller_engine_ctx ctrl_engine_ctx = {
         .enable_lflow_cache = true
@@ -2457,7 +2483,8 @@  main(int argc, char *argv[])
 
         update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all,
                      &reset_ovnsb_idl_min_index,
-                     &ctrl_engine_ctx.enable_lflow_cache);
+                     &ctrl_engine_ctx.enable_lflow_cache,
+                     &ovnsb_expected_cond_seqno);
         update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
         ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
 
@@ -2574,7 +2601,9 @@  main(int argc, char *argv[])
                                    &ct_zones_data->pending,
                                    sbrec_meter_table_get(ovnsb_idl_loop.idl),
                                    get_nb_cfg(sbrec_sb_global_table_get(
-                                                   ovnsb_idl_loop.idl)),
+                                                   ovnsb_idl_loop.idl),
+                                              ovnsb_cond_seqno,
+                                              ovnsb_expected_cond_seqno),
                                    engine_node_changed(&en_flow_output));
                     }
                     runtime_data = engine_get_data(&en_runtime_data);
@@ -2602,10 +2631,12 @@  main(int argc, char *argv[])
                                     &runtime_data->local_datapaths,
                                     &runtime_data->active_tunnels);
                         if (engine_node_changed(&en_runtime_data)) {
-                            update_sb_monitors(ovnsb_idl_loop.idl, chassis,
-                                               &runtime_data->local_lports,
-                                               &runtime_data->local_datapaths,
-                                               sb_monitor_all);
+                            ovnsb_expected_cond_seqno =
+                                update_sb_monitors(
+                                    ovnsb_idl_loop.idl, chassis,
+                                    &runtime_data->local_lports,
+                                    &runtime_data->local_datapaths,
+                                    sb_monitor_all);
                         }
                     }
                 }
@@ -2723,7 +2754,7 @@  loop_done:
         bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl);
         while (!done) {
             update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
-                         NULL, NULL, NULL);
+                         NULL, NULL, NULL, NULL);
             update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
 
             struct ovsdb_idl_txn *ovs_idl_txn