On 4/26/22 13:42, 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]>
> ---
> v2: Update formatting
>
> v3: Add test, address comments from Dumitru
> ---
Hi Ales,
> controller/lflow.c | 26 +++++++++++
> controller/lflow.h | 2 +
> controller/ovn-controller.c | 23 ++++++++++
> tests/ovn.at | 90 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 141 insertions(+)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index b7ddeb1c0..66376ad8c 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);
Not introduced by this patch, I just happened to look at the
consider_neighbor_flow() function. There's never a case when we would
pass both a non-null sbrec_mac_binding and a non-null
sbrec_static_mac_binding. I think that function will need a refactor in
the future.
I also don't really understand what was wrong with just adding an
"options" column to SB.Mac_Binding instead of a completely new but
almost exactly the same Static_MAC_Binding table. But that's, also, a
different story.
> + }
> + 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,
> + 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..5a6274eb2 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");
> +
> + struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_static_mac_binding", node),
> + "datapath");
> +
> 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);
>
> 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);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 34b6abfc0..5590c541a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -30496,3 +30496,93 @@ test_mac_binding_flows 150 00:00:11:22:33:88 0
>
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- lr mac_binding I-P update])
> +AT_KEYWORDS([mac_binding])
> +ovn_start
> +
> +# Add chassis
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl add-br br-phys
> +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +ovn_attach n1 br-phys 192.168.0.2
> +
> +as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true
> +as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true
> +
> +# Wait until ovn-monitor-all is processed by ovn-controller.
> +wait_row_count Chassis 1 name=hv1 other_config:ovn-monitor-all=true
> +wait_row_count Chassis 1 name=hv2 other_config:ovn-monitor-all=true
> +
> +ovn-nbctl ls-add public
> +ovn-nbctl ls-add internal
> +
> +ovn-nbctl lsp-add public ln_port
> +ovn-nbctl lsp-set-addresses ln_port unknown
> +ovn-nbctl lsp-set-type ln_port localnet
> +ovn-nbctl lsp-set-options ln_port network_name=phys
> +
> +ovn-nbctl lsp-add public public-gw
> +ovn-nbctl lsp-set-type public-gw router
> +ovn-nbctl lsp-set-addresses public-gw router
> +ovn-nbctl lsp-set-options public-gw router-port=gw-public
> +
> +ovn-nbctl lsp-add internal internal-gw
> +ovn-nbctl lsp-set-type internal-gw router
> +ovn-nbctl lsp-set-addresses internal-gw router
> +ovn-nbctl lsp-set-options internal-gw router-port=gw-internal
> +
> +ovn-nbctl lsp-add internal vif1
> +ovn-nbctl lsp-set-addresses vif1 "00:00:00:00:20:11 192.168.20.11"
> +
> +ovn-nbctl lsp-add internal vif2
> +ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:12 192.168.20.12"
> +
> +ovn-nbctl lr-add gw
> +ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:01 192.168.10.1/24
> +ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:01 192.168.20.1/24
I think I would've prefixed all the ovn-nbctl commands with 'check ' but
given the rest of the test we're probably fine as is.
> +
> +# Add a vif1 on HV1
> +as hv1 ovs-vsctl add-port br-int vif1 -- \
> + set Interface vif1 external-ids:iface-id=vif1 \
> + options:tx_pcap=hv1/vif1-tx.pcap \
> + options:rxq_pcap=hv1/vif1-rx.pcap
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up vif1) = xup])
> +
> +ovn-nbctl lrp-set-gateway-chassis gw-public hv1
> +ovn-nbctl --wait=sb sync
> +
> +test_mac_binding_flows() {
> + local hv=$1 mac=$2 count=$3
> + OVS_WAIT_UNTIL([test $(as $hv ovs-ofctl dump-flows br-int | grep
> table=66 | grep priority=100 | grep actions=mod_dl_dst:${mac} | wc -l) -eq
> ${count}])
Tiny nit: I would add '-c' to the grep arguments instead of 'wc -l'.
> +}
> +
> +# Create SB MAC_Binding entry on external gateway port
> +gw_dp_uuid=$(fetch_column datapath_binding _uuid external_ids:name=gw)
> +ovn-sbctl create mac_binding ip=192.168.10.10 logical_port=gw-public
> mac="00\:00\:00\:00\:10\:10" datapath=$gw_dp_uuid
> +
> +test_mac_binding_flows hv1 00\:00\:00\:00\:10\:10 1
> +test_mac_binding_flows hv2 00\:00\:00\:00\:10\:10 0
> +
> +# Add a vif2 on HV2
> +as hv2 ovs-vsctl add-port br-int vif2 -- \
> + set Interface vif2 external-ids:iface-id=vif2 \
> + options:tx_pcap=hv1/vif2-tx.pcap \
> + options:rxq_pcap=hv1/vif2-rx.pcap
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up vif2) = xup])
> +
> +test_mac_binding_flows hv1 00\:00\:00\:00\:10\:10 1
> +test_mac_binding_flows hv2 00\:00\:00\:00\:10\:10 1
> +
> +OVN_CLEANUP([hv1], [hv2])
> +AT_CLEANUP
> +])
Acked-by: Dumitru Ceara <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
