On 9/18/24 12:26, Ilya Maximets wrote:
> 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.
Also, there is already a similar code in northd for router policies. So,
it would make more sense to change both at once outside of a bug fix.
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
