On Tue, Apr 26, 2022 at 10:01 AM Dumitru Ceara <[email protected]> wrote:
> On 4/26/22 07:43, Ales Musil wrote: > > It can happen that MAC binding physical flow is not created > > on when new datapath is added to chassis. > > > > Consider following scenario: > > Chassis are set with "ovn-monitor-all=true". > > > > T0 - The CH0 have only DP0 > > T1 - MAC binding table gets new update for DP1 > > T2 - This is reported to CH0 and ignored as DP1 is not local for CH0 > > T3 - The DP1 is added to CH0 > > > > Under this scenario the MAC binding is never > > considered on the CH0 and the flow table. > > The CH0 is missing those MAC bindings that arrived in > > the time when DP1 was not local. > > > > When chassis gets new datapath add all MAC bindings > > to prevent this issue. > > > > Reported-at: https://bugzilla.redhat.com/2069783 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > Hi Ales, > > Thanks for the fix, it makes sense. I have a few comments though. > > > v2: Update formatting > > --- > > controller/lflow.c | 26 ++++++++++++++++++++++++++ > > controller/lflow.h | 2 ++ > > controller/ovn-controller.c | 23 +++++++++++++++++++++++ > > 3 files changed, 51 insertions(+) > > Would it be possible to add a self test to exercise this code path? > Added in v3. > > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index b7ddeb1c0..f608156f7 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -2646,6 +2646,32 @@ lflow_add_flows_for_datapath(const struct > sbrec_datapath_binding *dp, > > } > > sbrec_fdb_index_destroy_row(fdb_index_row); > > > > + struct sbrec_mac_binding *mb_index_row = > sbrec_mac_binding_index_init_row( > > + l_ctx_in->sbrec_mac_binding_by_datapath); > > + sbrec_mac_binding_index_set_datapath(mb_index_row, dp); > > + const struct sbrec_mac_binding *mb; > > + SBREC_MAC_BINDING_FOR_EACH_EQUAL ( > > + mb, mb_index_row, l_ctx_in->sbrec_mac_binding_by_datapath) { > > + consider_neighbor_flow(l_ctx_in->sbrec_port_binding_by_name, > > + l_ctx_in->local_datapaths, > > + mb, NULL, l_ctx_out->flow_table, 100); > > + } > > + sbrec_mac_binding_index_destroy_row(mb_index_row); > > + > > + struct sbrec_static_mac_binding *smb_index_row = > > + sbrec_static_mac_binding_index_init_row( > > + l_ctx_in->sbrec_static_mac_binding_by_datapath); > > + sbrec_static_mac_binding_index_set_datapath(smb_index_row, dp); > > + const struct sbrec_static_mac_binding *smb; > > + SBREC_STATIC_MAC_BINDING_FOR_EACH_EQUAL ( > > + smb, smb_index_row, > > + l_ctx_in->sbrec_static_mac_binding_by_datapath) { > > + consider_neighbor_flow(l_ctx_in->sbrec_port_binding_by_name, > > + l_ctx_in->local_datapaths, > > + NULL, smb, l_ctx_out->flow_table, 100); > > Shouldn't priority be 'smb->override_dynamic_mac ? 150 : 50'? > Indeed, thanks > > > + } > > + sbrec_static_mac_binding_index_destroy_row(smb_index_row); > > + > > dhcp_opts_destroy(&dhcp_opts); > > dhcp_opts_destroy(&dhcpv6_opts); > > nd_ra_opts_destroy(&nd_ra_opts); > > diff --git a/controller/lflow.h b/controller/lflow.h > > index 4003a15a5..ba2efcebd 100644 > > --- a/controller/lflow.h > > +++ b/controller/lflow.h > > @@ -136,6 +136,8 @@ struct lflow_ctx_in { > > struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group; > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > struct ovsdb_idl_index *sbrec_fdb_by_dp_key; > > + struct ovsdb_idl_index *sbrec_mac_binding_by_datapath; > > + struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath; > > const struct sbrec_port_binding_table *port_binding_table; > > const struct sbrec_dhcp_options_table *dhcp_options_table; > > const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 22b7fa935..1e2a7d306 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -2326,6 +2326,16 @@ init_lflow_ctx(struct engine_node *node, > > engine_get_input("SB_fdb", node), > > "dp_key"); > > > > + struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_mac_binding", node), > > + "datapath"); > > Nit: please match indentation from above this chunk, i.e., use 4 spaces. > > > + > > + struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_static_mac_binding", node), > > + "datapath"); > > + > > Same here. > > > struct sbrec_port_binding_table *port_binding_table = > > (struct sbrec_port_binding_table *)EN_OVSDB_GET( > > engine_get_input("SB_port_binding", node)); > > @@ -2408,6 +2418,9 @@ init_lflow_ctx(struct engine_node *node, > > sbrec_logical_flow_by_dp_group; > > l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > > l_ctx_in->sbrec_fdb_by_dp_key = sbrec_fdb_by_dp_key; > > + l_ctx_in->sbrec_mac_binding_by_datapath = > sbrec_mac_binding_by_datapath; > > + l_ctx_in->sbrec_static_mac_binding_by_datapath = > > + sbrec_static_mac_binding_by_datapath; > > l_ctx_in->port_binding_table = port_binding_table; > > l_ctx_in->dhcp_options_table = dhcp_table; > > l_ctx_in->dhcpv6_options_table = dhcpv6_table; > > @@ -3277,6 +3290,12 @@ main(int argc, char *argv[]) > > = ovsdb_idl_index_create2(ovnsb_idl_loop.idl, > > &sbrec_fdb_col_mac, > > &sbrec_fdb_col_dp_key); > > + struct ovsdb_idl_index *sbrec_mac_binding_by_datapath > > + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > + &sbrec_mac_binding_col_datapath); > > + struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath > > + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > + > &sbrec_static_mac_binding_col_datapath); > > Nit: please match indentation from above this chunk, i.e., use 4 spaces. > > > > > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > > @@ -3511,6 +3530,10 @@ main(int argc, char *argv[]) > > sbrec_datapath_binding_by_key); > > engine_ovsdb_node_add_index(&en_sb_fdb, "dp_key", > > sbrec_fdb_by_dp_key); > > + engine_ovsdb_node_add_index(&en_sb_mac_binding, "datapath", > > + sbrec_mac_binding_by_datapath); > > + engine_ovsdb_node_add_index(&en_sb_static_mac_binding, "datapath", > > + sbrec_static_mac_binding_by_datapath); > > > > struct ed_type_lflow_output *lflow_output_data = > > engine_get_internal_data(&en_lflow_output); > > Thanks, > Dumitru > > Thank you for the review, I have uploaded v3. Regards, Ales -- Ales Musil Senior Software Engineer - RHV Network Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
