Commit da792b85c ("northd: Track router echo request/reply in the
router port ct zone.") assigned a CT zone to the switch-side patch
port of every router port on a local logical switch. To keep that
assignment consistent it made ct_zones_runtime_data_handler() fall
back to a full recompute whenever any non-ACL patch port was tracked,
which is far more than the actual dependency: the zone of such a port
is needed if and only if the port is still present in the peer set of
a local logical switch.
Track the peer set instead. The runtime_data engine node now
maintains a shash of tracked peer ports, keyed by logical port name,
which binding.c fills in whenever a peer relationship is added to or
removed from an already local datapath. The ct_zones handler
processes those ports through the existing idempotent
ct_zone_handle_port_update(), and for non-ACL patch ports found in
tracked_dp_bindings it reads the need directly from the current peer
set of the port's datapath -- the same condition ct_zones_update()
applies on a full recompute -- so zones of ports that lost their peer
relationship (e.g. a router port that stops being an l3gateway port)
are still released. Datapaths that become local keep the same
TRACKED_RESOURCE_NEW fallback, which is also required for the NAT
zones of newly local router datapaths.
Assisted-by: GLM 5.3, OpenCode
Signed-off-by: Jacob Tanenbaum <[email protected]>
diff --git a/controller/binding.c b/controller/binding.c
index 3e8114e7e..e0ebe90e9 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -743,7 +743,8 @@ remove_related_lport(const struct sbrec_port_binding *pb,
*/
static void
update_ld_peers(const struct sbrec_port_binding *pb,
- struct hmap *local_datapaths)
+ struct hmap *local_datapaths,
+ struct shash *tracked_peer_ports)
{
struct local_datapath *ld =
get_local_datapath(local_datapaths, pb->datapath->tunnel_key);
@@ -760,7 +761,8 @@ update_ld_peers(const struct sbrec_port_binding *pb,
enum en_lport_type type = get_lport_type(pb);
size_t num_peers = vector_len(&ld->peer_ports);
if (type != LP_PATCH) {
- remove_local_datapath_peer_port(pb, ld, local_datapaths);
+ remove_local_datapath_peer_port(pb, ld, local_datapaths,
+ tracked_peer_ports);
if (num_peers != vector_len(&ld->peer_ports)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_DBG_RL(&rl,
@@ -2039,7 +2041,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
b_ctx_in->sbrec_port_binding_by_datapath,
b_ctx_in->sbrec_port_binding_by_name,
ld, b_ctx_out->local_datapaths,
- b_ctx_out->tracked_dp_bindings);
+ b_ctx_out->tracked_dp_bindings,
+ b_ctx_out->tracked_peer_ports);
}
}
@@ -2073,7 +2076,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
if (ld) {
remove_local_datapath_peer_port(pb, ld,
- b_ctx_out->local_datapaths);
+ b_ctx_out->local_datapaths,
+ b_ctx_out->tracked_peer_ports);
}
}
@@ -2460,7 +2464,8 @@ remove_pb_from_local_datapath(const struct
sbrec_port_binding *pb,
remove_related_lport(pb, b_ctx_out);
if (!strcmp(pb->type, "patch") ||
!strcmp(pb->type, "l3gateway")) {
- remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths);
+ remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths,
+ b_ctx_out->tracked_peer_ports);
} else if (!strcmp(pb->type, "localnet")) {
if (ld->localnet_port && !strcmp(ld->localnet_port->logical_port,
pb->logical_port)) {
@@ -3122,7 +3127,8 @@ consider_patch_port_for_local_datapaths(const struct
sbrec_port_binding *pb,
b_ctx_in->sbrec_port_binding_by_datapath,
b_ctx_in->sbrec_port_binding_by_name,
ld, b_ctx_out->local_datapaths,
- b_ctx_out->tracked_dp_bindings);
+ b_ctx_out->tracked_dp_bindings,
+ b_ctx_out->tracked_peer_ports);
}
}
@@ -3443,7 +3449,8 @@ delete_done:
}
if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) {
- update_ld_peers(pb, b_ctx_out->local_datapaths);
+ update_ld_peers(pb, b_ctx_out->local_datapaths,
+ b_ctx_out->tracked_peer_ports);
}
if (!handle_updated_port(b_ctx_in, b_ctx_out, pb)) {
diff --git a/controller/binding.h b/controller/binding.h
index 817c4e296..618506286 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -111,6 +111,11 @@ struct binding_ctx_out {
* the changed datapaths and port bindings. */
struct hmap *tracked_dp_bindings;
+ /* shash of 'struct tracked_peer_port' (keyed by logical port name)
+ * which the callee fills in when the peer relationship of a
+ * switch-side patch port changes. */
+ struct shash *tracked_peer_ports;
+
struct if_status_mgr *if_mgr;
struct sset *postponed_ports;
diff --git a/controller/local_data.c b/controller/local_data.c
index ad4b92f1f..38aa982f9 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -237,9 +237,13 @@ add_local_datapath_peer_port(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
struct local_datapath *ld,
struct hmap *local_datapaths,
- struct hmap *tracked_datapaths)
+ struct hmap *tracked_datapaths,
+ struct shash *tracked_peer_ports)
{
local_datapath_peer_port_add(ld, pb, peer);
+ if (ld->is_switch) {
+ tracked_peer_port_add(pb, true, tracked_peer_ports);
+ }
struct local_datapath *peer_ld =
get_local_datapath(local_datapaths,
@@ -254,6 +258,9 @@ add_local_datapath_peer_port(
}
local_datapath_peer_port_add(peer_ld, peer, pb);
+ if (peer_ld->is_switch) {
+ tracked_peer_port_add(peer, true, tracked_peer_ports);
+ }
}
void
@@ -275,7 +282,8 @@ local_data_dump_peer_ports(struct hmap *local_datapaths,
struct ds *peer_ports)
void
remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
struct local_datapath *ld,
- struct hmap *local_datapaths)
+ struct hmap *local_datapaths,
+ struct shash *tracked_peer_ports)
{
size_t i = 0;
const struct peer_ports *peers;
@@ -297,6 +305,10 @@ remove_local_datapath_peer_port(const struct
sbrec_port_binding *pb,
local_datapath_usage += vector_memory_usage(&ld->peer_ports);
}
+ if (ld->is_switch) {
+ tracked_peer_port_add(pb, false, tracked_peer_ports);
+ }
+
const struct sbrec_port_binding *peer = removed.remote;
struct local_datapath *peer_ld =
get_local_datapath(local_datapaths, peer->datapath->tunnel_key);
@@ -304,7 +316,8 @@ remove_local_datapath_peer_port(const struct
sbrec_port_binding *pb,
/* Remove the peer port from the peer datapath. The peer
* datapath also tries to remove its peer lport, but that would
* be no-op. */
- remove_local_datapath_peer_port(peer, peer_ld, local_datapaths);
+ remove_local_datapath_peer_port(peer, peer_ld, local_datapaths,
+ tracked_peer_ports);
}
}
@@ -436,6 +449,30 @@ tracked_datapath_lport_add(const struct sbrec_port_binding
*pb,
lport->tracked_type = tracked_type;
}
+/* Adds 'pb' to 'tracked_peer_ports' keyed by its logical port name,
+ * with the last write winning if the same logical port was already
+ * tracked (e.g. the peer relationship was added and removed within the
+ * same IDL iteration). A NULL 'tracked_peer_ports' is allowed, in
+ * which case this is a no-op. */
+void
+tracked_peer_port_add(const struct sbrec_port_binding *pb,
+ bool zone_needed,
+ struct shash *tracked_peer_ports)
+{
+ if (!tracked_peer_ports) {
+ return;
+ }
+
+ struct tracked_peer_port *tpp =
+ shash_find_data(tracked_peer_ports, pb->logical_port);
+ if (!tpp) {
+ tpp = xmalloc(sizeof *tpp);
+ shash_add(tracked_peer_ports, pb->logical_port, tpp);
+ }
+ tpp->pb = pb;
+ tpp->zone_needed = zone_needed;
+}
+
void
tracked_datapaths_clear(struct hmap *tracked_datapaths)
{
diff --git a/controller/local_data.h b/controller/local_data.h
index 130ae536a..06a73141c 100644
--- a/controller/local_data.h
+++ b/controller/local_data.h
@@ -101,11 +101,13 @@ void add_local_datapath_peer_port(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
struct local_datapath *,
struct hmap *local_datapaths,
- struct hmap *tracked_datapaths);
+ struct hmap *tracked_datapaths,
+ struct shash *tracked_peer_ports);
void remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
struct local_datapath *ld,
- struct hmap *local_datapaths);
+ struct hmap *local_datapaths,
+ struct shash *tracked_peer_ports);
enum en_tracked_resource_type {
TRACKED_RESOURCE_NEW,
@@ -138,6 +140,19 @@ void tracked_datapath_lport_add(const struct
sbrec_port_binding *,
void tracked_datapaths_clear(struct hmap *tracked_datapaths);
void tracked_datapaths_destroy(struct hmap *tracked_datapaths);
+/* A switch-side router patch port whose CT zone assignment may have
+ * changed: 'zone_needed' is true when the connected logical switch is
+ * (still) a local datapath, false when the peer relationship was
+ * removed. */
+struct tracked_peer_port {
+ const struct sbrec_port_binding *pb;
+ bool zone_needed;
+};
+
+void tracked_peer_port_add(const struct sbrec_port_binding *pb,
+ bool zone_needed,
+ struct shash *tracked_peer_ports);
+
/* Maps from a chassis to the OpenFlow port number of the tunnel that can be
* used to reach that chassis. */
struct chassis_tunnel {
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 56c917e64..0858312e2 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1390,6 +1390,11 @@ struct ed_type_runtime_data {
bool localnet_learn_fdb_changed;
struct hmap tracked_dp_bindings;
+ /* Tracked peer (switch-side router) patch ports whose CT zone
+ * assignment may have changed. Only meaningful when 'tracked' is
+ * true. */
+ struct shash tracked_peer_ports;
+
struct shash local_active_ports_ipv6_pd;
struct shash local_active_ports_ras;
@@ -1479,6 +1484,7 @@ en_runtime_data_clear_tracked_data(void *data_)
tracked_datapaths_destroy(&data->tracked_dp_bindings);
hmap_init(&data->tracked_dp_bindings);
+ shash_clear_free_data(&data->tracked_peer_ports);
data->local_lports_changed = false;
data->tracked = false;
}
@@ -1503,6 +1509,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
/* Init the tracked data. */
hmap_init(&data->tracked_dp_bindings);
+ shash_init(&data->tracked_peer_ports);
return data;
}
@@ -1522,6 +1529,7 @@ en_runtime_data_cleanup(void *data)
local_datapaths_destroy(&rt_data->local_datapaths);
shash_destroy(&rt_data->local_active_ports_ipv6_pd);
shash_destroy(&rt_data->local_active_ports_ras);
+ shash_destroy_free_data(&rt_data->tracked_peer_ports);
local_binding_data_destroy(&rt_data->lbinding_data);
}
@@ -1616,6 +1624,7 @@ init_binding_ctx(struct engine_node *node,
b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
b_ctx_out->postponed_ports = rt_data->postponed_ports;
b_ctx_out->tracked_dp_bindings = NULL;
+ b_ctx_out->tracked_peer_ports = NULL;
b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
b_ctx_out->localnet_learn_fdb = rt_data->localnet_learn_fdb;
b_ctx_out->localnet_learn_fdb_changed = false;
@@ -1839,6 +1848,7 @@ runtime_data_ovs_interface_shadow_handler(struct
engine_node *node, void *data)
init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
rt_data->tracked = true;
b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
+ b_ctx_out.tracked_peer_ports = &rt_data->tracked_peer_ports;
if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) {
return EN_UNHANDLED;
@@ -1991,6 +2001,7 @@ runtime_data_sb_port_binding_handler(struct engine_node
*node, void *data)
rt_data->tracked = true;
b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
+ b_ctx_out.tracked_peer_ports = &rt_data->tracked_peer_ports;
if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) {
return EN_UNHANDLED;
@@ -2003,7 +2014,8 @@ runtime_data_sb_port_binding_handler(struct engine_node
*node, void *data)
b_ctx_out.non_vif_ports_changed ||
b_ctx_out.local_lports_changed ||
b_ctx_out.localnet_learn_fdb_changed ||
- !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) {
+ !hmap_is_empty(b_ctx_out.tracked_dp_bindings) ||
+ !shash_is_empty(b_ctx_out.tracked_peer_ports)) {
result = EN_HANDLED_UPDATED;
}
@@ -2873,15 +2885,33 @@ ct_zones_runtime_data_handler(struct engine_node *node,
void *data)
continue;
}
- /* The CT zone of a patch port without the ACL flag depends
- * on the local datapaths of the connected switches, which is
- * not tracked incrementally. Fall back to a full recompute
- * of ct_zones so that zones for router ports are assigned
- * and released consistently with ct_zones_update(). */
+ /* The CT zone of a patch port without the ACL flag is needed
+ * if and only if the port is the switch-side peer of a router
+ * port on a local logical switch. Read that condition
+ * directly from the peer set of the port's datapath, which is
+ * the same condition ct_zones_update() applies on a full
+ * recompute; ports whose peer relationship was removed are
+ * also tracked in rt_data->tracked_peer_ports below. */
if (!strcmp(t_lport->pb->type, "patch") &&
!smap_get_bool(&t_lport->pb->options,
"enable_router_port_acl", false)) {
- return EN_UNHANDLED;
+ bool zone_needed = false;
+ struct local_datapath *ld = get_local_datapath(
+ &rt_data->local_datapaths,
+ t_lport->pb->datapath->tunnel_key);
+ if (ld && ld->is_switch) {
+ const struct peer_ports *peers;
+ VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
+ if (peers->local == t_lport->pb) {
+ zone_needed = true;
+ break;
+ }
+ }
+ }
+ updated |= ct_zone_handle_port_update(
+ &ct_zones_data->ctx, t_lport->pb, zone_needed,
+ &scan_start, min_ct_zone, max_ct_zone);
+ continue;
}
bool port_updated =
t_lport->tracked_type == TRACKED_RESOURCE_NEW ||
@@ -2893,6 +2923,15 @@ ct_zones_runtime_data_handler(struct engine_node *node,
void *data)
}
}
+ struct shash_node *tpp_node;
+ SHASH_FOR_EACH (tpp_node, &rt_data->tracked_peer_ports) {
+ struct tracked_peer_port *tpp = tpp_node->data;
+ updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
+ tpp->pb, tpp->zone_needed,
+ &scan_start, min_ct_zone,
+ max_ct_zone);
+ }
+
return updated ? EN_HANDLED_UPDATED : EN_HANDLED_UNCHANGED;
}
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index d600ba2cd..d00780478 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2870,6 +2870,85 @@ OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
OVN_CLEANUP([hv1])
AT_CLEANUP
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - I-P for CT zone patch ports])
+AT_KEYWORDS([ovn-ct-zone-patch-ip])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+# ls1 and ls2 are local through their VIFs and lr0 is local through
+# rp-ls1, so attaching lr0 to ls2 only changes the peer set of already
+# local datapaths.
+check ovn-nbctl lr-add lr0
+check ovn-nbctl ls-add ls1
+check ovn-nbctl lrp-add lr0 rp-ls1 00:00:01:01:02:01 10.0.1.1/24
+check ovn-nbctl lsp-add-router-port ls1 ls1-rp rp-ls1
+check ovn-nbctl lsp-add ls1 lsp1 -- lsp-set-addresses lsp1 \
+"00:11:00:00:00:01 10.0.1.2"
+check ovn-nbctl ls-add ls2
+check ovn-nbctl lsp-add ls2 lsp2 -- lsp-set-addresses lsp2 \
+"00:11:00:00:00:02 10.0.2.2"
+
+as hv1
+check ovs-vsctl add-port br-int vif1 \
+ -- set Interface vif1 external_ids:iface-id=lsp1
+check ovs-vsctl add-port br-int vif2 \
+ -- set Interface vif2 external_ids:iface-id=lsp2
+
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+# Attaching lr0 to the already local ls2 assigns the CT zone of the
+# switch-side patch port incrementally.
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lrp-add lr0 rp-ls2 00:00:01:01:02:02 10.0.2.1/24 \
+ -- lsp-add-router-port ls2 ls2-rp rp-ls2
+OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
+check_controller_engine_stats hv1 runtime_data norecompute compute
+check_controller_engine_stats hv1 ct_zones norecompute compute
+
+# Detaching lr0 from ls2 releases the CT zone incrementally.
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lsp-del ls2-rp -- lrp-del rp-ls2
+OVS_WAIT_WHILE([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
+check_controller_engine_stats hv1 runtime_data norecompute compute
+check_controller_engine_stats hv1 ct_zones norecompute compute
+
+# Re-attaching with the same port names re-assigns the zone
+# incrementally.
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lrp-add lr0 rp-ls2 00:00:01:01:02:02 10.0.2.1/24 \
+ -- lsp-add-router-port ls2 ls2-rp rp-ls2
+OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
+check_controller_engine_stats hv1 ct_zones norecompute compute
+
+# ls3 is not local yet: attaching it to the already local lr0 makes the
+# datapath newly tracked, so a full ct_zones recompute is expected and
+# the zone is assigned by it.
+check ovn-nbctl ls-add ls3
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lrp-add lr0 rp-ls3 00:00:01:01:02:03 10.0.3.1/24 \
+ -- lsp-add-router-port ls3 ls3-rp rp-ls3
+OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls3-rp])
+check_controller_engine_stats hv1 ct_zones recompute nocompute
+
+# Toggling enable_router_port_acl on a local patch port is still handled
+# by the tracked lports path of the ct_zones handler.
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lsp-set-options ls1-rp enable_router_port_acl=true
+OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls1-rp])
+check_controller_engine_stats hv1 ct_zones norecompute compute
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller - Local Chassis_Template_Var updates])
AT_KEYWORDS([templates])
--
2.56.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
