For routers with dynamic-routing enabled, if router ports are configured
with 'dynamic-routing-port-name=<LSP>' external routes should only be
learned if they're configured to use the Linux device that's configured
for the underlying OVS interface that binds <LSP>.
There were however a couple of races that made this feature unreliable:
1. the en_route I-P engine handler for port binding changes completely
ignored changes for port bindings corresponding to LSPs that are
configured as 'dynamic-routing-port-name=<LSP>'. If the LSP was
bound _after_ the router port configuration was parsed the en_route
data wouldn't be correctly recomputed and ovn-controller would ignore
routes that used the underlying OVS device until the next time a
recompute of the en_route node would happen (potentially indefinitely
long).
2. the function to extract the OVS interface name corresponding to a
port binding logical_port was using the local_binding_is_up() helper
to determine of the logical switch port is bound locally. While that
may seem correct (because it also takes into account whether OF rules
have been fully installed in OVS for that port - ovn-installed=true)
it introduces a race. If ovn-controller can't write the OVS
external_id (e.g., the last OVS transaction is still executing) the
function returns false.
Once the OVS transaction succeeds there's no easy way to inform the
en_route I-P engine node that it has to recompute.
However, from the en_route node perspective it's enough to check
that:
- a local binding exists for that LSP
- the local binding is claimed by the local chassis (it's "chassis
resident")
This commit takes this path as it has the lowest chance of
introducing bugs due to missed I-P dependencies.
A system test is also added to cover the scenarios above.
Fixes: d7d886eca553 ("controller: Support learning routes per iface.")
Reported-at: https://issues.redhat.com/browse/FDP-1842
Signed-off-by: Dumitru Ceara <[email protected]>
---
controller/binding.c | 44 +++++++++---------
controller/binding.h | 1 +
controller/ovn-controller.c | 25 +++++++++++
controller/route.c | 11 +++--
controller/route.h | 5 +++
tests/system-ovn.at | 90 +++++++++++++++++++++++++++++++++++++
6 files changed, 149 insertions(+), 27 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 74330b2560..b429323a9d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -808,8 +808,6 @@ static struct binding_lport *local_binding_add_lport(
struct local_binding *,
const struct sbrec_port_binding *,
enum en_lport_type);
-static struct binding_lport *local_binding_get_primary_lport(
- struct local_binding *);
static struct binding_lport *local_binding_get_first_lport(
struct local_binding *lbinding);
static struct binding_lport *local_binding_get_primary_or_localport_lport(
@@ -898,6 +896,27 @@ local_binding_get_primary_pb(const struct shash
*local_bindings,
return b_lport ? b_lport->pb : NULL;
}
+/* Returns the primary binding lport if present in lbinding's
+ * binding lports list. A binding lport is considered primary
+ * if binding lport's type is LP_VIF and the name matches
+ * with the 'lbinding'.
+ */
+struct binding_lport *
+local_binding_get_primary_lport(struct local_binding *lbinding)
+{
+ if (!lbinding) {
+ return NULL;
+ }
+
+ struct binding_lport *b_lport = local_binding_get_first_lport(lbinding);
+ if (b_lport && b_lport->type == LP_VIF &&
+ !strcmp(lbinding->name, b_lport->name)) {
+ return b_lport;
+ }
+
+ return NULL;
+}
+
ofp_port_t
local_binding_get_lport_ofport(const struct shash *local_bindings,
const char *pb_name)
@@ -3506,27 +3525,6 @@ local_binding_get_first_lport(struct local_binding
*lbinding)
return NULL;
}
-/* Returns the primary binding lport if present in lbinding's
- * binding lports list. A binding lport is considered primary
- * if binding lport's type is LP_VIF and the name matches
- * with the 'lbinding'.
- */
-static struct binding_lport *
-local_binding_get_primary_lport(struct local_binding *lbinding)
-{
- if (!lbinding) {
- return NULL;
- }
-
- struct binding_lport *b_lport = local_binding_get_first_lport(lbinding);
- if (b_lport && b_lport->type == LP_VIF &&
- !strcmp(lbinding->name, b_lport->name)) {
- return b_lport;
- }
-
- return NULL;
-}
-
static struct binding_lport *
local_binding_get_primary_or_localport_lport(struct local_binding *lbinding)
{
diff --git a/controller/binding.h b/controller/binding.h
index 8d978544f9..beca9cda97 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -163,6 +163,7 @@ void local_binding_data_destroy(struct local_binding_data
*);
const struct sbrec_port_binding *local_binding_get_primary_pb(
const struct shash *local_bindings, const char *pb_name);
+struct binding_lport *local_binding_get_primary_lport(struct local_binding *);
ofp_port_t local_binding_get_lport_ofport(const struct shash *local_bindings,
const char *pb_name);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index ea65d3a3e8..c2dab41c11 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5135,6 +5135,11 @@ struct ed_type_route {
* locally. */
struct sset tracked_ports_remote;
+ /* Contains all the currently configured dynamic-routing-port-name values
+ * on all datapaths.
+ */
+ struct sset filtered_ports;
+
/* Contains struct advertise_datapath_entry */
struct hmap announce_routes;
@@ -5186,6 +5191,7 @@ en_route_run(struct engine_node *node, void *data)
struct route_ctx_out r_ctx_out = {
.tracked_re_datapaths = &re_data->tracked_route_datapaths,
.tracked_ports_local = &re_data->tracked_ports_local,
+ .filtered_ports = &re_data->filtered_ports,
.tracked_ports_remote = &re_data->tracked_ports_remote,
.announce_routes = &re_data->announce_routes,
};
@@ -5194,6 +5200,7 @@ en_route_run(struct engine_node *node, void *data)
tracked_datapaths_clear(r_ctx_out.tracked_re_datapaths);
sset_clear(r_ctx_out.tracked_ports_local);
sset_clear(r_ctx_out.tracked_ports_remote);
+ sset_clear(r_ctx_out.filtered_ports);
route_run(&r_ctx_in, &r_ctx_out);
return EN_UPDATED;
@@ -5209,6 +5216,7 @@ en_route_init(struct engine_node *node OVS_UNUSED,
hmap_init(&data->tracked_route_datapaths);
sset_init(&data->tracked_ports_local);
sset_init(&data->tracked_ports_remote);
+ sset_init(&data->filtered_ports);
hmap_init(&data->announce_routes);
data->ovnsb_idl = arg->sb_idl;
@@ -5223,6 +5231,7 @@ en_route_cleanup(void *data)
tracked_datapaths_destroy(&re_data->tracked_route_datapaths);
sset_destroy(&re_data->tracked_ports_local);
sset_destroy(&re_data->tracked_ports_remote);
+ sset_destroy(&re_data->filtered_ports);
route_cleanup(&re_data->announce_routes);
hmap_destroy(&re_data->announce_routes);
}
@@ -5300,6 +5309,14 @@ route_runtime_data_handler(struct engine_node *node,
void *data)
return EN_UNHANDLED;
}
+ /* If this logical port name is used to filter on which router
+ * ports learning should happen then process the changes. */
+ if (sset_find(&re_data->filtered_ports, name)) {
+ /* XXX: Until we get I-P support for route exchange we need to
+ * request recompute. */
+ return EN_UNHANDLED;
+ }
+
const char *dp_name = smap_get(&lport->pb->options,
"distributed-port");
if (dp_name && sset_contains(tracked_ports, dp_name)) {
@@ -5365,6 +5382,14 @@ route_sb_port_binding_data_handler(struct engine_node
*node, void *data)
* request recompute. */
return EN_UNHANDLED;
}
+
+ /* If this logical port name is used to filter on which router
+ * ports learning should happen then process the changes. */
+ if (sset_find(&re_data->filtered_ports, sbrec_pb->logical_port)) {
+ /* XXX: Until we get I-P support for route exchange we need to
+ * request recompute. */
+ return EN_UNHANDLED;
+ }
}
return EN_HANDLED_UNCHANGED;
diff --git a/controller/route.c b/controller/route.c
index db4aa4122b..adffc335bc 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -125,13 +125,15 @@ ifname_from_port_name(const struct smap *port_mapping,
return iface;
}
- if (!local_binding_is_up(local_bindings, port_name, chassis)) {
+ const struct binding_lport *b_lport =
+ local_binding_get_primary_lport(local_binding_find(local_bindings,
+ port_name));
+
+ if (!b_lport || !lport_pb_is_chassis_resident(chassis, b_lport->pb)) {
return NULL;
}
- struct local_binding *binding = local_binding_find(local_bindings,
- port_name);
- return binding->iface->name;
+ return b_lport->lbinding->iface->name;
}
static void
@@ -238,6 +240,7 @@ route_run(struct route_ctx_in *r_ctx_in,
smap_add(&ad->bound_ports, local_peer->logical_port,
ifname);
}
+ sset_add(r_ctx_out->filtered_ports, port_name);
}
}
diff --git a/controller/route.h b/controller/route.h
index c2c92e70b6..438d66859a 100644
--- a/controller/route.h
+++ b/controller/route.h
@@ -49,6 +49,11 @@ struct route_ctx_out {
* locally. */
struct sset *tracked_ports_remote;
+ /* Contains all the currently configured dynamic-routing-port-name values
+ * on all datapaths.
+ */
+ struct sset *filtered_ports;
+
/* Contains struct advertise_datapath_entry */
struct hmap *announce_routes;
};
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 2b880ec378..78d12cbdc4 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -17396,6 +17396,96 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
patch-.*/d
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([dynamic-routing - route learning filter port name])
+
+VRF_RESERVE([1337])
+
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_BR([br-int])
+check ovs-vsctl
\
+ -- set Open_vSwitch . external-ids:system-id=hv1
\
+ -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+ -- set Open_vSwitch . external-ids:ovn-encap-type=geneve
\
+ -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1
\
+ -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+check ovn-nbctl \
+ -- lr-add lr \
+ -- set logical_router lr options:chassis=hv1 \
+ options:dynamic-routing=true \
+ options:requested-tnl-key=1337 \
+ -- lrp-add lr lrp1 00:00:00:00:00:01 1.1.1.1/24 \
+ -- lrp-add lr lrp2 00:00:00:00:00:02 2.2.2.1/24 \
+ -- lrp-set-options lrp2 dynamic-routing-port-name=vif2 \
+ dynamic-routing-maintain-vrf=true \
+ -- ls-add ls1 \
+ -- lsp-add-router-port ls1 ls1-lr lrp1 \
+ -- lsp-add ls1 vif1 \
+ -- ls-add ls2 \
+ -- lsp-add-router-port ls2 ls2-lr lrp2 \
+ -- lsp-add ls2 vif2
+
+check ovs-vsctl add-port br-int vif1 \
+ -- set interface vif1 type=internal external_ids:iface-id=vif1
+check ovn-nbctl --wait=hv sync
+wait_for_ports_up vif1
+
+lrp1=$(fetch_column port_binding _uuid logical_port="lrp1")
+lrp2=$(fetch_column port_binding _uuid logical_port="lrp2")
+
+AT_CHECK([ip vrf show ovnvrf1337], [0], [dnl
+ovnvrf1337 1337
+])
+
+AS_BOX([Unbound vif2: no routes learned])
+check ovs-vsctl add-port br-int vif2 \
+ -- set interface vif2 type=internal
+check ip link set vif2 up
+check ip route add 3.3.3.0/24 via 2.2.2.2 dev vif2 onlink vrf ovnvrf1337
+check ovn-nbctl --wait=hv sync
+check_row_count Learned_Route 0
+
+AS_BOX([Bound vif2: routes learned on lrp2])
+check ovs-vsctl set interface vif2 external-ids:iface-id=vif2
+wait_for_ports_up vif2
+check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \
+ nexthop=2.2.2.2 \
+ logical_port=$lrp2
+check_row_count Learned_Route 1
+
+AS_BOX([No dynamic-routing-port-name: routes learned on lrp1 and lrp2])
+check ovn-nbctl --wait=hv \
+ remove logical_router_port lrp2 options dynamic-routing-port-name
+check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \
+ nexthop=2.2.2.2 \
+ logical_port=$lrp1
+check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \
+ nexthop=2.2.2.2 \
+ logical_port=$lrp2
+check_row_count Learned_Route 2
+
+OVN_CLEANUP_CONTROLLER([hv1])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([ovn-northd])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([Mac binding aging - Probing])
AT_KEYWORDS([mac_binding_probing])
--
2.51.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
