On Thu, Apr 24, 2025 at 10:33:52AM +0200, Ales Musil wrote:
> On Mon, Apr 14, 2025 at 1:37 PM Felix Huettner via dev <
> ovs-dev@openvswitch.org> wrote:
>
> > 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>
> > ---
> >
>
> Hi Felix,
>
> there is only one nit that can be addressed during merge.
Hi Ales,
since i need a new version anyway i will include it in there.
>
> 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))) {
> >
>
> nit: Indentation
>
> + 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
> >
> >
> Other than that it looks good:
> Acked-by: Ales Musil <amu...@redhat.com>
Thanks a lot,
Felix
>
> Thanks,
> Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
