On Mon, Feb 24, 2025 at 03:11:06PM +0100, Dumitru Ceara wrote:
> On 2/24/25 3:01 PM, Felix Huettner wrote:
> > On Thu, Feb 20, 2025 at 02:46:53PM +0100, Dumitru Ceara wrote:
> >> On 2/18/25 3:02 PM, Felix Huettner via dev wrote:
> >>> This adds initial incremental support for the new group_ecmp_route
> >>> engine. It only supports incremental processing for learned routes for
> >>> now as other route types miss the incremental handling up the chain.
> >>>
> >>> Signed-off-by: Felix Huettner <[email protected]>
> >>> ---
> >>
> >> Hi Felix,
> >
> > Hi Dumitru,
> >
> > thanks for the review.
> >
>
> Hi Felix,
Hi Dumitru,
>
> [...]
>
> >>> +
> >>> +bool
> >>> +group_ecmp_route_learned_route_change_handler(struct engine_node
> >>> *eng_node,
> >>> + void *_data)
> >>> +{
> >>> + struct group_ecmp_route_data *data = _data;
> >>> + struct learned_route_sync_data *learned_route_data
> >>> + = engine_get_input_data("learned_route_sync", eng_node);
> >>> +
> >>> + if (learned_route_data->trk_data.type ==
> >>> LEARNED_ROUTES_TRACKED_NONE) {
> >>> + data->tracked = false;
> >>> + return false;
> >>> + }
> >>> +
> >>> + data->tracked = true;
> >>> +
> >>> + struct hmapx updated_routes = HMAPX_INITIALIZER(&updated_routes);
> >>> +
> >>> + const struct hmapx_node *hmapx_node;
> >>> + const struct parsed_route *pr;
> >>> + HMAPX_FOR_EACH (hmapx_node,
> >>> +
> >>> &learned_route_data->trk_data.trk_created_parsed_route) {
> >>> + pr = hmapx_node->data;
> >>> + handle_added_route(data, pr, &updated_routes);
> >>> + }
> >>> +
> >>> + HMAPX_FOR_EACH (hmapx_node,
> >>> +
> >>> &learned_route_data->trk_data.trk_deleted_parsed_route) {
> >>> + pr = hmapx_node->data;
> >>> + if (!handle_deleted_route(data, pr, &updated_routes)) {
> >>> + return false;
> >>> + }
> >>> + }
> >>
> >> Is there a reason to not handle deletions first? It might be safer to
> >> do so, just in case.
> >
> > This might point to also an issue in the incremental logic of the
> > en_learned_route_sync.
> >
> > I am thinking now of two scenarios that we need to handle in a single
> > incremental run.
> >
> > In Scenario 1 we have no preexisting learned route. First a learned_route is
> > added and is then removed again.
> >
> > In Scenario 2 we have a preexisting learned route. First this route is
> > removed. Then a new one is added with basically the same value.
> >
> > The final result of Scenario 1 should be that we do not learn this
> > route, the final result of Scenario 2 contains the route.
> >
> > We could handle Scenario 1 (on the en_learned_route_sync side) by
> > checking if a route is both created and deleted. If yes it could be
> > removed from both hmapx. That means that this engine node never needs to
> > care about that issue.
>
> Maybe we should do this anyway though, i.e., don't advertise the route
> as changed if it was somehow added + deleted in the same
> en_learned_route_sync run.
>
> > Alternatively the IDL already handles this and does not even present
> > this Learned_Route. I just don't know if that is the case.
> >
>
> That is the case already. If addition followed by deletion both happen
> between two runs of the I-P engine, the IDL will take care of it and
> ovn-northd won't be notified, i.e., it's level driven.
>
> > I think Scenario 2 should already be fine, but it is probably more
> > efficient if we first handle the deletions.
> >
> > I would be interested in your opinion.
> >
>
> FWIW I vote for deletion first and for checking whether add + delete
> somehow happened in the same en_learned_route_sync execution and
> dropping those notifications.
Ok thanks a lot,
I'll update the patches.
Thanks,
Felix
>
> Regards,
> Dumitru
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
