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,
[...]
>>> +
>>> +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.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
