On Fri, May 2, 2025 at 7:48 PM Mark Michelson <mmich...@redhat.com> wrote:
> Hi Felix,
>
> I have one nit below, and it can easily be fixed when committing.
> Otherwise, this patch looks good to me.
>
> Acked-by: Mark Michelson <mmich...@redhat.com>
>
> On 4/30/25 03:37, Felix Huettner via dev wrote:
> > This fixes two issues at once:
> > 1. previously we did not treat changed potential route-exchange ports
> > correctly as route_exchange_relevant_port does not consider
> > chassisredirect ports.
> > 2. The sbrec_advertised_route change handler also triggered for updates
> > which would cause it to also trigger on any changed port_binding that
> > is referenced. As these changes are already handled in the other
> > handlers this is unnecessary.
> >
> > We can also remove the change handling of local<->remote port in the
> > port_binding handler as [1] now handles them in runtime_data without a
> > roundtrip to southbound.
> >
> > [1]: a3f72e44ddde72fbc82d72568ac517455648cbc1
> >
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
> > controller/ovn-controller.c | 81 ++++++++++++++++++++++++++-----------
> > 1 file changed, 57 insertions(+), 24 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 37406f923..b3e50f7aa 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -5096,10 +5096,36 @@ route_runtime_data_handler(struct engine_node
> *node, void *data)
> > struct ed_type_runtime_data *rt_data =
> > engine_get_input_data("runtime_data", node);
> >
> > + const struct ovsrec_open_vswitch_table *ovs_table =
> > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + ovs_assert(chassis_id);
> > +
> > + struct ovsdb_idl_index *sbrec_chassis_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_chassis", node),
> > + "name");
> > + const struct sbrec_chassis *chassis
> > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
> > + ovs_assert(chassis);
> > +
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_port_binding", node),
> > + "name");
> > +
> > if (!rt_data->tracked) {
> > return false;
> > }
> >
> > + /* There are the following cases where we need to handle updates to
> > + * runtime_data:
> > + * 1. A datapath binding has changed that is already taking part in
> route
> > + * exchange.
> > + * 2. A route-exchange relevant port went form local to remote or
> the
> > + * other way round.
> > + * 2. A tracked_port went from local to remote or the other way
> round.
>
> This should be numbered 3 instead of 2.
>
> > + * */
> > struct tracked_datapath *t_dp;
> > HMAP_FOR_EACH (t_dp, node, &rt_data->tracked_dp_bindings) {
> > struct tracked_datapath *re_t_dp =
> > @@ -5114,7 +5140,10 @@ route_runtime_data_handler(struct engine_node
> *node, void *data)
> > struct shash_node *shash_node;
> > SHASH_FOR_EACH (shash_node, &t_dp->lports) {
> > struct tracked_lport *lport = shash_node->data;
> > - if (route_exchange_relevant_port(lport->pb)) {
> > +
> > + if (route_exchange_find_port(sbrec_port_binding_by_name,
> chassis,
> > + &rt_data->active_tunnels,
> > + lport->pb)) {
> > /* XXX: Until we get I-P support for route exchange we
> need to
> > * request recompute. */
> > return false;
> > @@ -5174,6 +5203,16 @@ route_sb_port_binding_data_handler(struct
> engine_node *node, void *data)
> > struct ed_type_runtime_data *rt_data =
> > engine_get_input_data("runtime_data", node);
> >
> > +
> > + /* There are the following cases where we need to handle updates to
> the
> > + * port_binding table:
> > + * 1. The port_binding is part of a router datapath that already
> takes
> > + * part in route exchange.
> > + * 2. The port_binding is now becoming part of route exchange.
> > + *
> > + * We do not need to handle port_bindings that are tracked_ports
> and switch
> > + * between being local and remote. This is handled as part of the
> > + * runtime_data handler. */
> > const struct sbrec_port_binding *sbrec_pb;
> > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_pb, pb_table) {
> > struct tracked_datapath *re_t_dp =
> > @@ -5185,32 +5224,12 @@ route_sb_port_binding_data_handler(struct
> engine_node *node, void *data)
> > return false;
> > }
> >
> > - if (route_exchange_relevant_port(sbrec_pb)) {
> > + if (route_exchange_find_port(sbrec_port_binding_by_name,
> chassis,
> > + &rt_data->active_tunnels,
> sbrec_pb)) {
> > /* XXX: Until we get I-P support for route exchange we
> need to
> > * request recompute. */
> > return false;
> > }
> > -
> > - if (sset_contains(&re_data->tracked_ports_local,
> > - sbrec_pb->logical_port)) {
> > - if (!route_exchange_find_port(sbrec_port_binding_by_name,
> chassis,
> > - &rt_data->active_tunnels,
> > - sbrec_pb)) {
> > - /* The port was previously local but now it no longer
> is. */
> > - return false;
> > - }
> > - }
> > -
> > - if (sset_contains(&re_data->tracked_ports_remote,
> > - sbrec_pb->logical_port)) {
> > - if (route_exchange_find_port(sbrec_port_binding_by_name,
> chassis,
> > - &rt_data->active_tunnels,
> > - sbrec_pb)) {
> > - /* The port was previously remote but now we bound it.
> */
> > - return false;
> > - }
> > - }
> > -
> > }
> >
> > return true;
> > @@ -5223,13 +5242,27 @@ route_sb_advertised_route_data_handler(struct
> engine_node *node, void *data)
> > const struct sbrec_advertised_route_table *advertised_route_table =
> > EN_OVSDB_GET(engine_get_input("SB_advertised_route", node));
> >
> > + /* There are the following cases where we need to handle updates to
> the
> > + * advertised_route table:
> > + * 1. The advertised_route is created or deleted and we know about
> its
> > + * datapath locally.
> > + *
> > + * Updates to advertised_route can generally be ignored as northd
> will not
> > + * update these entries. We also get update notifications if a
> referenced
> > + * port_binding is updated, but these are handled in the
> runtime_data
> > + * handler. */
> > const struct sbrec_advertised_route *sbrec_route;
> > SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_TRACKED (sbrec_route,
> >
> advertised_route_table) {
> > struct tracked_datapath *re_t_dp =
> > tracked_datapath_find(&re_data->tracked_route_datapaths,
> > sbrec_route->datapath);
> > - if (re_t_dp) {
> > + if (!re_t_dp) {
> > + continue;
> > + }
> > +
> > + if (sbrec_advertised_route_is_new(sbrec_route) ||
> > + sbrec_advertised_route_is_deleted(sbrec_route)) {
> > /* XXX: Until we get I-P support for route exchange we
> need to
> > * request recompute. */
> > return false;
>
>
Thanks Felix and Mark,
I took care of the comment and applied this to main and branch-25.03.
Regards,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
