> 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>
Acked-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> ---
> 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
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
