[ovs-dev] [PATCH ovn] ovn: introduce IP_SRC_POLICY stage in ingress router pipeline

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Fri May 22 21:23:29 UTC 2020


> On Wed, May 20, 2020 at 12:43 PM Lorenzo Bianconi <
> lorenzo.bianconi at redhat.com> wrote:
> >

[...]

> >
> > Regards,
> > Lorenzo
> >
> 
> Hi Lorenzo,
> 

Hi Han,

thx for the detailed explanation

> Thanks for explaining the scenario (here and also today's OVN meeting).
> Going through the patch again, I think there are still something unclear to
> me. Let me describe my understanding to the problem first:
> The logical topology is:
> VM -> LS -> LR -> External LS -> TOR
> 
> And physically, VM is on a Chassis, the LR has a distributed gateway port
> on a GW chassis.
> The VM has dnat_and_snat configured in OVN with a floating IP and external
> mac, and we expect the VM -> TOR traffic to directly go out through the
> VM's chassis without going through the GW node.

correct

> 
> Now IP traffic goes as expected, but for the first packet before the ARP is
> resolved from LR for the TOR's IP, the ARP request is triggered and sent
> from the VM chassis to the GW node over the tunnel, and broadcasted from GW
> node to the TOR. The first IP packet that triggers the ARP is buffered on
> the VM chassis during the ARP process, and when ARP response is received
> and MAC-binding is updated, the buffered IP packet is also sent to GW node
> over the tunnel and sent to the TOR from there, instead of directly from
> the VM chassis (according to what you mentioned in the ovn meeting). The
> second and later IP packets go from VM chassis to the TOR directly.
> However, since the first IP packet (with the nat external MAC) is sent out
> from gateway node, the TOR's MAC table can get confused.

correct, this was the original issue opened by OSP folks

> 
> Please let know if the above is accurate about the behavior before commit
> c0bf32d72f8b, and correct me if any step is wrong.
> 
> Now to the root cause. It seems the problem is in the ARP resolving. In
> ingress stage "ARP Request", since the dst MAC is still unresolved, the
> arp() action will be executed by pinctrl (now still on the VM chassis).
> When the arp is reinjected to OVS, since it is going through the pipelines
> from the begining (this is my understanding about PACKET_OUT, but I am not
> sure), when it hits the GW_REDIRECT stage, since it is not IP packet, it
> won't get matched by the flow that bypasses the redirect, so the output
> port is replaced by the CR-P, and redirected to the GW chassis through
> tunnel.
> 
> As to the buffered packet, when it is reinjected to OVS, if it go through
> the pipeline again, it should bypass the redirection since in GW_REDIRECT
> stage it should hit the flow that matches: ip.src == nat->logical_ip. So I
> don't understand why would it get tunneled to GW node.
> 
> Maybe there is something I misunderstood about the packet reinjection
> behavior. Please correct me. I should have spent time myself to debug it
> but I guess you might have already debugged so I'd like to save some effort
> here ;)
> 
> Now to the solution. Since I am not completely clear about the root cause,
> I can't say for sure about the solution. However, here are some suggestions
> based on your patch, if it is proved to solve the problem already.
> 1) This patch is completely different solution from the original commit
> c0bf32d72f8b, and the old solution was wrong and abandoned, so I'd suggest
> to submit as two separate patches: i) revert the old commit, and ii) the
> new solution. This way, it would be more clear for the review and also for
> others to understand the patch later.

ack, fine

> 
> 2) It seems all this patch is doing is matching the "ip.src ==
> nat->logical_ip and is_chassis_residency(nat->logical_port)" and then
> update the eth.src and reg1. I am not sure why it solves the problem, but I
> think this doesn't need a new stage. I think it can be added in the
> GW_REDIRECT stage (before ARP_REQUEST), with a higher priority flow with
> the same match and actions.

ack, I am fine with it. It seemed to me more clear having a dedicated stage for
it but I am 100% fine with your suggestion since it is cheaper

