diff mbox series

[ovs-dev,v3] lflow: Add MAC bindings when new datapath is added to chassis

Message ID 20220426114245.1730475-1-amusil@redhat.com
State Accepted
Headers show
Series [ovs-dev,v3] lflow: Add MAC bindings when new datapath is added to chassis | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Ales Musil April 26, 2022, 11:42 a.m. UTC
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 <amusil@redhat.com>
---
v2: Update formatting

v3: Add test, address comments from Dumitru
---
 controller/lflow.c          | 26 +++++++++++
 controller/lflow.h          |  2 +
 controller/ovn-controller.c | 23 ++++++++++
 tests/ovn.at                | 90 +++++++++++++++++++++++++++++++++++++
 4 files changed, 141 insertions(+)

Comments

Mark Michelson April 26, 2022, 2:29 p.m. UTC | #1
Thanks Ales for adding a test in this revision. Looks good to me.

Acked-by: Mark Michelson <mmichels@redhat.com>

On 4/26/22 07: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 <amusil@redhat.com>
> ---
> v2: Update formatting
> 
> v3: Add test, address comments from Dumitru
> ---
>   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);
> +    }
> +    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
> +
> +# 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}])
> +}
> +
> +# 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
> +])
>
Dumitru Ceara April 28, 2022, 2:38 p.m. UTC | #2
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 <amusil@redhat.com>
> ---
> 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 <dceara@redhat.com>
Mark Michelson May 10, 2022, 5:19 p.m. UTC | #3
I pushed this change to main, branch-22.03, and branch-21.12. Sorry 
Ales, I didn't notice that you had uploaded a branch-21.12 version of 
the patch so I just did the backport myself.

I also added a couple of Dumitru's suggestions:
* I added "check" calls before all commands where I could in the test.
* I removed a `wc -l` call from the test in favor of putting a -c on the 
previous grep call.

On 4/28/22 10:38, Dumitru Ceara wrote:
> 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 <amusil@redhat.com>
>> ---
>> 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 <dceara@redhat.com>
>
diff mbox series

Patch

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);
+    }
+    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
+
+# 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}])
+}
+
+# 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
+])