On 12/18/24 4:54 PM, Felix Huettner via dev wrote:
> add support for routing-protocol-redirects in combination with
> active-active routing.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
Hi Felix,
This is a rather superficial review. That's because I have some
architectural concerns in patch 2/4 that might affect the whole series.
In any case, please see below.
Regards,
Dumitru
> northd/northd.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--
> ovn-nb.xml | 4 ++++
> tests/ovn.at | 40 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 8cd158617..9538ec2d6 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2349,6 +2349,7 @@ join_logical_ports_lrp(struct hmap *ports,
>
> struct active_active_port {
> const struct nbrec_logical_switch_port *nbsp;
> + const struct nbrec_logical_switch_port *routing_protocol_redirect;
> const struct nbrec_logical_router_port *nbrp;
> struct ovn_datapath *switch_dp;
> struct ovn_datapath *router_dp;
> @@ -2416,6 +2417,7 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> ovs_list_init(both);
>
> struct shash active_active_ports =
> SHASH_INITIALIZER(&active_active_ports);
> + struct shash aa_redirect_targets =
> SHASH_INITIALIZER(&aa_redirect_targets);
Nit: let's be consistent in naming. Maybe "active_active_redirects"?
>
> const struct sbrec_port_binding *sb;
> SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sbrec_pb_table) {
> @@ -2444,6 +2446,12 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> aap->nbrp = nbrp;
> aap->router_dp = od;
> shash_add(&active_active_ports, nbrp->name, aap);
> +
> + const char *redirect_port_name = smap_get(&nbrp->options,
> + "routing-protocol-redirect");
Nit: indentation.
> + if (redirect_port_name) {
> + shash_add(&aa_redirect_targets, redirect_port_name, aap);
> + }
> continue;
> }
>
> @@ -2474,6 +2482,12 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> aap->switch_dp = od;
> continue;
> }
> + struct active_active_port *aap =
> + shash_find_data(&aa_redirect_targets, nbsp->name);
> + if (aap) {
> + aap->routing_protocol_redirect = nbsp;
> + continue;
> + }
> join_logical_ports_lsp(ports, nb_only, both, od, nbsp,
> nbsp->name, queue_id_bitmap,
> tag_alloc_table);
> @@ -2557,6 +2571,8 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> SHASH_FOR_EACH (aa_snode, &active_active_ports) {
> const struct active_active_port *aap = aa_snode->data;
> const struct nbrec_logical_switch_port *nbsp = aap->nbsp;
> + const struct nbrec_logical_switch_port *nbsp_rpr =
> + aap->routing_protocol_redirect;
> const struct nbrec_logical_router_port *nbrp = aap->nbrp;
> ovs_assert(nbrp);
> ovs_assert(aap->router_dp);
> @@ -2649,6 +2665,18 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> lsp->aa_chassis_name = xstrdup(chassis->name);
> lrp->aa_chassis_index = j;
> lsp->aa_chassis_index = j;
> +
> + if (nbsp_rpr) {
> + char *lsp_rpr_name = xasprintf("%s-%s-%"PRIuSIZE,
> + nbsp_rpr->name,
> + chassis->name, j);
> + join_logical_ports_lsp(ports, nb_only, both,
> + aap->switch_dp, nbsp_rpr,
> + lsp_rpr_name, queue_id_bitmap,
> + tag_alloc_table);
> + free(lsp_rpr_name);
> + }
> +
> }
> free(networks.network_name);
> free(networks.addresses);
> @@ -2712,6 +2740,7 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> ipam_add_port_addresses(op->od, op);
> }
>
> + shash_destroy(&aa_redirect_targets);
> shash_destroy_free_data(&active_active_ports);
> }
>
> @@ -14546,6 +14575,20 @@ build_arp_resolve_flows_for_lrp(struct ovn_port *op,
> }
> }
>
> +static char*
> +ovn_port_get_redirect_port_name(struct ovn_port *op) {
> + const char *rpr = smap_get(&op->nbrp->options,
> + "routing-protocol-redirect");
> + if (!rpr) {
> + return NULL;
> + }
> + if (op->is_active_active) {
> + return xasprintf("%s-%s-%"PRIuSIZE, rpr,
> + op->aa_chassis_name, op->aa_chassis_index);
> + }
> + return xstrdup(rpr);
> +}
> +
> static void
> build_routing_protocols_redirect_rule__(
> const char *s_addr, const char *redirect_port_name, int
> protocol_port,
> @@ -14679,8 +14722,7 @@ build_lrouter_routing_protocol_redirect(
> /* Proceed only for LRPs that have 'routing-protocol-redirect' option
> set.
> * Value of this option is the name of LSP to which the routing protocol
> * traffic will be redirected. */
> - const char *redirect_port_name = smap_get(&op->nbrp->options,
> - "routing-protocol-redirect");
> + char *redirect_port_name = ovn_port_get_redirect_port_name(op);
> if (!redirect_port_name) {
> return;
> }
> @@ -14696,6 +14738,7 @@ build_lrouter_routing_protocol_redirect(
> "Switch Port. Routing protocol redirecting won't
> be "
> "configured.",
> op->key);
> + free(redirect_port_name);
This might be a good candidate for a "cleanup goto".
> return;
> }
> if (lsp_in_peer->od != op->peer->od) {
> @@ -14706,6 +14749,7 @@ build_lrouter_routing_protocol_redirect(
> "option. Routing protocol redirecting won't be "
> "configured.",
> op->key, redirect_port_name);
> + free(redirect_port_name);
> return;
> }
>
> @@ -14717,6 +14761,7 @@ build_lrouter_routing_protocol_redirect(
> "were set via 'routing-protocols' options. This "
> "configuration has no effect.",
> op->key);
> + free(redirect_port_name);
> return;
> }
>
> @@ -14764,6 +14809,7 @@ build_lrouter_routing_protocol_redirect(
> ds_cstr(match),
> REGBIT_PORT_SEC_DROP " = 1; next;",
> lflow_ref);
> + free(redirect_port_name);
> }
>
> /* This function adds ARP resolve flows related to a LSP. */
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 9896d9310..9d3e65864 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -3689,6 +3689,10 @@ or
>
> The MAC and IP configuration of this LRP in the northbound database
> is ignored.
> +
> + In case the option <ref column="options"
> + key="routing-protocol-redirect"/> is set the target LSP will also
> be
> + duplicated per LRP/LSP combination.
> </p>
> <p>
> The LRP must be connected via its LSP to an external network.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index fbf83d2fc..d9a8c320d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -39970,6 +39970,27 @@ check ovn-nbctl lsp-add phys phys1 \
> -- lsp-set-type phys1 localnet \
> -- lsp-set-options phys1 network_name=phys
>
> +# routing-protocol redirection
> +check ovn-nbctl set logical_router_port internet-phys \
> + options:routing-protocol-redirect=bgp-redirect \
> + options:routing-protocols=BGP,BFD
> +check ovn-nbctl lsp-add phys bgp-redirect
> +
> +as hv1 ovs-vsctl add-port br-int bgp-redirect-0 -- \
> + set Interface bgp-redirect-0
> external-ids:iface-id=bgp-redirect-hv1-0 \
> + options:tx_pcap=hv1/bgp-redirect-0-tx.pcap \
> + options:rxq_pcap=hv1/bgp-redirect-0-rx.pcap
> +as hv1 ovs-vsctl add-port br-int bgp-redirect-1 -- \
> + set Interface bgp-redirect-1
> external-ids:iface-id=bgp-redirect-hv1-1 \
> + options:tx_pcap=hv1/bgp-redirect-1-tx.pcap \
> + options:rxq_pcap=hv1/bgp-redirect-1-rx.pcap
> +as hv2 ovs-vsctl add-port br-int bgp-redirect-0 -- \
> + set Interface bgp-redirect-0
> external-ids:iface-id=bgp-redirect-hv2-0 \
> + options:tx_pcap=hv2/bgp-redirect-0-tx.pcap \
> + options:rxq_pcap=hv2/bgp-redirect-0-rx.pcap
> +
> +# now all setup is done
> +
> check ovn-nbctl --wait=hv sync
> wait_for_ports_up
> OVN_POPULATE_ARP
> @@ -40053,6 +40074,25 @@ packet=$(fmt_pkt "Ether(dst='00:cc:cc:cc:cc:20',
> src='00:02:01:00:00:01')/ \
> cut -c 1-68)
> OVN_CHECK_PACKETS_CONTAIN_PARTS([hv2/br-phys_hv2-1-tx.pcap], ["$packet"])
>
> +# bgp packet to the active-active router ports are forwarded to the
> respective
> +# bgp redirect LSP
> +outside_to_bgp() {
> + local hv=$1 if=$2 redirindex=$3 mac=$4 ip=$5
> + packet=$(fmt_pkt "Ether(dst='$mac', src='00:aa:bb:cc:dd:ee')/ \
> + IP(dst='$ip', src='1.1.1.1')/ \
> + TCP(dport=179)")
> + as hv$hv ovs-appctl netdev-dummy/receive $if $packet
> + local filename="hv$hv-bgp-redirect-$redirindex-tx.expected"
> + rm -f $filename
> + echo $packet > $filename
> +
> OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv$hv/bgp-redirect-$redirindex-tx.pcap],
> + [$filename])
> +}
> +
> +outside_to_bgp 1 br-phys_hv1-1 0 00:01:01:00:00:01 192.168.10.10
> +outside_to_bgp 1 br-phys2_hv1-2 1 00:01:02:00:00:01 192.168.11.10
> +outside_to_bgp 2 br-phys_hv2-1 0 00:02:01:00:00:01 192.168.20.10
> +
> OVN_CLEANUP([hv1
> /Couldn't parse MAC binding/d
> /left allocated when ofproto/d],[hv2
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
