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?
>
> 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'?
> + }
> + 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