> 
> 3) In the new function add_ip_src_policy_flows(), the condition checks
> seems not sufficient to determine if it the nat can be performed in a
> "distributed" manner. Since the logic is related to NAT, it is better to
> put it together under the section /* NAT, Defrag and load balancing. */ of
> the function build_lrouter_flows(), where the related checks are already
> performed to determine if it is "distributed".
> 
>             /* For distributed router NAT, determine whether this NAT rule
>              * satisfies the conditions for distributed NAT processing. */
>             bool distributed = false;
>             struct eth_addr mac;
>             if (od->l3dgw_port && !strcmp(nat->type, "dnat_and_snat") &&
>                 nat->logical_port && nat->external_mac) {
>                 if (eth_addr_from_string(nat->external_mac, &mac)) {
>                     distributed = true;
>                 } else {
>                     static struct vlog_rate_limit rl =
>                         VLOG_RATE_LIMIT_INIT(5, 1);
>                     VLOG_WARN_RL(&rl, "bad mac %s for dnat in router "
>                         ""UUID_FMT"", nat->external_mac,
> UUID_ARGS(&od->key));
>                     continue;
>                 }
>             }
> 
> Just put the new flow addition under this section it would be easier to
> follow and avoid repeating the logic of determing if it is "distributed".

ack, thx a lot for the hint, I will implement it tomorrow and I will send a v3
if OSP folks confirm it works fine (but I guess so)

Regards,
Lorenzo

