Previously local ports that had an external_id:iface-id set would get a
ct-zone assigned on recompute, independent if there is a matching
southbound port_binding for them or if this port_binding can actually
reside on the local chassis.
However during incremental processing this was not the case and only
bound local ports would have their ct zone added or removed (as only
they are included in tracked_dp_bindings).
To fix this we add a status enum to local_lports that defines if there
is a matching port_binding or not.
The ct_zones engine node no only considers local_lports that are backed
by some port_binding entry.
This means we can also get rid of workarounds in testcases that where
introduced in 9f052bdb because of this problem.
Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
---
v6->v7:
* introduced
* addressed review comments
controller/binding.c | 33 ++++++++++++++++++++++++---------
controller/binding.h | 8 +++++++-
controller/ct-zone.c | 19 ++++++++++++++-----
controller/ct-zone.h | 2 +-
controller/ovn-controller.c | 21 ++++++++++++---------
controller/physical.h | 2 +-
controller/route.h | 2 +-
tests/ovn.at | 21 ---------------------
8 files changed, 60 insertions(+), 48 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 470aa8c53..f0cc84a50 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -654,11 +654,19 @@ update_ld_localnet_port(const struct sbrec_port_binding
*binding_rec,
* Also track if the set has changed.
*/
static void
-update_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx)
+update_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx,
+ enum binding_local_lport_status status)
{
- if (sset_add(b_ctx->local_lports, iface_id) != NULL) {
- b_ctx->local_lports_changed = true;
+ struct simap_node *node = simap_find(b_ctx->local_lports, iface_id);
+ if (node && node->data == status) {
+ return;
}
+ if (node) {
+ node->data = status;
+ } else {
+ simap_put(b_ctx->local_lports, iface_id, status);
+ }
+ b_ctx->local_lports_changed = true;
}
/* Remove an interface ID from the set of local lports. Also track if the
@@ -667,7 +675,7 @@ update_local_lports(const char *iface_id, struct
binding_ctx_out *b_ctx)
static void
remove_local_lports(const char *iface_id, struct binding_ctx_out *b_ctx)
{
- if (sset_find_and_delete(b_ctx->local_lports, iface_id)) {
+ if (simap_find_and_delete(b_ctx->local_lports, iface_id)) {
b_ctx->local_lports_changed = true;
}
}
@@ -1603,7 +1611,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
b_ctx_out->local_datapaths,
b_ctx_out->tracked_dp_bindings);
update_related_lport(pb, b_ctx_out);
- update_local_lports(pb->logical_port, b_ctx_out);
+ update_local_lports(pb->logical_port, b_ctx_out,
+ LPORT_STATUS_BOUND);
if (binding_lport_update_port_sec(b_lport, pb) &&
b_ctx_out->tracked_dp_bindings) {
tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED,
@@ -1893,6 +1902,10 @@ consider_localport(const struct sbrec_port_binding *pb,
remove_related_lport(pb, b_ctx_out);
}
+ /* Add all localnet ports to local_ifaces so that we allocate ct zones
+ * for them. */
+ update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
+
update_related_lport(pb, b_ctx_out);
return true;
}
@@ -1911,7 +1924,7 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
pb->datapath->tunnel_key);
if (our_chassis) {
- update_local_lports(pb->logical_port, b_ctx_out);
+ update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
if (!ld) {
add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
b_ctx_in->sbrec_port_binding_by_datapath,
@@ -2017,7 +2030,7 @@ consider_localnet_lport(const struct sbrec_port_binding
*pb,
/* Add all localnet ports to local_ifaces so that we allocate ct zones
* for them. */
- update_local_lports(pb->logical_port, b_ctx_out);
+ update_local_lports(pb->logical_port, b_ctx_out, LPORT_STATUS_BOUND);
add_or_del_qos_port(pb->logical_port, true);
update_related_lport(pb, b_ctx_out);
@@ -2118,7 +2131,8 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
iface_rec->name);
}
- update_local_lports(iface_id, b_ctx_out);
+ update_local_lports(iface_id, b_ctx_out,
+ LPORT_STATUS_NOT_BOUND);
smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
iface_id);
} else if (smap_get_bool(&iface_rec->external_ids,
@@ -2399,7 +2413,6 @@ consider_iface_claim(const struct ovsrec_interface
*iface_rec,
struct binding_ctx_in *b_ctx_in,
struct binding_ctx_out *b_ctx_out)
{
- update_local_lports(iface_id, b_ctx_out);
smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
@@ -2427,6 +2440,8 @@ consider_iface_claim(const struct ovsrec_interface
*iface_rec,
pb = b_lport->pb;
}
+ update_local_lports(iface_id, b_ctx_out, pb ? LPORT_STATUS_BOUND :
+ LPORT_STATUS_NOT_BOUND);
if (!pb) {
/* There is no port_binding row for this local binding. */
return true;
diff --git a/controller/binding.h b/controller/binding.h
index 00dd9dbf5..8a5c6996c 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -71,6 +71,12 @@ struct related_lports {
*/
};
+enum binding_local_lport_status {
+ LPORT_STATUS_NOT_BOUND, /* Set if just iface-id is set but the port is
+ not in sb. */
+ LPORT_STATUS_BOUND, /* Set if the port is in sb and locally relevant. */
+};
+
void related_lports_init(struct related_lports *);
void related_lports_destroy(struct related_lports *);
@@ -81,7 +87,7 @@ struct binding_ctx_out {
struct local_binding_data *lbinding_data;
/* sset of (potential) local lports. */
- struct sset *local_lports;
+ struct simap *local_lports;
/* Track if local_lports have been updated. */
bool local_lports_changed;
diff --git a/controller/ct-zone.c b/controller/ct-zone.c
index 469a8fc54..cf8f92b41 100644
--- a/controller/ct-zone.c
+++ b/controller/ct-zone.c
@@ -167,7 +167,7 @@ out:
}
void
-ct_zones_update(const struct sset *local_lports,
+ct_zones_update(const struct simap *local_lports,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct hmap *local_datapaths, struct ct_zone_ctx *ctx)
{
@@ -178,9 +178,11 @@ ct_zones_update(const struct sset *local_lports,
unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES);
struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones);
- const char *local_lport;
- SSET_FOR_EACH (local_lport, local_lports) {
- sset_add(&all_users, local_lport);
+ const struct simap_node *sinode;
+ SIMAP_FOR_EACH (sinode, local_lports) {
+ if (sinode->data == LPORT_STATUS_BOUND) {
+ sset_add(&all_users, sinode->name);
+ }
}
/* Local patched datapath (gateway routers) need zones assigned. */
@@ -217,7 +219,14 @@ ct_zones_update(const struct sset *local_lports,
SHASH_FOR_EACH_SAFE (node, &ctx->current) {
struct ct_zone *ct_zone = node->data;
if (!sset_contains(&all_users, node->name)) {
- ct_zone_remove(ctx, node->name);
+ /* If we started recently and do not monitor all db entries then we
+ * might not have yet learned a port binding entry to the port.
+ * In this case we skip removing the zone until we either know the
+ * port binding or we are sure the zone is no longer needed. */
+ if (!(daemon_started_recently() &&
+ simap_contains(local_lports, node->name))) {
+ ct_zone_remove(ctx, node->name);
+ }
} else if (!simap_find(&req_snat_zones, node->name)) {
if (ct_zone->zone < min_ct_zone || ct_zone->zone > max_ct_zone) {
ct_zone_remove(ctx, node->name);
diff --git a/controller/ct-zone.h b/controller/ct-zone.h
index 6df03975c..cc313d423 100644
--- a/controller/ct-zone.h
+++ b/controller/ct-zone.h
@@ -70,7 +70,7 @@ void ct_zones_restore(struct ct_zone_ctx *ctx,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct sbrec_datapath_binding_table *dp_table,
const struct ovsrec_bridge *br_int);
-void ct_zones_update(const struct sset *local_lports,
+void ct_zones_update(const struct simap *local_lports,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct hmap *local_datapaths,
struct ct_zone_ctx *ctx);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 4dfb426f9..4e1623163 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -218,7 +218,7 @@ static char *get_file_system_id(void)
static unsigned int
update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
const struct sbrec_chassis *chassis,
- const struct sset *local_ifaces,
+ const struct simap *local_ifaces,
const struct shash *local_bindings,
struct hmap *local_datapaths,
bool monitor_all)
@@ -358,7 +358,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
const char *name;
ovs_assert(local_bindings);
- SSET_FOR_EACH (name, local_ifaces) {
+ const struct simap_node *n;
+ SIMAP_FOR_EACH (n, local_ifaces) {
+ name = n->name;
/* Skip the VIFs we bound already, we should have a local datapath
* for those. */
const struct sbrec_port_binding *local_pb
@@ -1291,8 +1293,9 @@ struct ed_type_runtime_data {
* hypervisor. These logical ports include the VIFs (and their child
* logical ports, if any) that belong to VMs running on the hypervisor,
* l2gateway ports for which options:l2gateway-chassis designates the
- * local hypervisor, and localnet ports. */
- struct sset local_lports;
+ * local hypervisor, and localnet ports.
+ * The value is mapped to enum binding_local_lport_status. */
+ struct simap local_lports;
/* Port bindings that are relevant to the local chassis (VIFs bound
* localy, patch ports).
@@ -1396,7 +1399,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
struct ed_type_runtime_data *data = xzalloc(sizeof *data);
hmap_init(&data->local_datapaths);
- sset_init(&data->local_lports);
+ simap_init(&data->local_lports);
related_lports_init(&data->related_lports);
sset_init(&data->active_tunnels);
hmap_init(&data->qos_map);
@@ -1416,7 +1419,7 @@ en_runtime_data_cleanup(void *data)
{
struct ed_type_runtime_data *rt_data = data;
- sset_destroy(&rt_data->local_lports);
+ simap_destroy(&rt_data->local_lports);
related_lports_destroy(&rt_data->related_lports);
sset_destroy(&rt_data->active_tunnels);
destroy_qos_map(&rt_data->qos_map);
@@ -1530,7 +1533,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
struct hmap *local_datapaths = &rt_data->local_datapaths;
struct shash *local_active_ipv6_pd = &rt_data->local_active_ports_ipv6_pd;
struct shash *local_active_ras = &rt_data->local_active_ports_ras;
- struct sset *local_lports = &rt_data->local_lports;
+ struct simap *local_lports = &rt_data->local_lports;
struct sset *active_tunnels = &rt_data->active_tunnels;
static bool first_run = true;
@@ -1542,13 +1545,13 @@ en_runtime_data_run(struct engine_node *node, void
*data)
shash_clear(local_active_ipv6_pd);
shash_clear(local_active_ras);
local_binding_data_destroy(&rt_data->lbinding_data);
- sset_destroy(local_lports);
+ simap_destroy(local_lports);
related_lports_destroy(&rt_data->related_lports);
sset_destroy(active_tunnels);
destroy_qos_map(&rt_data->qos_map);
smap_destroy(&rt_data->local_iface_ids);
hmap_init(local_datapaths);
- sset_init(local_lports);
+ simap_init(local_lports);
related_lports_init(&rt_data->related_lports);
sset_init(active_tunnels);
hmap_init(&rt_data->qos_map);
diff --git a/controller/physical.h b/controller/physical.h
index 1a126ce8d..77831e536 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -60,7 +60,7 @@ struct physical_ctx {
const struct sset *active_tunnels;
const struct if_status_mgr *if_mgr;
struct hmap *local_datapaths;
- struct sset *local_lports;
+ struct simap *local_lports;
const struct shash *ct_zones;
enum mf_field_id mff_ovn_geneve;
struct shash *local_bindings;
diff --git a/controller/route.h b/controller/route.h
index 5047ce202..15323d357 100644
--- a/controller/route.h
+++ b/controller/route.h
@@ -37,7 +37,7 @@ struct route_ctx_in {
const char *dynamic_routing_port_mapping;
const struct sset *active_tunnels;
const struct hmap *local_datapaths;
- const struct sset *local_lports;
+ const struct simap *local_lports;
struct shash *local_bindings;
};
diff --git a/tests/ovn.at b/tests/ovn.at
index cf8cdce3a..fb68abbd1 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -15772,16 +15772,6 @@ echo $request >> hv3/third.expected
check_packets
-# ct_zone allocation differs between I+P and recompute:
-# - In I+P an ovs interface with external-ids:iface-id set (but no
-# logical_port in sb) results in no ct_zone allocated.
-# - In recompute, in the same case, a ct_zone is allocated - this is
-# necessary to avoid flushing ct_zone if/when controller is restarted.
-# As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
-# some ct-zone related differences in flows before and after recompute.
-# Avoid/hide this by removing iface-id on hv1.
-
-as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
OVN_CLEANUP([hv1],[hv2],[hv3])
AT_CLEANUP
@@ -16172,11 +16162,6 @@ echo $request >> hv2/n1.expected
check_packets
-# ct_zone allocation differs between I+P and recompute.
-# As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
-# some ct-zone related differences in flows before and after recompute.
-# Avoid/hide this by removing iface-id on hv1.
-as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
OVN_CLEANUP([hv1],[hv2],[hv3])
AT_CLEANUP
@@ -16733,12 +16718,6 @@ m4_define([ACTIVATION_STRATEGY_TEST],
check ovn-nbctl lsp-set-options migrator requested-chassis=hv2
OVS_WAIT_UNTIL([test x = x$(ovn-sbctl get Port_Binding $pb_uuid
options:additional-chassis-activated)])
- # ct_zone allocation differs between I+P and recompute.
- # As migrrator has iface-id set on hv1 but is bound to hv2, we would hit
- # some ct-zone related differences in flows before and after recompute.
- # Avoid/hide this by removing iface-id on hv1.
- as hv1 check ovs-vsctl remove Interface migrator external-ids iface-id
-
OVN_CLEANUP([hv1],[hv2],[hv3])
AT_CLEANUP
--
2.43.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
