On Mon, Apr 07, 2025 at 11:15:32AM +0200, Felix Huettner via dev wrote:
> On Thu, Apr 03, 2025 at 12:35:22PM +0200, Ales Musil wrote:
> > On Thu, Mar 20, 2025 at 1:01 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.
> > >
> > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > > ---
> > >
> >
> > Hi Felix,
> >
> > thank you for the patch. There are some workaround in tests
> > introduced by 9f052bdb [0]. Could please try to remove those
> > workaround as part of this patch to see if it helps?
>
> Hi Ales,
>
> thanks a lot.
> I'll remove them in the next version.
>
> >
> > Also I have some concerns with the approach. See down below.
> >
> > v6->v7: introduced
> > >
> > > controller/binding.c | 29 ++++++++++++++++++++---------
> > > 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 +-
> > > 7 files changed, 56 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index 470aa8c53..a080b1041 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,
> > > @@ -1911,7 +1920,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 +2026,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 +2127,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 +2409,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;
> > > @@ -2429,8 +2438,10 @@ consider_iface_claim(const struct ovsrec_interface
> > > *iface_rec,
> > >
> > > if (!pb) {
> > > /* There is no port_binding row for this local binding. */
> > > + update_local_lports(iface_id, b_ctx_out, LPORT_STATUS_NOT_BOUND);
> > > return true;
> > > }
> > > + update_local_lports(iface_id, b_ctx_out, LPORT_STATUS_BOUND);
> > >
> >
> > nit: there could be single function call above the if statement:
> >
> > update_local_lports(iface_id, b_ctx_out, pb ? LPORT_STATUS_BOUND :
> > LPORT_STATUS_NOT_BOUND);
>
> Ok, will update that.
>
> >
> >
> > >
> > > /* Check if iface-id-ver just becomes correct */
> > > struct smap *external_ids_old =
> > > 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))) {
> > >
> >
> > The logic here worries me, ovn-controller guarantees that it won't
> > flush zones for ports after restart. However, this might not be the
> > case anymore after this patch. The started_recently adds some delay
> > which is finite. Also looking at the code, who will actually trigger
> > recompute of the ct_zones node? We might not clear the port zones
> > until something else triggers ct_zone node.
>
> Isn't this something that is generally valid for all uses of
> daemon_started_recently?
> I mean once it returns true we will also start unplugging ports,
> independent if we actually have all data or not. So maybe this would
> just extend this "problem" to also ct zones.
> I'm not sure if there is a nice other option, until we have a way of
> knowing when all data has been loaded.
>
> For the recompute part we previously did not have the issue, since the
> code that waits does run outside the engine.
> However we could probably trigger a general recompute once the
> started_recently timer elapses.
>
> Let me know what you think.
Hi Ales,
i thought more about this and maybe have an idea how we can generally
improve the daemon_started_recently.
If we have monitor-all enabled then we can generally say
daemon_started_recently should return true once we got our initial
monitoring updates.
If monitor-all is disabled we would need a few more steps:
1. Get the initial data from southbound
2. Run the engine
3. Update the monitor conditions
4. Wait for the data with the new monitoring conditions in place
In case the ovsdb-server supports monitor_cond or monitor_cond_since
(which probably all of them do by now) we also know that the initial
monitor data is sent as part of the reply to the monitor request. This
is at least what i learned during experimenting, i hope that holds
generally.
When we call ovsdb_idl_get_condition_seqno in the ovn-controller main
loop we can compare that against the monitor condition ids previously
set (we get that from update_sb_monitors) and if they are equal we know
we have up-to-data data available in the idl.
This means for monitor-all we are now good to process changes.
For non-monitor-all we would need to go though the above states to get
accurate monitor definitions.
That would allow us to get rid of the time and iteration sensitive
method that is currently used by daemon_started_recently. Also since
then the condition of daemon_started_recently is only depending on the
sb connection we trigger computes/recomputes automatically anyway.
Does that sound valid to you and would it also resolve your above
concerns?
Thanks a lot,
Felix
>
> Thanks a lot,
> Felix
>
> >
> >
> > > + 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 a54db35ae..1ce076a04 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
> > > @@ -1277,8 +1279,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).
> > > @@ -1382,7 +1385,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);
> > > @@ -1402,7 +1405,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);
> > > @@ -1516,7 +1519,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;
> > > @@ -1528,13 +1531,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;
> > > };
> > >
> > > --
> > > 2.43.0
> > >
> > >
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > >
> > [0]
> > https://github.com/ovn-org/ovn/commit/9f052bdb2b969b13f8165d9bf465ec0852832750
> >
> > Thanks,
> > Ales
> _______________________________________________
> 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
