On 9/18/24 12:39, Dumitru Ceara wrote:
> On 9/18/24 12:31, Ilya Maximets wrote:
>> 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.
>
> Good point. Although our official upgrade policy still states that
> ovn-controller should be upgraded first or version pinning should be
> enabled (in this case ovn-controller will not try to parse the new SB
> flow until it is restarted).
>
> https://docs.ovn.org/en/latest/intro/install/ovn-upgrades.html#upgrade-procedures
Sure, but shouldn't we also remove some of the chassis feature flags then?
The ones that rely only on ovn-controller version and not anything external
like OVS or kernel. Some of them were added very recently with the
justification of the opposite upgrade order. See this commit for example:
d9c97878eb23 ("actions: New action ct_commit_to_zone.")
If the upgrade order is not enforced for new features, there is no point
doing that for bug fix patches.
Also, the upgrade order only applies to major upgrades AFAIK. After
this fix is backported, users are free to perform minor package updates
in any order and they'll hit the issue.
>
> This bug report came from OpenShift and there, as far as I know, there's
> a guarantee that the ovn-controller version will always be >= to the
> ovn-northd version. To my knowledge they're the only users of
> ecmp-symmetric-reply but of course we can't know that 100% sure.
At least kube-ovn supports ecmp with symmetric reply configuration.
So, OpenShift is unlikely to be the only user.
>
>>
>> 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.
>>
>
> It's a bit different there as there's no concept of "equal cost" policy
> so we can't really know if the intention is to add more paths in the
> future or not. While with routes, IMO, it doesn't make sense to have a
> "single path equal cost multi-path" route - except when that state is
> transient.
I can see someone adding this flag to the route just in case someday
they'll need to add another one. Totally valid use case IMO, and the
configuration will not be transient.
>
> But given that we have this similar code for policies, we might as well
> do it for routes too as Rosemarie's patch does.
OK.
>
> I'd still prefer to have system tests to cover the single path case too
> though.
Sure.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
