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.
> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, priority,
> ds_cstr(&route_match), ds_cstr(&actions),
> lflow_ref);
> @@ -13543,6 +13549,11 @@ build_static_route_flows_for_lrouter(
> if (group) {
> ecmp_groups_add_route(group, route);
> }
> + } else if (route->ecmp_symmetric_reply) {
> + /* Traffic for symmetric reply routes has to be conntracked
> + * even if there is only one next-hop, in case another
> next-hop
> + * is added later. */
> + ecmp_groups_add(&ecmp_groups, route);
I think we can simplify the fix to only this part if we adapt the
"select()" action as described above.
> } else {
> unique_routes_add(&unique_routes, route);
> }
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index ede38882a..ef5cd0c8c 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -4328,7 +4328,18 @@ next;
> ip.ttl--;
> flags.loopback = 1;
> reg8[0..15] = <var>GID</var>;
> -select(reg8[16..31], <var>MID1</var>, <var>MID2</var>, ...);
> +reg8[16..31] = select(<var>MID1</var>, <var>MID2</var>, ...);
> + </pre>
> + <p>
> + However, when there is only one route in an ECMP group, group
> actions
> + will be:
> + </p>
> +
> + <pre>
> +ip.ttl--;
> +flags.loopback = 1;
> +reg8[0..15] = <var>GID</var>;
> +reg8[16..31] = <var>MID1</var>);
> </pre>
> </li>
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index dcc3dbbc3..d459c23c0 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -6799,20 +6799,23 @@ check ovn-nbctl lsp-set-type public-lr0 router
> check ovn-nbctl lsp-set-addresses public-lr0 router
> check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>
> +# ECMP flows will be added even if there is only one next-hop.
> check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1
> 192.168.0.10
>
> ovn-sbctl dump-flows lr0 > lr0flows
>
> AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> table=??(lr_in_ip_routing ), priority=0 , match=(1), action=(drop;)
> + table=??(lr_in_ip_routing ), priority=10300,
> match=(ct_mark.ecmp_reply_port == 1 && reg7 == 0 && ip4.dst == 1.0.0.1/32),
> action=(ip.ttl--; flags.loopback = 1; eth.src = 00:00:20:20:12:13; reg1 =
> 192.168.0.1; outport = "lr0-public"; next;)
> table=??(lr_in_ip_routing ), priority=10550, match=(nd_rs || nd_ra),
> action=(drop;)
> table=??(lr_in_ip_routing ), priority=194 , match=(inport ==
> "lr0-public" && ip6.dst == fe80::/64), action=(ip.ttl--; reg8[[0..15]] = 0;
> xxreg0 = ip6.dst; xxreg1 = fe80::200:20ff:fe20:1213; eth.src =
> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; next;)
> table=??(lr_in_ip_routing ), priority=74 , match=(ip4.dst ==
> 192.168.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = ip4.dst; reg1 =
> 192.168.0.1; eth.src = 00:00:20:20:12:13; outport = "lr0-public";
> flags.loopback = 1; next;)
> - table=??(lr_in_ip_routing ), priority=97 , match=(reg7 == 0 && ip4.dst
> == 1.0.0.1/32), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 192.168.0.10;
> reg1 = 192.168.0.1; eth.src = 00:00:20:20:12:13; outport = "lr0-public";
> flags.loopback = 1; next;)
> + table=??(lr_in_ip_routing ), priority=97 , match=(reg7 == 0 && ip4.dst
> == 1.0.0.1/32), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1;
> reg8[[16..31]] = 1; next;)
> ])
>
> AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0],
> [dnl
> table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), action=(drop;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 1), action=(reg0 = 192.168.0.10; reg1 = 192.168.0.1;
> eth.src = 00:00:20:20:12:13; outport = "lr0-public"; next;)
> table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] ==
> 0), action=(next;)
> ])
>
Thanks for updating the test but would it also be possible to update the
"ECMP symmetric reply" and "ECMP IPv6 symmetric reply" system tests in
system-ovn.at?
We could try to ensure that when we delete one of the two equal cost
paths traffic still flows and conntrack label (and mark?) are correctly set.
What do you think?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
