Re: [ovs-dev] [PATCH ovn 2/3] northd: Add nexhop id in ct_label.label.

Fri, 08 Mar 2024 08:43:13 -0800 

Hi Lorenzo,

Just a couple of small comments below.

On 3/7/24 08:19, Lorenzo Bianconi wrote:

Introduce the nexthop identifier in the ct_label.label field for
ecmp-symmetric replies connections. This field will be used by
ovn-controller to track ct entries and to flush them if requested by the
CMS (e.g. removing the related static routes).

Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
---
  northd/northd.c     | 11 +++++++++--
  tests/ovn.at        |  4 ++--
  tests/system-ovn.at | 48 ++++++++++++++++++++++++---------------------
  3 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 3770f9f94..e85339704 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10600,15 +10600,22 @@ add_ecmp_symmetric_reply_flows(struct lflow_table 
*lflows,
       * ds_put_cstr() call. The previous contents are needed.
       */
      ds_put_cstr(&match, " && !ct.rpl && (ct.new || ct.est)");
+    struct ds nexthop_label = DS_EMPTY_INITIALIZER;
+    int id = smap_get_int(&st_route->options, "nexthop-id", -1);
+    if (id > 0) {
+        ds_put_format(&nexthop_label, "ct_label.label = %d;", id);
+    }
As mentioned in my review of patch 1, this should use the SB ECMP_nexthop nexthop-id instead of the NB static route nexthop-id. 


      ds_put_format(&actions,
              "ct_commit { ct_label.ecmp_reply_eth = eth.src; "
-            " %s = %" PRId64 ";}; "
+            " %s = %" PRId64 "; %s }; "
              "next;",
-            ct_ecmp_reply_port_match, out_port->sb->tunnel_key);
+            ct_ecmp_reply_port_match, out_port->sb->tunnel_key,
+            ds_cstr(&nexthop_label));
      ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100,
                              ds_cstr(&match), ds_cstr(&actions),
                              &st_route->header_,
                              lflow_ref);
+    ds_destroy(&nexthop_label);
/* Bypass ECMP selection if we already have ct_label information 
       * for where to route the packet.
diff --git a/tests/ovn.at b/tests/ovn.at
index d26c95054..d5ee7a1f3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29181,7 +29181,7 @@ AT_CHECK([
      for hv in 1 2; do
          grep table=17 hv${hv}flows | \
          grep "priority=100" | \
-        grep -c 
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+        grep -c 
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \ 
          grep "priority=200" | \
@@ -29306,7 +29306,7 @@ AT_CHECK([
      for hv in 1 2; do
          grep table=17 hv${hv}flows | \
          grep "priority=100" | \
-        grep -c 
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+        grep -c 
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \ 
          grep "priority=200" | \
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 2411b0267..146bf70e2 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6121,19 +6121,20 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 
10.0.0.2 | FORMAT_PING], \
  # and just ensure that the known ethernet address is present.
  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
  sed -e 's/zone=[[0-9]]*/zone=<cleared>/' |
-sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl
-icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x401020400000000
-tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x401020400000000,protoinfo=(state=<cleared>)
+sed -e 's/mark=[[0-9]]*/mark=<cleared>/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/'], [0], [dnl
+icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020400000000
+tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020400000000,protoinfo=(state=<cleared>)
  ])
# Ensure datapaths show conntrack states as expected 
  # Like with conntrack entries, we shouldn't try to predict
  # port binding tunnel keys. So omit them from expected labels.
  ovs-appctl dpctl/dump-flows | grep 
'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)'
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' | 
grep '401020400000000' -c], [0], [dnl
  2
  ])
I'm not a fan of this change. By removing "ct(.*label=" from the grep, it's not as clear what is being searched for in the flow. 


-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk)' | 
grep '401020400000000)' -c], [0], [dnl
  2
  ])
@@ -6152,18 +6153,19 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | FORMAT_PING], \ 
  [0], [dnl
  3 packets transmitted, 3 received, 0% packet loss, time 0ms
  ])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' | 
grep '1001020400000000/.*)' -c], [0], [dnl
  2
  ])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk)' | 
grep '1001020400000000)' -c], [0], [dnl
  2
  ])
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x1001020400000000 | FORMAT_CT(172.16.0.1) | \ 
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 1001020400000000 | 
FORMAT_CT(172.16.0.1) | \
  sed -e 's/zone=[[0-9]]*/zone=<cleared>/' |
-sed -e 's/mark=[[0-9]]*/mark=<cleared>/' | sort], [0], [dnl
-icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000
-tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000,protoinfo=(state=<cleared>)
+sed -e 's/mark=[[0-9]]*/mark=<cleared>/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
+icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000
+tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000,protoinfo=(state=<cleared>)
  ])
  # Check entries in table 76 and 77 expires w/o traffic
  OVS_WAIT_UNTIL([
@@ -6322,11 +6324,11 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 fd01::2 
| FORMAT_PING], \
  # Ensure datapaths show conntrack states as expected
  # Like with conntrack entries, we shouldn't try to predict
  # port binding tunnel keys. So omit them from expected labels.
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' | 
grep '401020400000000/.*)' -c], [0], [dnl
  2
  ])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl 
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk)' | 
grep '401020400000000)' -c], [0], [dnl
  2
  ])
@@ -6335,9 +6337,10 @@ AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_lab 
  # and just ensure that the known ethernet address is present.
  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd01::2) | \
  sed -e 's/zone=[[0-9]]*/zone=<cleared>/' |
-sed -e 's/mark=[[0-9]]*/mark=<cleared>/' | sort], [0], [dnl
-icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x401020400000000
-tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x401020400000000,protoinfo=(state=<cleared>)
+sed -e 's/mark=[[0-9]]*/mark=<cleared>/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/' | sort], [0], [dnl
+icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020400000000
+tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000000401020400000000,protoinfo=(state=<cleared>)
  ])
# Flush conntrack entries for easier output parsing of next test. 
@@ -6354,18 +6357,19 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 fd01::2 
| FORMAT_PING], \
  3 packets transmitted, 3 received, 0% packet loss, time 0ms
  ])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl 
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' | 
grep '1001020400000000/.*)' -c], [0], [dnl
  2
  ])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep 
'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk)' | 
grep '1001020400000000)' -c], [0], [dnl
  2
  ])
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x1001020400000000 | FORMAT_CT(fd01::2) | \ 
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 1001020400000000 | 
FORMAT_CT(fd01::2) | \
  sed -e 's/zone=[[0-9]]*/zone=<cleared>/' |
-sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl
-icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000
-tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000,protoinfo=(state=<cleared>)
+sed -e 's/mark=[[0-9]]*/mark=<cleared>/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/'], [0], [dnl
+icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000
+tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x?000000001001020400000000,protoinfo=(state=<cleared>)
  ])
# Check entries in table 76 and 77 expires w/o traffic 

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to