On 12/12/24 4:52 PM, Felix Huettner via dev wrote:
> There is no need to set this manually. In all cases where a user would
> set option:centralize_routing they would not work without this setting.
> Therefor we enable it automatically.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
I fixed two minor nits below and then applied the patch to main.
Regards,
Dumitru
> NEWS | 2 ++
> northd/northd.c | 6 ++----
> ovn-nb.xml | 34 --------------------------------
> tests/multinode.at | 7 -------
> tests/ovn-northd.at | 48 ++-------------------------------------------
> 5 files changed, 6 insertions(+), 91 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c9eb8ede8..264549817 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,8 @@ Post v24.09.0
> - ovn-ic: Add support for route tag to prevent route learning.
> - Support for STT tunnels in ovn-encap-type is deprecated and will be
> removed in the next release.
> + - The LRP option 'centralize_routing' has been removed. The behavior is
> now
> + enabled in all cases where it is needed.
>
> OVN v24.09.0 - 13 Sep 2024
> --------------------------
> diff --git a/northd/northd.c b/northd/northd.c
> index 75dedd83f..3cc8afc05 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2332,7 +2332,6 @@ create_cr_port(struct ovn_port *op, struct hmap *ports,
> * Chassis resident port needs to be created if the following
> * conditionsd are met:
> * - op is a distributed gateway port
> - * - op has the option 'centralize_routing' set to true
> * - op is the only distributed gateway port attached to its
> * router
> * - op's peer logical switch has no localnet ports.
> @@ -2343,7 +2342,7 @@ peer_needs_cr_port_creation(struct ovn_port *op)
> if ((op->nbrp->n_gateway_chassis || op->nbrp->ha_chassis_group)
> && op->od->n_l3dgw_ports == 1 && op->peer && op->peer->nbsp
> && !op->peer->od->n_localnet_ports) {
> - return smap_get_bool(&op->nbrp->options, "centralize_routing",
> false);
> + return true;
> }
>
> return false;
> @@ -2503,8 +2502,7 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> * peer if
> * - DGP's router has only one DGP and
> * - Its peer is a logical switch port and
> - * - It's peer's logical switch has no localnet ports and
> - * - option 'centralize_routing' is set to true for the DGP.
> + * - It's peer's logical switch has no localnet ports
Nit: s/It's/Its/
> *
> * This is required to support
> * - NAT via geneve (for the overlay provider networks) and
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 5114bbc2e..a78e2410f 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -3645,40 +3645,6 @@ or
> </p>
> </column>
>
> - <column name="options" key="centralize_routing"
> - type='{"type": "boolean"}'>
> - <p>
> - This option is applicable only if the router port is a
> - distributed gateway port i.e if the <ref
> table="Logical_Router_Port"
> - column="ha_chassis_group"/> column or
> - <ref table="Logical_Router_Port" column="gateway_chassis"/>
> - is set.
> - </p>
> -
> - <p>
> - If set to <code>true</code>, routing for the router port's
> - networks (set in the column <ref table="Logical_Router_Port"
> - column="networks"/>) is centralized on the gateway chassis
> - which claims this distributed gateway port.
> - </p>
> -
> - <p>
> - Additionally for this option to take effect, below conditions
> - must be met:
> - </p>
> -
> - <ul>
> - <li>
> - The Logical router has only one distributed gateway port.
> - </li>
> -
> - <li>
> - The router port's peer logical switch has no localnet ports.
> - </li>
> -
> - </ul>
> - </column>
> -
> <column name="options" key="ic-route-tag" type='{"type": "string"}'>
> <p>
> This option expects a name of a route-tag that's present in the
> diff --git a/tests/multinode.at b/tests/multinode.at
> index a45dc55cc..9602358aa 100644
> --- a/tests/multinode.at
> +++ b/tests/multinode.at
> @@ -1197,13 +1197,6 @@ run_ns_traffic
> # Delete the localnet port by changing the type of ln-public to VIF port.
> check multinode_nbctl --wait=hv lsp-set-type ln-public ""
>
> -# cr-port should not be created for public-lr0 since the option
> -# centralize_routing=true is not yet set for lr0-public.
> -m_check_row_count Port_Binding 0 logical_port=cr-public-lr0
> -
> -# Set the option - centralize_routing now.
> -check multinode_nbctl --wait=hv set logical_router_port lr0-public
> options:centralize_routing=true
> -
> m_check_row_count Port_Binding 1 logical_port=cr-public-lr0
> m_check_column chassisredirect Port_Binding type logical_port=cr-public-lr0
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 45816836e..9df65fdcf 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -13736,52 +13736,8 @@ check_flows_no_cr_port_for_public_lr0
> # Remove the localnet port from public logical switch.
> check ovn-nbctl --wait=sb lsp-set-type ln-public ""
>
> -# Check that the lflows are as expected and there is no cr port
> -# created for "public-lr0" when public has no localnet port
> -# since public doesn't have the option "overlay_provider_network=true"
> -# set.
> -check_row_count Port_Binding 0 logical_port=cr-public-lr0
> -
> -ovn-sbctl dump-flows lr0 > lr0flows
> -ovn-sbctl dump-flows public > publicflows
> -
> -AT_CHECK([grep -e "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e
> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" lr0flows |
> ovn_strip_lflows], [0], [dnl
> - table=??(lr_in_admission ), priority=50 , match=(eth.dst ==
> 30:54:00:00:00:03 && inport == "lr0-public" &&
> is_chassis_resident("sw0-port1")), action=(xreg0[[0..47]] =
> 00:00:00:00:ff:02; next;)
> - table=??(lr_in_arp_resolve ), priority=100 , match=(outport ==
> "lr0-public" && reg0 == 172.168.0.110), action=(eth.dst = 30:54:00:00:00:03;
> next;)
> - table=??(lr_in_arp_resolve ), priority=100 , match=(outport ==
> "lr0-public" && reg0 == 172.168.0.120), action=(eth.dst = 00:00:00:00:ff:02;
> next;)
> - table=??(lr_in_arp_resolve ), priority=150 , match=(inport ==
> "lr0-public" && outport == "lr0-public" && ip4.dst == 172.168.0.110),
> action=(drop;)
> - table=??(lr_in_arp_resolve ), priority=150 , match=(inport ==
> "lr0-public" && outport == "lr0-public" && ip4.dst == 172.168.0.120),
> action=(drop;)
> - table=??(lr_in_dnat ), priority=100 , match=(ip && ip4.dst ==
> 172.168.0.110 && inport == "lr0-public"), action=(ct_dnat(10.0.0.3);)
> - table=??(lr_in_dnat ), priority=100 , match=(ip && ip4.dst ==
> 172.168.0.120 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_dnat(20.0.0.3);)
> - table=??(lr_in_gw_redirect ), priority=100 , match=(ip4.src == 10.0.0.3
> && outport == "lr0-public" && is_chassis_resident("sw0-port1")),
> action=(eth.src = 30:54:00:00:00:03; reg1 = 172.168.0.110; next;)
> - table=??(lr_in_ip_input ), priority=90 , match=(arp.op == 1 &&
> arp.tpa == 172.168.0.110), action=(eth.dst = eth.src; eth.src =
> xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
> xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1;
> output;)
> - table=??(lr_in_ip_input ), priority=90 , match=(arp.op == 1 &&
> arp.tpa == 172.168.0.120), action=(eth.dst = eth.src; eth.src =
> xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
> xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1;
> output;)
> - table=??(lr_in_ip_input ), priority=91 , match=(inport ==
> "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.110), action=(drop;)
> - table=??(lr_in_ip_input ), priority=91 , match=(inport ==
> "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.120), action=(drop;)
> - table=??(lr_in_ip_input ), priority=92 , match=(inport ==
> "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.110 &&
> is_chassis_resident("sw0-port1")), action=(eth.dst = eth.src; eth.src =
> 30:54:00:00:00:03; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
> 30:54:00:00:00:03; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1;
> output;)
> - table=??(lr_in_ip_input ), priority=92 , match=(inport ==
> "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.120 &&
> is_chassis_resident("cr-lr0-public")), action=(eth.dst = eth.src; eth.src =
> xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
> xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1;
> output;)
> - table=??(lr_in_unsnat ), priority=100 , match=(ip && ip4.dst ==
> 172.168.0.110 && inport == "lr0-public"), action=(ct_snat;)
> - table=??(lr_in_unsnat ), priority=100 , match=(ip && ip4.dst ==
> 172.168.0.120 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> - table=??(lr_out_egr_loop ), priority=100 , match=(ip4.dst ==
> 172.168.0.110 && outport == "lr0-public" &&
> is_chassis_resident("sw0-port1")), action=(clone { ct_clear; inport =
> outport; outport = ""; eth.dst <-> eth.src; flags = 0; flags.loopback = 1;
> reg0 = 0; reg1 = 0; reg2 = 0; reg3 = 0; reg4 = 0; reg5 = 0; reg6 = 0; reg7 =
> 0; reg8 = 0; reg9 = 0; reg9[[0]] = 1; next(pipeline=ingress, table=??); };)
> - table=??(lr_out_egr_loop ), priority=100 , match=(ip4.dst ==
> 172.168.0.120 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(clone { ct_clear; inport =
> outport; outport = ""; eth.dst <-> eth.src; flags = 0; flags.loopback = 1;
> reg0 = 0; reg1 = 0; reg2 = 0; reg3 = 0; reg4 = 0; reg5 = 0; reg6 = 0; reg7 =
> 0; reg8 = 0; reg9 = 0; reg9[[0]] = 1; next(pipeline=ingress, table=??); };)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("sw0-port1") &&
> (!ct.trk || !ct.rpl)), action=(eth.src = 30:54:00:00:00:03;
> ct_snat(172.168.0.110);)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 20.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.120);)
> - table=??(lr_out_undnat ), priority=100 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public"), action=(eth.src = 30:54:00:00:00:03;
> ct_dnat;)
> - table=??(lr_out_undnat ), priority=100 , match=(ip && ip4.src ==
> 20.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")),
> action=(ct_dnat;)
> -])
> -
> -AT_CHECK([grep -e "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e
> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" publicflows |
> ovn_strip_lflows], [0], [dnl
> - table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst ==
> 30:54:00:00:00:03 && is_chassis_resident("sw0-port1")), action=(outport =
> "public-lr0"; output;)
> - table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
> {00:00:00:00:ff:02, 30:54:00:00:00:03} && (arp.op == 1 || rarp.op == 3 ||
> nd_ns)), action=(outport = "_MC_flood_l2"; output;)
> - table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
> arp.op == 1 && arp.tpa == 172.168.0.110), action=(clone {outport =
> "public-lr0"; output; }; outport = "_MC_flood_l2"; output;)
> - table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
> arp.op == 1 && arp.tpa == 172.168.0.120), action=(clone {outport =
> "public-lr0"; output; }; outport = "_MC_flood_l2"; output;)
> -])
> -
> -
> -# Set the option "centralize_routing=true" for lr0-public.
> -check ovn-nbctl --wait=sb set logical_router_port lr0-public
> options:centralize_routing=true
> -
> -# Check that the lflows are as expected and there is cr port created for
> public-lr0.
> +# we know we still need the cr port so we check that the lflows are as
Nit: comments are sentences and shoult start with a capital letter.
> +# expected and there is cr port created for public-lr0.
> check_flows_cr_port_for_public_lr0
>
> # Set the type of ln-public back to localnet
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