> 
> Thanks,
> Han
> 
> > >
> > > > > ---
> > > > > Changes since RFC:
> > > > > - added unit-tests
> > > > > ---
> > > > >  northd/ovn-northd.8.xml | 65
> ++++++++++++++++++++---------------------
> > > > >  northd/ovn-northd.c     | 38 ++++++++++--------------
> > > > >  tests/ovn.at            | 28 +++++-------------
> > > > >  tests/system-ovn.at     | 28 ++++++++++++++++++
> > > > >  4 files changed, 82 insertions(+), 77 deletions(-)
> > > > >
> > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > > > > index 8f224b07f..09dbb52b4 100644
> > > > > --- a/northd/ovn-northd.8.xml
> > > > > +++ b/northd/ovn-northd.8.xml
> > > > > @@ -2484,37 +2484,6 @@ output;
> > > > >          </p>
> > > > >        </li>
> > > > >
> > > > > -      <li>
> > > > > -        <p>
> > > > > -          For distributed logical routers where one of the logical
> > > router ports
> > > > > -          specifies a <code>redirect-chassis</code>, a priority-400
> > > logical
> > > > > -          flow for each <code>dnat_and_snat</code> NAT rules
> > > configured.
> > > > > -          These flows will allow to properly forward traffic to the
> > > external
> > > > > -          connections if available and avoid sending it through the
> > > tunnel.
> > > > > -          Assuming the following NAT rule has been configured:
> > > > > -        </p>
> > > > > -
> > > > > -        <pre>
> > > > > -external_ip = <var>A</var>;
> > > > > -external_mac = <var>B</var>;
> > > > > -logical_ip = <var>C</var>;
> > > > > -        </pre>
> > > > > -
> > > > > -        <p>
> > > > > -          the following action will be applied:
> > > > > -        </p>
> > > > > -
> > > > > -        <pre>
> > > > > -ip.ttl--;
> > > > > -reg0 = <var>ip.dst</var>;
> > > > > -reg1 = <var>A</var>;
> > > > > -eth.src = <var>B</var>;
> > > > > -outport = <var>router-port</var>;
> > > > > -next;
> > > > > -        </pre>
> > > > > -
> > > > > -      </li>
> > > > > -
> > > > >        <li>
> > > > >          <p>
> > > > >            IPv4 routing table.  For each route to IPv4 network
> > > <var>N</var> with
> > > > > @@ -2660,7 +2629,35 @@ outport = <var>P</var>;
> > > > >        </li>
> > > > >      </ul>
> > > > >
> > > > > -    <h3>Ingress Table 12: ARP/ND Resolution</h3>
> > > > > +    <h3>Ingress Table 12: IP Source Policy</h3>
> > > > > +
> > > > > +    <p>
> > > > > +      This table contains for distributed logical routers where
> one of
> > > > > +      the logical router ports specifies a
> > > <code>redirect-chassis</code>,
> > > > > +      a priority-100 logical flow for each
> <code>dnat_and_snat</code>
> > > > > +      NAT rules configured.
> > > > > +      These flows will allow to properly forward traffic to the
> > > external
> > > > > +      connections if available and avoid sending it through the
> tunnel.
> > > > > +      Assuming the following NAT rule has been configured:
> > > > > +    </p>
> > > > > +
> > > > > +    <pre>
> > > > > +external_ip = <var>A</var>;
> > > > > +external_mac = <var>B</var>;
> > > > > +logical_ip = <var>C</var>;
> > > > > +    </pre>
> > > > > +
> > > > > +    <p>
> > > > > +      the following action will be applied:
> > > > > +    </p>
> > > > > +
> > > > > +    <pre>
> > > > > +reg1 = <var>A</var>;
> > > > > +eth.src = <var>B</var>;
> > > > > +next;
> > > > > +    </pre>
> > > > > +
> > > >
> > > > This section of the man page will show what action we perform on
> > > > traffic, i.e., move external_ip in reg1 (for SNAT) and move
> external_mac
> > > > in eth.src but there's no mention about what kind of traffic will
> match
> > > > these flows, that is: "ip.src == logical_ip &&
> > > > is_chassis_resident(logical_port)".
> > > >
> > > > > +    <h3>Ingress Table 13: ARP/ND Resolution</h3>
> > > > >
> > > > >      <p>
> > > > >        Any packet that reaches this table is an IP packet whose
> next-hop
> > > > > @@ -2819,7 +2816,7 @@ outport = <var>P</var>;
> > > > >
> > > > >      </ul>
> > > > >
> > > > > -    <h3>Ingress Table 13: Check packet length</h3>
> > > > > +    <h3>Ingress Table 14: Check packet length</h3>
> > > > >
> > > > >      <p>
> > > > >        For distributed logical routers with distributed gateway port
> > > configured
> > > > > @@ -2849,7 +2846,7 @@ REGBIT_PKT_LARGER =
> > > check_pkt_larger(<var>L</var>); next;
> > > > >        and advances to the next table.
> > > > >      </p>
> > > > >
> > > > > -    <h3>Ingress Table 14: Handle larger packets</h3>
> > > > > +    <h3>Ingress Table 15: Handle larger packets</h3>
> > > > >
> > > >
> > > > Indices for tables "Gateway Redirect" and "ARP Request" should be
> > > > updated too.
> > > >
> > > > >      <p>
> > > > >        For distributed logical routers with distributed gateway port
> > > configured
> > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > > index 3c0070ea7..d5f3997a9 100644
> > > > > --- a/northd/ovn-northd.c
> > > > > +++ b/northd/ovn-northd.c
> > > > > @@ -175,11 +175,12 @@ enum ovn_stage {
> > > > >      PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,      9,
> > > "lr_in_ip_routing")   \
> > > > >      PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 10,
> > > "lr_in_ip_routing_ecmp") \
> > > > >      PIPELINE_STAGE(ROUTER, IN,  POLICY,          11,
> "lr_in_policy")
> > >     \
> > > > > -    PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE,     12,
> > > "lr_in_arp_resolve")  \
> > > > > -    PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN   ,  13,
> > > "lr_in_chk_pkt_len")   \
> > > > > -    PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS,
> > > 14,"lr_in_larger_pkts")   \
> > > > > -    PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT,     15,
> > > "lr_in_gw_redirect")  \
> > > > > -    PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST,     16,
> > > "lr_in_arp_request")  \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  IP_SRC_POLICY,   12,
> > > "lr_in_ip_src_policy") \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE,     13,
> > > "lr_in_arp_resolve")  \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN   ,  14,
> > > "lr_in_chk_pkt_len")   \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS,
> > > 15,"lr_in_larger_pkts")   \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT,     16,
> > > "lr_in_gw_redirect")  \
> > > > > +    PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST,     17,
> > > "lr_in_arp_request")  \
> > > > >
>    \
> > > > >      /* Logical router egress stages. */
>     \
> > > > >      PIPELINE_STAGE(ROUTER, OUT, UNDNAT,    0, "lr_out_undnat")
>    \
> > > > > @@ -7103,8 +7104,6 @@ build_routing_policy_flow(struct hmap *lflows,
> > > struct ovn_datapath *od,
> > > > >      ds_destroy(&actions);
> > > > >  }
> > > > >
> > > > > -/* default logical flow prioriry for distributed routes */
> > > > > -#define DROUTE_PRIO 400
> > > > >  struct parsed_route {
> > > > >      struct ovs_list list_node;
> > > > >      struct v46_ip prefix;
> > > > > @@ -7493,7 +7492,7 @@ build_ecmp_route_flow(struct hmap *lflows,
> struct
> > > ovn_datapath *od,
> > > > >  }
> > > > >
> > > > >  static void
> > > > > -add_distributed_routes(struct hmap *lflows, struct ovn_datapath
> *od)
> > > > > +add_ip_src_policy_flows(struct hmap *lflows, struct ovn_datapath
> *od)
> > > > >  {
> > > > >      struct ds actions = DS_EMPTY_INITIALIZER;
> > > > >      struct ds match = DS_EMPTY_INITIALIZER;
> > > > > @@ -7511,12 +7510,9 @@ add_distributed_routes(struct hmap *lflows,
> > > struct ovn_datapath *od)
> > > > >                        is_ipv4 ? "4" : "6", nat->logical_ip,
> > > > >                        nat->logical_port);
> > > > >          char *prefix = is_ipv4 ? "" : "xx";
> > > > > -        ds_put_format(&actions, "outport = %s; eth.src = %s; "
> > > > > -                      "%sreg0 = ip%s.dst; %sreg1 = %s; next;",
> > > > > -                      od->l3dgw_port->json_key, nat->external_mac,
> > > > > -                      prefix, is_ipv4 ? "4" : "6",
> > > > > -                      prefix, nat->external_ip);
> > > > > -        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
> DROUTE_PRIO,
> > > > > +        ds_put_format(&actions, "eth.src = %s; %sreg1 = %s; next;",
> > > > > +                      nat->external_mac, prefix, nat->external_ip);
> > > > > +        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_SRC_POLICY, 100,
> > > > >                        ds_cstr(&match), ds_cstr(&actions));
> > > > >          ds_clear(&match);
> > > > >          ds_clear(&actions);
> > > > > @@ -7547,12 +7543,6 @@ add_route(struct hmap *lflows, const struct
> > > ovn_port *op,
> > > > >      }
> > > > >      build_route_match(op_inport, network_s, plen, is_src_route,
> > > is_ipv4,
> > > > >                        &match, &priority);
> > > > > -    /* traffic for internal IPs of logical switch ports must be
> sent to
> > > > > -     * the gw controller through the overlay tunnels
> > > > > -     */
> > > > > -    if (op->nbrp && !op->nbrp->n_gateway_chassis) {
> > > > > -        priority += DROUTE_PRIO;
> > > > > -    }
> > > > >
> > > > >      struct ds actions = DS_EMPTY_INITIALIZER;
> > > > >      ds_put_format(&actions, "ip.ttl--; "REG_ECMP_GROUP_ID" = 0;
> %sreg0
> > > = ",
> > > > > @@ -9519,9 +9509,13 @@ build_lrouter_flows(struct hmap *datapaths,
> > > struct hmap *ports,
> > > > >       * logical router
> > > > >       */
> > > > >      HMAP_FOR_EACH (od, key_node, datapaths) {
> > > > > -        if (od->nbr && od->l3dgw_port) {
> > > > > -            add_distributed_routes(lflows, od);
> > > > > +        if (!od->nbr) {
> > > > > +            continue;
> > > > > +        }
> > > > > +        if (od->l3dgw_port) {
> > > > > +            add_ip_src_policy_flows(lflows, od);
> > > > >          }
> > > > > +        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_SRC_POLICY, 0,
> "1",
> > > "next;");
> > > > >      }
> > > > >
> > > > >      /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP:
> IP
> > > Routing.
> > > > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > > > index f39fda2e4..fcc34fd5d 100644
> > > > > --- a/tests/ovn.at
> > > > > +++ b/tests/ovn.at
> > > > > @@ -9637,20 +9637,6 @@ AT_CHECK([as hv3 ovs-vsctl set Open_vSwitch .
> > > external-ids:ovn-bridge-mappings=p
> > > > >  OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-vsctl show | \
> > > > >  grep "Port patch-br-int-to-ln_port" | wc -l`])
> > > > >
> > > > > -AT_CHECK([test 1 = `ovn-sbctl dump-flows lr0 | grep
> lr_in_ip_routing |
> > > \
> > > > > -grep "ip4.src == 10.0.0.3 && is_chassis_resident(\"foo1\")" -c`])
> > > > > -AT_CHECK([test 1 = `ovn-sbctl dump-flows lr0 | grep
> lr_in_ip_routing |
> > > \
> > > > > -grep "ip4.src == 10.0.0.4 && is_chassis_resident(\"foo2\")" -c`])
> > > > > -
> > > > > -key=`ovn-sbctl --bare --columns tunnel_key list datapath_Binding
> lr0`
> > > > > -# Check that the OVS flows appear for the dnat_and_snat entries in
> > > > > -# lr_in_ip_routing table.
> > > > > -OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-ofctl dump-flows br-int
> table=17
> > > | \
> > > > > -grep "priority=400,ip,metadata=0x$key,nw_src=10.0.0.3" -c`])
> > > > > -
> > > > > -OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-ofctl dump-flows br-int
> table=17
> > > | \
> > > > > -grep "priority=400,ip,metadata=0x$key,nw_src=10.0.0.4" -c`])
> > > > > -
> > > > >  # Re-add nat-addresses option
> > > > >  ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0
> > > nat-addresses="router"
> > > > >
> > > > > @@ -15141,7 +15127,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  # Since the sw0-vir is not claimed by any chassis, eth.dst should
> be
> > > set to
> > > > >  # zero if the ip4.dst is the virtual ip in the router pipeline.
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00;
> next;)
> > > > >  ])
> > > > >
> > > > >  ip_to_hex() {
> > > > > @@ -15192,7 +15178,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  # There should be an arp resolve flow to resolve the virtual_ip
> with
> > > the
> > > > >  # sw0-p1's MAC.
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03;
> next;)
> > > > >  ])
> > > > >
> > > > >  # Forcibly clear virtual_parent. ovn-controller should release the
> > > binding
> > > > > @@ -15233,7 +15219,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  # There should be an arp resolve flow to resolve the virtual_ip
> with
> > > the
> > > > >  # sw0-p2's MAC.
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05;
> next;)
> > > > >  ])
> > > > >
> > > > >  # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
> > > > > @@ -15256,7 +15242,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  # There should be an arp resolve flow to resolve the virtual_ip
> with
> > > the
> > > > >  # sw0-p3's MAC.
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04;
> next;)
> > > > >  ])
> > > > >
> > > > >  # Now send arp reply from sw0-p1. hv1 should claim sw0-vir
> > > > > @@ -15277,7 +15263,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  > lflows.txt
> > > > >
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03;
> next;)
> > > > >  ])
> > > > >
> > > > >  # Delete hv1-vif1 port. hv1 should release sw0-vir
> > > > > @@ -15295,7 +15281,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  > lflows.txt
> > > > >
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00;
> next;)
> > > > >  ])
> > > > >
> > > > >  # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
> > > > > @@ -15316,7 +15302,7 @@ ovn-sbctl dump-flows lr0 | grep
> > > lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> > > > >  > lflows.txt
> > > > >
> > > > >  AT_CHECK([cat lflows.txt], [0], [dnl
> > > > > -  table=12(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04;
> next;)
> > > > > +  table=13(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> > > "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04;
> next;)
> > > > >  ])
> > > > >
> > > > >  # Delete sw0-p2 logical port
> > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > >
> > > > I think it's preferable to have the unit tests in ovn.at whenever
> > > > possible so that they get run regularly.
> > > >
> > > > > index 9ae6c6b1f..1e4f147b4 100644
> > > > > --- a/tests/system-ovn.at
> > > > > +++ b/tests/system-ovn.at
> > > > > @@ -2747,6 +2747,17 @@ ADD_VETH(alice1, alice1, br-int, "
> 172.16.1.2/24",
> > > "f0:00:00:01:02:05", \
> > > > >  ovn-nbctl lsp-add alice alice1 \
> > > > >  -- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
> > > > >
> > > > > +# Add external network
> > > > > +ADD_NAMESPACES(ext-net)
> > > > > +ip link add alice-ext netns alice1 type veth peer name ext-veth
> netns
> > > ext-net
> > > > > +ip -n ext-net link set dev ext-veth up
> > > > > +ip -n ext-net addr add 10.0.0.1/24 dev ext-veth
> > > > > +ip -n ext-net route add default via 10.0.0.2
> > > > > +
> > > > > +ip -n alice1 link set dev alice-ext up
> > > > > +ip -n alice1 addr add 10.0.0.2/24 dev alice-ext
> > > > > +ip netns exec alice1 sysctl -w net.ipv4.conf.all.forwarding=1
> > > > > +
> > > > >  # Add DNAT rules
> > > > >  AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3
> 192.168.1.2
> > > foo1 00:00:02:02:03:04])
> > > > >  AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4
> 192.168.1.3
> > > foo2 00:00:02:02:03:05])
> > > > > @@ -2754,6 +2765,9 @@ AT_CHECK([ovn-nbctl lr-nat-add R1
> dnat_and_snat
> > > 172.16.1.4 192.168.1.3 foo2 00:0
> > > > >  # Add a SNAT rule
> > > > >  AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16])
> > > > >
> > > > > +# Add default route to ext-net
> > > > > +AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 172.16.1.2])
> > > > > +
> > > > >  ovn-nbctl --wait=hv sync
> > > > >  OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep
> > > 'nat(src=172.16.1.1)'])
> > > > >
> > > > > @@ -2776,6 +2790,20 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3
> -w 2
> > > 172.16.1.2 | FORMAT_PING], \
> > > > >  3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > > > >  ])
> > > > >
> > > > > +# Try to ping external network
> > > > > +NS_CHECK_EXEC([ext-net], [tcpdump -n -c 3 -i ext-veth dst
> 172.16.1.3
> > > and icmp > ext-net.pcap &])
> > > > > +sleep 1
> > > > > +AT_CHECK([ovn-nbctl lr-nat-del R1 snat])
> > > > > +NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.1 |
> > > FORMAT_PING], \
> > > > > +[0], [dnl
> > > > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > > > > +])
> > > > > +
> > > > > +OVS_WAIT_UNTIL([
> > > > > +    total_pkts=$(cat ext-net.pcap | wc -l)
> > > > > +    test "${total_pkts}" = "3"
> > > > > +])
> > > > > +
> > > > >  # We verify that SNAT indeed happened via 'dump-conntrack' command.
> > > > >  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1)
> | \
> > > > >  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> > > > >
> > > >
> > > > The system test fails on my test machine (Fedora 32 with OVS master
> and
> > > > OVN master + your patch applied):
> > > >
> > > > make check-system-userspace TESTSUITEFLAGS="17"
> > > > [...]
> > > >  17: ovn -- DNAT and SNAT on distributed router - N/S FAILED
> > > > (system-ovn.at:2823)
> > > >
> > > > [...]
> > > > system-ovn.at:2802: wait succeeded after 1 seconds
> > > > ./system-ovn.at:2808: ovs-appctl dpctl/dump-conntrack | grep
> > > > "dst=172.16.1.1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e
> > > > 's/id=[0-9]*/id=<cleared>/g' -e
> 's/state=[0-9_A-Z]*/state=<cleared>/g' |
> > > > sort | uniq | \
> > > > sed -e 's/zone=[0-9]*/zone=<cleared>/'
> > > > ./system-ovn.at:2813: ovs-appctl dpctl/flush-conntrack
> > > > ./system-ovn.at:2817: ip netns exec bar1 sh << NS_EXEC_HEREDOC ping -q
> > > > -c 3 -i 0.3 -w 2 172.16.1.2 | grep "transmitted" | sed
> 's/time.*ms$/time
> > > > 0ms/'
> > > > NS_EXEC_HEREDOC
> > > > ./system-ovn.at:2823: ovs-appctl dpctl/dump-conntrack | grep
> > > > "dst=172.16.1.1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e
> > > > 's/id=[0-9]*/id=<cleared>/g' -e
> 's/state=[0-9_A-Z]*/state=<cleared>/g' |
> > > > sort | uniq | \
> > > > sed -e 's/zone=[0-9]*/zone=<cleared>/'
> > > > --- -   2020-05-20 10:28:50.053113201 -0400
> > > > +++ /root/ovn/tests/system-userspace-testsuite.dir/at-groups/17/stdout
> > > > 2020-05-20 10:28:50.050668409 -0400
> > > > @@ -1,2 +1 @@
> > > >
> > >
> -icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
> > > > [...]
> > > >
> > > > Same happens when running the system test with the kernel datapath:
> > > > make check-kernel TESTSUITEFLAGS="17"
> > > > [...]
> > > > 17: ovn -- DNAT and SNAT on distributed router - N/S FAILED
> > > > (system-ovn.at:2823)
> > > >
> > > > Regards,
> > > > Dumitru
> > > >


More information about the dev mailing list