On Wed, Feb 12, 2025 at 02:43:09AM +0100, Martin Kalcok wrote: > This is a continuation of previous commit. It's separate > for ease of review. It will be squshed together for the > final version.
Hi Martin, i took a look at the changes and i'll add my findings below. Note that i squashed part 3 and 4 together locally for reviewing. I'll try to put my comments to the appropriate patch, but i am already sorry for when i mess that up :) > > Signed-off-by: Martin Kalcok <[email protected]> > --- > northd/en-advertised-route-sync.c | 20 ++- > northd/en-learned-route-sync.c | 3 +- > northd/northd.c | 217 ++++++++++++++++++++++++++++-- > northd/northd.h | 11 +- > 4 files changed, 231 insertions(+), 20 deletions(-) > > diff --git a/northd/en-advertised-route-sync.c > b/northd/en-advertised-route-sync.c > index 9d4fb308d..e81c86afa 100644 > --- a/northd/en-advertised-route-sync.c > +++ b/northd/en-advertised-route-sync.c > @@ -25,7 +25,6 @@ > #include "openvswitch/hmap.h" > #include "ovn-util.h" > > - > static void > advertised_route_table_sync( > struct ovsdb_idl_txn *ovnsb_txn, > @@ -205,9 +204,13 @@ en_dynamic_routes_run(struct engine_node *node, void > *data) > if (!od->dynamic_routing) { > continue; > } > - build_lb_nat_parsed_routes(od, lr_stateful_rec->lrnat_rec, > - &dynamic_routes_data->parsed_routes); > + build_nat_parsed_routes(od, lr_stateful_rec->lrnat_rec, > + northd_data, > + &dynamic_routes_data->parsed_routes); > > + build_lb_parsed_routes(od, lr_stateful_rec->lb_ips, > + northd_data, > + &dynamic_routes_data->parsed_routes); > } > engine_set_node_state(node, EN_UPDATED); > } > @@ -442,10 +445,19 @@ advertised_route_table_sync_route_add( > if (route->source == ROUTE_SOURCE_NAT && (drr & DRRM_NAT) == 0) { > return; > } > + if (route->source == ROUTE_SOURCE_LB && (drr & DRRM_LB) == 0) { > + return; > + } > > + /* XXX: I'm not sure if normalize prefix is the best call here. It > doesn't > + * include "/plen" for host routes, so they get announced without it. */ This is unfortunate but i don't think it would cause issues. In route.c we parse this value using ip46_parse_cidr and this will deduce it is a host route if there is no "/.*". Also this would probably also happen for now if we would specify a host route in a Logical_Router_Static_Route. > char *ip_prefix = normalize_v46_prefix(&route->prefix, route->plen); > + const struct sbrec_port_binding *tracked_port = NULL; > + if (route->tracked_port) { > + tracked_port = route->tracked_port->sb; > + } > ar_add_entry(sync_routes, route->od->sb, route->out_port->sb, > - ip_prefix, NULL); > + ip_prefix, tracked_port); > } > > static void > diff --git a/northd/en-learned-route-sync.c b/northd/en-learned-route-sync.c > index 406f1551f..4e87b3265 100644 > --- a/northd/en-learned-route-sync.c > +++ b/northd/en-learned-route-sync.c > @@ -181,7 +181,8 @@ parse_route_from_sbrec_route(struct hmap > *parsed_routes_out, > > parsed_route_add(od, nexthop, &prefix, plen, false, lrp_addr_s, > out_port, 0, false, false, NULL, > - ROUTE_SOURCE_LEARNED, &route->header_, > parsed_routes_out); > + ROUTE_SOURCE_LEARNED, &route->header_, NULL, > + parsed_routes_out); > } > > static void > diff --git a/northd/northd.c b/northd/northd.c > index 4b5708059..c0953028a 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -10992,6 +10992,7 @@ parsed_route_init(const struct ovn_datapath *od, > bool ecmp_symmetric_reply, > const struct sset *ecmp_selection_fields, > enum route_source source, > + const struct ovn_port *tracked_port, > const struct ovsdb_idl_row *source_hint) > { > > @@ -11007,6 +11008,7 @@ parsed_route_init(const struct ovn_datapath *od, > new_pr->is_discard_route = is_discard_route; > new_pr->lrp_addr_s = nullable_xstrdup(lrp_addr_s); > new_pr->out_port = out_port; > + new_pr->tracked_port = tracked_port; > new_pr->source = source; > if (ecmp_selection_fields) { > sset_clone(&new_pr->ecmp_selection_fields, ecmp_selection_fields); > @@ -11030,7 +11032,7 @@ parsed_route_clone(const struct parsed_route *pr) > pr->od, nexthop, pr->prefix, pr->plen, pr->is_discard_route, > pr->lrp_addr_s, pr->out_port, pr->route_table_id, pr->is_src_route, > pr->ecmp_symmetric_reply, &pr->ecmp_selection_fields, pr->source, > - pr->source_hint); > + pr->tracked_port, pr->source_hint); > > new_pr->hash = pr->hash; > return new_pr; > @@ -11069,13 +11071,14 @@ parsed_route_add(const struct ovn_datapath *od, > const struct sset *ecmp_selection_fields, > enum route_source source, > const struct ovsdb_idl_row *source_hint, > + const struct ovn_port *tracked_port, > struct hmap *routes) > { > > struct parsed_route *new_pr = parsed_route_init( > od, nexthop, *prefix, plen, is_discard_route, lrp_addr_s, out_port, > route_table_id, is_src_route, ecmp_symmetric_reply, > - ecmp_selection_fields, source, source_hint); > + ecmp_selection_fields, source, tracked_port, source_hint); > > new_pr->hash = route_hash(new_pr); > > @@ -11212,7 +11215,7 @@ parsed_routes_add_static(const struct ovn_datapath > *od, > parsed_route_add(od, nexthop, &prefix, plen, is_discard_route, > lrp_addr_s, > out_port, route_table_id, is_src_route, > ecmp_symmetric_reply, &ecmp_selection_fields, source, > - &route->header_, routes); > + &route->header_, NULL, routes); > sset_destroy(&ecmp_selection_fields); > } > > @@ -11230,7 +11233,7 @@ parsed_routes_add_connected(const struct ovn_datapath > *od, > false, addr->addr_s, op, > 0, false, > false, NULL, ROUTE_SOURCE_CONNECTED, > - &op->nbrp->header_, routes); > + &op->nbrp->header_, NULL, routes); > } > > for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > @@ -11242,7 +11245,7 @@ parsed_routes_add_connected(const struct ovn_datapath > *od, > false, addr->addr_s, op, > 0, false, > false, NULL, ROUTE_SOURCE_CONNECTED, > - &op->nbrp->header_, routes); > + &op->nbrp->header_, NULL, routes); > } > } > > @@ -11280,10 +11283,153 @@ build_parsed_routes(const struct ovn_datapath *od, > const struct hmap *lr_ports, > } > } > > +static int > +lrouter_check_nat_entry(const struct ovn_datapath *od, > + const struct ovn_nat *nat_entry, > + const struct hmap *lr_ports, ovs_be32 *mask, > + bool *is_v6, int *cidr_bits, struct eth_addr *mac, > + bool *distributed, struct ovn_port **nat_l3dgw_port); > + > +static const struct ovn_port * > +get_nat_route_tracked_port(const struct ovn_port *op, > + const struct ovn_nat *nat, > + const struct northd_data *northd_data) > +{ > + /* Ports on GW routers don't need tracked_port because all resources > + * are located on the same chassis.*/ > + if (op->od->is_gw_router) { > + return NULL; > + } > + struct eth_addr mac = eth_addr_broadcast; > + bool is_v6, distributed_nat; > + ovs_be32 mask; > + int cidr_bits; > + struct ovn_port *l3dgw_port; > + > + if (lrouter_check_nat_entry(op->od, nat, &northd_data->lr_ports, &mask, > &is_v6, > + &cidr_bits, > + &mac, &distributed_nat, &l3dgw_port) < 0) { > + return NULL; > + } > + > + /* For distributed NAT rules, we find ovn_port with name that matches > + * logical_port value of the NAT entry, and use that as tracked_port. */ > + if (distributed_nat) { > + return ovn_port_find(&northd_data->ls_ports, nat->nb->logical_port); > + /* For centralized NAT rules, we use designated DGP as a tracked port. */ > + } else { > + return l3dgw_port; > + } > + > + return NULL; > +} > + > +static void > +lport_addrs_to_parsed_routes(const struct ovn_port *out_port, > + const struct ovn_port *tracked_port, > + const struct lport_addresses *laddrs, > + enum route_source route_type, > + struct hmap *routes) > +{ > + for (int i = 0; i < laddrs->n_ipv4_addrs; i++) { > + struct ipv4_netaddr *addr = &laddrs->ipv4_addrs[i]; > + struct in6_addr prefix; > + ip46_parse(addr->network_s, &prefix); > + parsed_route_add(out_port->od, NULL, &prefix, addr->plen, > + false, addr->addr_s, out_port, > + 0, false, > + false, NULL, route_type, > + &out_port->nbrp->header_, tracked_port, routes); > + } > + for (int i = 0; i < laddrs->n_ipv6_addrs; i++) { > + struct ipv6_netaddr *addr = &laddrs->ipv6_addrs[i]; > + parsed_route_add(out_port->od, NULL, &addr->addr, addr->plen, > + false, addr->addr_s, out_port, > + 0, false, > + false, NULL, route_type, > + &out_port->nbrp->header_, tracked_port, routes); > + } > +} > + > +/* XXX: This function is, in nature, very similar to how > + * publish_host_routes_lrp looks up neighboring host routes. I really wanted > + * to reuse it, but it's designed to work with already existing parsed_routes > + * when creating Advertised_Route records. And that doesn't work in following > + * scenario: > + * > + * R1 --- ls_int --- R2 > + * > + * If R1 and R2 don't have IPv4/6 configured on their LRPs plugged to > + * ls_int, and you set "connected-as-host", no parsed_routes will be > generated > + * (makes sense). But as a consequence, publish_host_routes_lrp is never > + * executed. there is a parsed_route generated for the link local address but we have been filtering it out before processing it further. However in the case of connected-as-host i see no reason to keep this limitation. I have built a small patch on top of current main to share this. Not sure if that is exactly what you are looking for. https://github.com/felixhuettner/ovn/commit/5326af4e0fb0b054a7f023ed8d15ae5b2969928a > + * We would very much like this scenario to work. i.e. no explicit IP > + * configuration on ls_int, but NATs/LBs on R2 are reachable from R1 over > + * R2s IPv6 LLA. This function aims to solve it by generating > + * advertised_routes for NATs that are on op's neighbors (either directly > + * connected LRs or LRs connected to same LS as op). I tried to keep the > + * overhead to minimum. > + */ > +static void > +build_connected_nat_routes(const struct ovn_port *op, struct hmap *routes) > +{ > + if (!op->peer) { > + return; > + } > + struct ovn_datapath *peer_od = op->peer->od; > + if (!peer_od->nbs && !peer_od->nbr) { > + return; > + } > + > + const struct ovn_port *peer_port = NULL; > + /* This is directly connected LR peer. */ > + if (peer_od->nbr) { > + peer_port = op->peer; > + size_t n_nats = 0; > + char **nats = NULL; > + nats = get_nat_addresses(peer_port, &n_nats, false, false, NULL, > true); > + for (size_t i = 0; i < n_nats; i++) { > + /* XXX: This block should be probably squshed to a function. It's > + * bit repetitive with the one bellow. */ > + struct lport_addresses laddrs; > + int ofs = 0; > + extract_addresses(nats[i], &laddrs, &ofs); > + lport_addrs_to_parsed_routes(op, peer_port, &laddrs, > ROUTE_SOURCE_NAT, routes); > + free(nats[i]); > + destroy_lport_addresses(&laddrs); > + } > + free(nats); > + return; > + } > + > + /* This peer is LSP, we need to check all connected router ports for > NAT.*/ > + for (size_t i = 0; i < peer_od->n_router_ports; i++) { > + peer_port = peer_od->router_ports[i]->peer; > + if (peer_port == op) { > + /* no need to check NAT addresses on ovn_port that initiated this > + * function.*/ > + continue; > + } > + size_t n_nats = 0; > + char **nats = NULL; > + nats = get_nat_addresses(peer_port, &n_nats, false, false, NULL, > true); > + for (size_t j = 0; j < n_nats; j++) { > + struct lport_addresses laddrs; > + int ofs = 0; > + extract_addresses(nats[j], &laddrs, &ofs); > + lport_addrs_to_parsed_routes(op, peer_port, &laddrs, > ROUTE_SOURCE_NAT, routes); > + free(nats[j]); > + destroy_lport_addresses(&laddrs); > + } > + free(nats); > + } > +} > + > void > -build_lb_nat_parsed_routes(const struct ovn_datapath *od, > - const struct lr_nat_record *lr_nat, > - struct hmap *routes) > +build_nat_parsed_routes(const struct ovn_datapath *od, > + const struct lr_nat_record *lr_nat, > + const struct northd_data *northd_data, > + struct hmap *routes) > { > const struct ovn_port *op; > HMAP_FOR_EACH (op, dp_node, &od->ports) { > @@ -11291,19 +11437,62 @@ build_lb_nat_parsed_routes(const struct > ovn_datapath *od, > if (!(drrm & DRRM_NAT)) { > continue; > } > - /* I'm thinking of extending parsed_route struct with "tracked_port". > - * Since we are already parsing/iterating NATs here, it feels more > - * efficinet to figure out the tracked_port here, rather than > - * re-parsing NATs down the line in the advertised_route_table_sync > - * function before calling "ar_add_entry".*/ > + > for (size_t i = 0; i < lr_nat->n_nat_entries; i++) { > const struct ovn_nat *nat = &lr_nat->nat_entries[i]; > int plen = nat_entry_is_v6(nat) ? 128 : 32; > struct in6_addr prefix; > ip46_parse(nat->nb->external_ip, &prefix); > + const struct ovn_port *tracked_port = > + get_nat_route_tracked_port(op, nat, northd_data); > parsed_route_add(od, NULL, &prefix, plen, false, > nat->nb->external_ip, op, 0, false, false, > - NULL, ROUTE_SOURCE_NAT, &op->nbrp->header_, > routes); > + NULL, ROUTE_SOURCE_NAT, &op->nbrp->header_, > + tracked_port, routes); > + } > + > + /* XXX: This could be made optional by adding "nat-connected" option > + * to dynamic-routing-redistribute. Similar to how connected and > + * connected-as-host work.*/ > + build_connected_nat_routes(op, routes); > + } > + > +} > + > +void > +build_lb_parsed_routes(const struct ovn_datapath *od, > + const struct ovn_lb_ip_set *lb_ips, > + const struct northd_data *northd_data, This seems to be unused. > + struct hmap *routes) > +{ > + const struct ovn_port *op; > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > + enum dynamic_routing_redistribute_mode drrm = > op->dynamic_routing_redistribute; > + if (!(drrm & DRRM_LB)) { > + continue; > + } > + > + const char *ip_address; > + SSET_FOR_EACH (ip_address, &lb_ips->ips_v4) { > + struct in6_addr prefix; > + ip46_parse(ip_address, &prefix); > + /* XXX: Need to figure out tracked_port for LB without re-parsing > + * ovn_datapath. lr_stateful_record doens't have as much data > + * about LBs as it does about NATs. */ > + const struct ovn_port *tracked_port = NULL; > + parsed_route_add(od, NULL, &prefix, 32, false, > + ip_address, op, 0, false, false, > + NULL, ROUTE_SOURCE_LB, &op->nbrp->header_, > + tracked_port, routes); > + } > + SSET_FOR_EACH (ip_address, &lb_ips->ips_v6) { > + struct in6_addr prefix; > + ip46_parse(ip_address, &prefix); > + const struct ovn_port *tracked_port = NULL; > + parsed_route_add(od, NULL, &prefix, 128, false, > + ip_address, op, 0, false, false, > + NULL, ROUTE_SOURCE_LB, &op->nbrp->header_, > + tracked_port, routes); > } > } > > diff --git a/northd/northd.h b/northd/northd.h > index 95f2fe010..5c0f7bc52 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -761,6 +761,7 @@ struct parsed_route { > const struct ovsdb_idl_row *source_hint; > char *lrp_addr_s; > const struct ovn_port *out_port; > + const struct ovn_port *tracked_port; /* May be NULL. */ I am not sure if adding this here makes sense. It seems to me that in the case that tracked_port is set then most other fields have empty/default values. Therefore maybe it makes sense to create a separate struct just with the needed fields (datapath, port, prefix and tracked_port). Thanks a lot, Felix > }; > > /* Returns an independent clone of the provided parsed_route. The returned > @@ -789,6 +790,7 @@ void parsed_route_add(const struct ovn_datapath *od, > const struct sset *ecmp_selection_fields, > enum route_source source, > const struct ovsdb_idl_row *source_hint, > + const struct ovn_port *tracked_port, > struct hmap *routes); > > bool > @@ -823,7 +825,14 @@ void route_policies_destroy(struct route_policies_data > *); > void build_parsed_routes(const struct ovn_datapath *, const struct hmap *, > const struct hmap *, struct hmap *, struct simap *, > struct hmap *); > -void build_lb_nat_parsed_routes(const struct ovn_datapath *, const struct > lr_nat_record *, struct hmap *); > +void build_nat_parsed_routes(const struct ovn_datapath *, > + const struct lr_nat_record *, > + const struct northd_data *, > + struct hmap *); > +void build_lb_parsed_routes(const struct ovn_datapath *, > + const struct ovn_lb_ip_set *, > + const struct northd_data *, > + struct hmap *); > uint32_t get_route_table_id(struct simap *, const char *); > void routes_init(struct routes_data *); > void routes_destroy(struct routes_data *); > -- > 2.43.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
