On Tue, Apr 08, 2025 at 07:45:01AM +0200, Ales Musil via dev wrote:
> On Mon, Apr 7, 2025 at 5:17 PM Felix Huettner <felix.huettner@stackit.cloud>
> wrote:
>
> > 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.
> >
>
> Yes we would extend that behavior to CT zones, but that doesn't mean
> it's the right thing to do. TBF the reason to talk about this was to
> spark the discussion about potential ideas how to deal with this
> which seems to be successful.
>
> >
> > > 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.
> >
>
> That might be very costly to just run full recompute once
> we are done with the waiting.
>
>
> > >
> > > 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.
>
>
> We would need to make sure that this is the case to not potentially
> break combinations with older ovs releases.
Hi Ales,
at least as part of the documentation of the method this should not
happen. Also i think that monitor_cond is in since at least ovs v2.6, so
i guess everyone should have that by now.
>
>
> >
> > 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?
> >
>
> That definitely sounds like something worth pursuing, thank you for
> looking into this.
I'll submit a proposal in the next version.
Thanks,
Felix
>
>
> >
> > 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
> >
> >
> Regards,
> 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
