| Message ID | 1a4c990579a5c1cf90c8eba6e0f4d21e7344c297.1709817303.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce ECMP_nexthop monitor in ovn-controller | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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.bianconi@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
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.bianconi@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
> 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.bianconi@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. ack, I will fix it in v2. > > > 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. ack, I will fix it in v2. Regards, Lorenzo > > > -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 >
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); + } 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 ]) -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
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.bianconi@redhat.com> --- northd/northd.c | 11 +++++++++-- tests/ovn.at | 4 ++-- tests/system-ovn.at | 48 ++++++++++++++++++++++++--------------------- 3 files changed, 37 insertions(+), 26 deletions(-)