[ovs-dev] ovn-northd: Do not add lflows in lr_in_arp_resolve stage for disabled logical ports

Message ID 20170927160618.32523-1-nusiddiq@redhat.com
State Accepted
Delegated to: Guru Shetty
Headers show
Series
  • [ovs-dev] ovn-northd: Do not add lflows in lr_in_arp_resolve stage for disabled logical ports
Related show

Commit Message

Numan Siddique Sept. 27, 2017, 4:06 p.m.
From: Numan Siddique <nusiddiq@redhat.com>

ovn-northd is adding the below logical flow for a disabled logical port (with mac M
and IP 'A')

table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 == 'A'),
action=(eth.dst = 'M'; next;)

In the case of openstack load balancer 'octavia' service, it creates logical
ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2 and
adds IP2 to P1 - (M1 IP1 IP2).

When another port tries to reach IP2, it doesn't get delivered to port P1 because
of the above flow.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/northd/ovn-northd.c |  4 ++++
 tests/ovn.at            | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Ben Pfaff Oct. 25, 2017, 9:29 p.m. | #1
On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> ovn-northd is adding the below logical flow for a disabled logical port (with mac M
> and IP 'A')
> 
> table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 == 'A'),
> action=(eth.dst = 'M'; next;)
> 
> In the case of openstack load balancer 'octavia' service, it creates logical
> ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2 and
> adds IP2 to P1 - (M1 IP1 IP2).
> 
> When another port tries to reach IP2, it doesn't get delivered to port P1 because
> of the above flow.
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Thanks a lot, I applied this to master.

Please let me know if I should backport it to 2.8 (or earlier).
Numan Siddique Oct. 26, 2017, 1:26 a.m. | #2
On Thu, Oct 26, 2017 at 2:59 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > ovn-northd is adding the below logical flow for a disabled logical port
> (with mac M
> > and IP 'A')
> >
> > table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 ==
> 'A'),
> > action=(eth.dst = 'M'; next;)
> >
> > In the case of openstack load balancer 'octavia' service, it creates
> logical
> > ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2
> and
> > adds IP2 to P1 - (M1 IP1 IP2).
> >
> > When another port tries to reach IP2, it doesn't get delivered to port
> P1 because
> > of the above flow.
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>
> Thanks a lot, I applied this to master.
>
> Please let me know if I should backport it to 2.8 (or earlier).
>

Thanks for the review. It would be great if it can be backported to 2.8.

Numan
Ben Pfaff Feb. 2, 2018, 12:32 a.m. | #3
On Thu, Oct 26, 2017 at 06:56:46AM +0530, Numan Siddique wrote:
> On Thu, Oct 26, 2017 at 2:59 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusiddiq@redhat.com wrote:
> > > From: Numan Siddique <nusiddiq@redhat.com>
> > >
> > > ovn-northd is adding the below logical flow for a disabled logical port
> > (with mac M
> > > and IP 'A')
> > >
> > > table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 ==
> > 'A'),
> > > action=(eth.dst = 'M'; next;)
> > >
> > > In the case of openstack load balancer 'octavia' service, it creates
> > logical
> > > ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2
> > and
> > > adds IP2 to P1 - (M1 IP1 IP2).
> > >
> > > When another port tries to reach IP2, it doesn't get delivered to port
> > P1 because
> > > of the above flow.
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> >
> > Thanks a lot, I applied this to master.
> >
> > Please let me know if I should backport it to 2.8 (or earlier).
> >
> 
> Thanks for the review. It would be great if it can be backported to 2.8.

Apparently I forgot to ever do that, but I did it now.
Numan Siddique Feb. 2, 2018, 8:53 a.m. | #4
On Fri, Feb 2, 2018 at 6:02 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Oct 26, 2017 at 06:56:46AM +0530, Numan Siddique wrote:
> > On Thu, Oct 26, 2017 at 2:59 AM, Ben Pfaff <blp@ovn.org> wrote:
> >
> > > On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusiddiq@redhat.com wrote:
> > > > From: Numan Siddique <nusiddiq@redhat.com>
> > > >
> > > > ovn-northd is adding the below logical flow for a disabled logical
> port
> > > (with mac M
> > > > and IP 'A')
> > > >
> > > > table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0
> ==
> > > 'A'),
> > > > action=(eth.dst = 'M'; next;)
> > > >
> > > > In the case of openstack load balancer 'octavia' service, it creates
> > > logical
> > > > ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port
> P2
> > > and
> > > > adds IP2 to P1 - (M1 IP1 IP2).
> > > >
> > > > When another port tries to reach IP2, it doesn't get delivered to
> port
> > > P1 because
> > > > of the above flow.
> > > >
> > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > >
> > > Thanks a lot, I applied this to master.
> > >
> > > Please let me know if I should backport it to 2.8 (or earlier).
> > >
> >
> > Thanks for the review. It would be great if it can be backported to 2.8.
>
> Apparently I forgot to ever do that, but I did it now.
>

Thanks Ben.

Numan

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 2db238073..c1a76480a 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5368,6 +5368,10 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
      * resolves the IP address in reg0 into an output port in outport and an
      * Ethernet address in eth.dst. */
     HMAP_FOR_EACH (op, key_node, ports) {
+        if (op->nbsp && !lsp_is_enabled(op->nbsp)) {
+            continue;
+        }
+
         if (op->nbrp) {
             /* This is a logical router port. If next-hop IP address in
              * '[xx]reg0' matches IP address of this router port, then
diff --git a/tests/ovn.at b/tests/ovn.at
index 6c38b973f..490841c63 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3050,6 +3050,27 @@  echo $packet | ovstest test-ovn expr-to-packets > expected
 
 OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
 
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_arp_resolve | \
+grep "reg0 == 172.16.1.2" | wc -l], [0], [1
+])
+
+# Disable the ls2-lp1 port.
+ovn-nbctl --wait=hv set logical_switch_port ls2-lp1 enabled=false
+
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_arp_resolve | \
+grep "reg0 == 172.16.1.2" | wc -l], [0], [0
+])
+
+# Generate the packet destined for ls2-lp1 and it should not be delivered.
+# Packet to send.
+packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac &&
+        ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
+        udp && udp.src==53 && udp.dst==4369"
+
+as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+# The 2nd packet sent shound not be received.
+OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
+
 OVN_CLEANUP([hv1],[hv2])
 
 AT_CLEANUP