build_route_policies calls find_policy_outport() on all reroute policies in order to ensure that the policy can actually be applied on the given logical router.
Then en_lflow calls the function again when writing the logical flows. The values cannot have changed, so it makes sense to cache the learned values in en-route-policies and then use those cached values in en-lflow. Rather than caching the ovn_port structure directly, we just save the name of the outport port so that in a later commit, we will be able to incrementally process route policy changes even if en-northd recomputes. This also was a good excuse to convert the valid_nexthops array over to a vector. As a side effect, this commit moves find_policy_outport() into en-route-policies.c since this is the only file where the function is used now. Signed-off-by: Mark Michelson <[email protected]> --- northd/en-route-policies.c | 93 +++++++++++++++++++++++++++------ northd/en-route-policies.h | 16 +++++- northd/northd.c | 102 ++++++++++--------------------------- northd/northd.h | 6 --- 4 files changed, 120 insertions(+), 97 deletions(-) diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index 8b53714db..0cdfb511d 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -66,6 +66,61 @@ policy_chain_add(struct simap *chain_ids, const char *chain_name) } } +/* Returns true if the output port to be used for forwarding traffic through + * this policy could be determined. Stores a pointer to the output port + * in 'p_output_port' and a pointer to the router IP address to be used for + * this policy, in 'p_lrp_addr_s'. */ +static bool +find_policy_outport(struct ovn_datapath *od, + const struct nbrec_logical_router_policy *policy, + const char *nexthop, bool is_ipv4, + const char **p_lrp_addr_s, struct ovn_port **p_out_port) +{ + if (nexthop == NULL) { + return false; + } + + struct ovn_port *out_port = NULL; + const char *lrp_addr_s = NULL; + + if (policy->output_port) { + if (!find_route_outport(od, policy->output_port->name, + "policy", policy->match, + nexthop, is_ipv4, true, &out_port, + &lrp_addr_s)) { + return false; + } + } else { + /* If output_port is not specified, find the router port matching + * the next hop. */ + HMAP_FOR_EACH (out_port, dp_node, &od->ports) { + lrp_addr_s = lrp_find_member_ip(out_port, nexthop); + if (lrp_addr_s) { + break; + } + } + } + + if (!out_port || !lrp_addr_s) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "Logical Router: %s, policy " + "(chain: '%s', match: '%s', priority %"PRId64"): " + "no path for next hop %s", + od->nbr->name, + policy->chain ? policy->chain : "<Default>", + policy->match, policy->priority, nexthop); + return false; + } + if (p_out_port) { + *p_out_port = out_port; + } + if (p_lrp_addr_s) { + *p_lrp_addr_s = lrp_addr_s; + } + + return true; +} + static bool check_bfd_state(const struct nbrec_logical_router_policy *rule, struct ovn_port *out_port, const char *nexthop, @@ -147,8 +202,6 @@ build_route_policies(struct ovn_datapath *od, continue; } - size_t n_valid_nexthops = 0; - char **valid_nexthops = NULL; uint32_t chain_id = 0; uint32_t jump_chain_id = 0; @@ -186,6 +239,8 @@ build_route_policies(struct ovn_datapath *od, chain_id = -1; } + struct vector valid_nexthops = + VECTOR_EMPTY_INITIALIZER(struct route_policy_nexthop); if (!strcmp(rule->action, "reroute")) { if (rule->nexthop && rule->nexthop[0]) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); @@ -234,7 +289,7 @@ build_route_policies(struct ovn_datapath *od, continue; } - valid_nexthops = xcalloc(rule->n_nexthops, sizeof *valid_nexthops); + vector_reserve(&valid_nexthops, rule->n_nexthops); for (size_t j = 0; j < rule->n_nexthops; j++) { char *nexthop = rule->nexthops[j]; if (!nexthop || !nexthop[0]) { @@ -242,9 +297,10 @@ build_route_policies(struct ovn_datapath *od, } struct ovn_port *out_port = NULL; + const char *lrp_addr_s = NULL; - if (!find_policy_outport(od, rule, nexthop, is_ipv4, NULL, - &out_port)) { + if (!find_policy_outport(od, rule, nexthop, is_ipv4, + &lrp_addr_s, &out_port)) { continue; } if (!check_bfd_state(rule, out_port, nexthop, @@ -252,21 +308,28 @@ build_route_policies(struct ovn_datapath *od, bfd_active_connections)) { continue; } - valid_nexthops[n_valid_nexthops++] = nexthop; + struct route_policy_nexthop policy_nexthop = { + .nexthop_addr = nexthop, + .outport_key = out_port->nbrp->name, + }; + strncpy(policy_nexthop.src_addr, lrp_addr_s, + sizeof(policy_nexthop.src_addr)); + vector_push(&valid_nexthops, &policy_nexthop); } - if (!n_valid_nexthops) { - free(valid_nexthops); + if (vector_len(&valid_nexthops) == 0) { + vector_destroy(&valid_nexthops); continue; } } - struct route_policy *new_rp = xzalloc(sizeof *new_rp); - new_rp->rule = rule; - new_rp->n_valid_nexthops = n_valid_nexthops; - new_rp->valid_nexthops = valid_nexthops; - new_rp->chain_id = chain_id; - new_rp->jump_chain_id = jump_chain_id; + struct route_policy *new_rp = xmalloc(sizeof *new_rp); + *new_rp = (struct route_policy) { + .rule = rule, + .valid_nexthops = vector_steal(&valid_nexthops), + .chain_id = chain_id, + .jump_chain_id = jump_chain_id, + }; hmap_insert(route_policies, &new_rp->key_node, hash); } } @@ -283,7 +346,7 @@ route_policies_destroy(struct route_policies_data *data) { struct route_policy *rp; HMAP_FOR_EACH_POP (rp, key_node, &data->route_policies) { - free(rp->valid_nexthops); + vector_destroy(&rp->valid_nexthops); free(rp); }; hmap_destroy(&data->route_policies); diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h index 1b13eb4cf..490ba09e7 100644 --- a/northd/en-route-policies.h +++ b/northd/en-route-policies.h @@ -17,9 +17,22 @@ #ifndef EN_ROUTE_POLICIES_H #define EN_ROUTE_POLICIES_H +#include <arpa/inet.h> + #include "inc-proc-eng.h" #include "openvswitch/hmap.h" +#include "vec.h" + +/* Each instance of this represents a nexthop for a router + * policy with "reroute" action. The fields are used for + * building the associated logical flows later. + */ +struct route_policy_nexthop { + const char *nexthop_addr; + char src_addr[INET6_ADDRSTRLEN]; + const char *outport_key; +}; /* Represents the data associated with an instance of a northbound * Logical Router Policy for a particular Logical Router. @@ -27,8 +40,7 @@ struct route_policy { struct hmap_node key_node; const struct nbrec_logical_router_policy *rule; - size_t n_valid_nexthops; - char **valid_nexthops; + struct vector valid_nexthops; /* struct route_policy_nexthop */ uint32_t chain_id; uint32_t jump_chain_id; }; diff --git a/northd/northd.c b/northd/northd.c index c0de0e444..d3f80f898 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -12190,61 +12190,6 @@ lrp_find_member_ip(const struct ovn_port *op, const char *ip_s) return find_lport_address(&op->lrp_networks, ip_s); } -/* Returns true if the output port to be used for forwarding traffic through - * this policy could be determined. Stores a pointer to the output port - * in 'p_output_port' and a pointer to the router IP address to be used for - * this policy, in 'p_lrp_addr_s'. */ -bool -find_policy_outport(struct ovn_datapath *od, - const struct nbrec_logical_router_policy *policy, - const char *nexthop, bool is_ipv4, - const char **p_lrp_addr_s, struct ovn_port **p_out_port) -{ - if (nexthop == NULL) { - return false; - } - - struct ovn_port *out_port = NULL; - const char *lrp_addr_s = NULL; - - if (policy->output_port) { - if (!find_route_outport(od, policy->output_port->name, - "policy", policy->match, - nexthop, is_ipv4, true, &out_port, - &lrp_addr_s)) { - return false; - } - } else { - /* If output_port is not specified, find the router port matching - * the next hop. */ - HMAP_FOR_EACH (out_port, dp_node, &od->ports) { - lrp_addr_s = lrp_find_member_ip(out_port, nexthop); - if (lrp_addr_s) { - break; - } - } - } - - if (!out_port || !lrp_addr_s) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL(&rl, "Logical Router: %s, policy " - "(chain: '%s', match: '%s', priority %"PRId64"): " - "no path for next hop %s", - od->nbr->name, - policy->chain ? policy->chain : "<Default>", - policy->match, policy->priority, nexthop); - return false; - } - if (p_out_port) { - *p_out_port = out_port; - } - if (p_lrp_addr_s) { - *p_lrp_addr_s = lrp_addr_s; - } - - return true; -} - static void build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od, struct route_policy *rp, @@ -12256,19 +12201,21 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od, struct ds actions = DS_EMPTY_INITIALIZER; if (!strcmp(rule->action, "reroute")) { - ovs_assert(rp->n_valid_nexthops <= 1); + ovs_assert(vector_len(&rp->valid_nexthops) <= 1); - if (!rp->n_valid_nexthops) { + if (vector_len(&rp->valid_nexthops) == 0) { return; } - char *nexthop = rp->valid_nexthops[0]; + struct route_policy_nexthop *policy_nexthop = + vector_get_ptr(&rp->valid_nexthops, 0); + const char *nexthop = policy_nexthop->nexthop_addr; bool is_ipv4 = strchr(nexthop, '.') ? true : false; - const char *lrp_addr_s = NULL; - struct ovn_port *out_port = NULL; + const char *lrp_addr_s = policy_nexthop->src_addr; + const struct ovn_port *out_port = + ovn_port_find_in_datapath_by_name(od, policy_nexthop->outport_key); - if (!find_policy_outport(od, rule, nexthop, is_ipv4, &lrp_addr_s, - &out_port)) { + if (!out_port) { return; } @@ -12329,21 +12276,24 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, struct lflow_ref *lflow_ref) { const struct nbrec_logical_router_policy *rule = rp->rule; - ovs_assert(rp->n_valid_nexthops > 1); + ovs_assert(vector_len(&rp->valid_nexthops) > 1); struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; - for (size_t i = 0; i < rp->n_valid_nexthops; i++) { - bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false; - const char *lrp_addr_s = NULL; - struct ovn_port *out_port = NULL; + struct route_policy_nexthop *policy_nexthop; + size_t i = 0; + VECTOR_FOR_EACH_PTR (&rp->valid_nexthops, policy_nexthop) { + const char *nexthop = policy_nexthop->nexthop_addr; + const char *lrp_addr_s = policy_nexthop->src_addr; + const struct ovn_port *out_port = + ovn_port_find_in_datapath_by_name(od, policy_nexthop->outport_key); - if (!find_policy_outport(od, rule, rp->valid_nexthops[i], is_ipv4, - &lrp_addr_s, &out_port)) { - goto cleanup; + if (!out_port) { + continue; } + bool is_ipv4 = strchr(nexthop, '.') ? true : false; ds_clear(&actions); uint32_t pkt_mark = smap_get_uint(&rule->options, "pkt_mark", 0); if (pkt_mark) { @@ -12359,7 +12309,7 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, REGBIT_NEXTHOP_IS_IPV4" = %d; " "next;", is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6, - rp->valid_nexthops[i], + nexthop, is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6, lrp_addr_s, out_port->lrp_networks.ea_s, @@ -12373,6 +12323,7 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_ECMP, 100, ds_cstr(&match), ds_cstr(&actions), lflow_ref, WITH_HINT(&rule->header_)); + i++; } ds_clear(&actions); @@ -12380,17 +12331,19 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id, REG_ECMP_MEMBER_ID); - for (size_t i = 0; i < rp->n_valid_nexthops; i++) { + i = 0; + VECTOR_FOR_EACH_PTR (&rp->valid_nexthops, policy_nexthop) { if (i > 0) { ds_put_cstr(&actions, ", "); } ds_put_format(&actions, "%"PRIuSIZE, i + 1); + i++; } ds_put_cstr(&actions, ");"); + ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, rule->priority, rule->match, ds_cstr(&actions), lflow_ref, WITH_HINT(&rule->header_)); -cleanup: ds_destroy(&match); ds_destroy(&actions); } @@ -15739,7 +15692,8 @@ build_ingress_policy_flows_for_lrouter( route_policies) { const struct nbrec_logical_router_policy *rule = rp->rule; bool is_ecmp_reroute = - (!strcmp(rule->action, "reroute") && rp->n_valid_nexthops > 1); + (!strcmp(rule->action, "reroute") && + vector_len(&rp->valid_nexthops) > 1); if (is_ecmp_reroute) { build_ecmp_routing_policy_flows(lflows, od, rp, ecmp_group_id, diff --git a/northd/northd.h b/northd/northd.h index 81ebbf0f6..30264e6be 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -1020,12 +1020,6 @@ bool northd_handle_lb_data_changes(struct tracked_lb_data *, const struct hmap *lr_lb_map, struct northd_tracked_data *); -bool find_policy_outport(struct ovn_datapath *od, - const struct nbrec_logical_router_policy *policy, - const char *nexthop, bool is_ipv4, - const char **p_lrp_addr_s, - struct ovn_port **p_out_port); - void bfd_table_sync(struct ovsdb_idl_txn *, const struct nbrec_bfd_table *, const struct hmap *, const struct hmap *, const struct hmap *, const struct hmap *, -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
