Recheck-request: github-robot-_ovn-kubernetes
On 9/17/24 1:00 PM, 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]>
> ---
> 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, ");");
> -
> 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);
> } 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;)
> ])
>
--
Rosemarie O'Riorden
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
