| Message ID | 1605631820-28801-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: Fix nb_cfg update with monitor_cond_change in flight. | expand |
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.
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
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.
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>
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 --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
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(-)