On 9/18/24 12:06, Dumitru Ceara wrote:
> On 9/17/24 19:00, Rosemarie O'Riorden wrote:
>> When preparing to build ECMP and static route flows, routes are sorted
>> into unique routes (meaning they are not part of a group) or they are
>> added to EMCP groups. Then, ECMP route flows are built out of the
>> groups, and static route flows are built out of the unique routes.
>> However, 'unique routes' include ones that use the
>> --ecmp-symmetric-reply flag, meaning that they may not be added to an
>> ECMP group, and thus ECMP symmetric reply would not be used for those
>> flows.
>>
>> For example, if one route is added and traffic is started, and then
>> later another route is added, the already flowing traffic might be
>> rerouted since it wasn't conntracked initially. This could break
>> symmetric reply with traffic using a different next-hop than before.
>>
>> This change makes it so that when the --ecmp-symmetric-reply flag is
>> used, even for unique routes, an ECMP group is created which they are
>> added to. Thus they are added to the ECMP route flow, rather than
>> static. This allows ECMP groups to persist even when there is only one
>> route.
>>
>> Edited documentation to support this change.
>> Also updated incorrect actions in documentation.
>>
>> Fixes: 4fdca656857d ("Add ECMP symmetric replies.")
>> Reported-at: https://issues.redhat.com/browse/FDP-786
>> Signed-off-by: Rosemarie O'Riorden <[email protected]>
>> ---
>
> Hi Rosemarie,
>
> Thanks for the fix!
>
>> northd/northd.c | 33 ++++++++++++++++++++++-----------
>> northd/ovn-northd.8.xml | 13 ++++++++++++-
>> tests/ovn-northd.at | 5 ++++-
>> 3 files changed, 38 insertions(+), 13 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 983c464eb..8ae3a75bd 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -11567,21 +11567,27 @@ build_ecmp_route_flow(struct lflow_table *lflows,
>> struct ovn_datapath *od,
>>
>> struct ds actions = DS_EMPTY_INITIALIZER;
>> ds_put_format(&actions, "ip.ttl--; flags.loopback = 1; %s = %"PRIu16
>> - "; %s = select(", REG_ECMP_GROUP_ID, eg->id,
>> - REG_ECMP_MEMBER_ID);
>> + "; %s = ", REG_ECMP_GROUP_ID, eg->id, REG_ECMP_MEMBER_ID);
>>
>> - bool is_first = true;
>> - LIST_FOR_EACH (er, list_node, &eg->route_list) {
>> - if (is_first) {
>> - is_first = false;
>> - } else {
>> - ds_put_cstr(&actions, ", ");
>> + if (!ovs_list_is_singleton(&eg->route_list)) {
>> + bool is_first = true;
>> +
>> + ds_put_cstr(&actions, "select(");
>> + LIST_FOR_EACH (er, list_node, &eg->route_list) {
>> + if (is_first) {
>> + is_first = false;
>> + } else {
>> + ds_put_cstr(&actions, ", ");
>> + }
>> + ds_put_format(&actions, "%"PRIu16, er->id);
>> }
>> - ds_put_format(&actions, "%"PRIu16, er->id);
>> + ds_put_cstr(&actions, ");");
>> + } else {
>> + er = CONTAINER_OF(ovs_list_front(&eg->route_list),
>> + struct ecmp_route_list_node, list_node);
>> + ds_put_format(&actions, "%"PRIu16"; next;", er->id);
>> }
>>
>> - ds_put_cstr(&actions, ");");
>> -
>
> Isn't it simpler to just allow "select(<single-value>)" logical actions?
> It doesn't feel too wrong to allow select to select a value from a set
> that only contains a single value.
>
> As far as I can tell, from a performance perspective this doesn't make
> much difference. Also, in general, we expect "ecmp routes with a single
> path" to be a transient state, e.g., due to maintenance some paths are
> brought down for a finite duration of time.
>
> We do have to remove the restriction in `parse_select_action()` to allow
> ovn-controller to parse actions of form "select(<single-value>)".
>
> Then we don't need this special treatment for the single next hop case here.
Wouldn't this require even more special treatment in a form of a
feature flag that ovn-controller reports so we don't generate actions
it doesn't understand?
Might be painful, especially if we're going to backport this fix
to stable releases.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
