[ovs-dev,3/3] ovn-controller: Add test for setting up and tearing down patch ports.
diff mbox

Message ID 1444251436-26967-3-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 7, 2015, 8:57 p.m. UTC
The initial plan for OVN logical routers will make more extensive use of
patch ports, so it seems like a good idea to add some tests to avoid
regressions before messing with them.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 tests/automake.mk       |  1 +
 tests/ovn-controller.at | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/testsuite.at      |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 tests/ovn-controller.at

Comments

Justin Pettit Oct. 7, 2015, 9:25 p.m. UTC | #1
> On Oct 7, 2015, at 1:57 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> +# Waits until the OVS database contains exactly the specified patch ports.
> +# Each argument should be of the form BRIDGE PORT PEER.
> +check_patches () {
> +    # Generate code to check that the set of patch ports is exactly as
> +    # specified.
> +    printf > query 'ovs-vsctl -f csv -d bare --no-headings --columns=name find interface type=patch | sort
> +ovs-vsctl init'

I think it would be helpful to explain why "ovs-vsctl init" is being used.  I got hung up on "ovs-vsctl init" and not the fact that it was building an ovs-vsctl command with a variable number of arguments, which could be zero, in which case the "init" is a no-op that allows the command to run without generating an error.

Acked-by: Justin Pettit <jpettit@nicira.com>

--Justin
Ben Pfaff Oct. 7, 2015, 9:32 p.m. UTC | #2
On Wed, Oct 07, 2015 at 02:25:18PM -0700, Justin Pettit wrote:
> 
> > On Oct 7, 2015, at 1:57 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > +# Waits until the OVS database contains exactly the specified patch ports.
> > +# Each argument should be of the form BRIDGE PORT PEER.
> > +check_patches () {
> > +    # Generate code to check that the set of patch ports is exactly as
> > +    # specified.
> > +    printf > query 'ovs-vsctl -f csv -d bare --no-headings --columns=name find interface type=patch | sort
> > +ovs-vsctl init'
> 
> I think it would be helpful to explain why "ovs-vsctl init" is being
> used.  I got hung up on "ovs-vsctl init" and not the fact that it was
> building an ovs-vsctl command with a variable number of arguments,
> which could be zero, in which case the "init" is a no-op that allows
> the command to run without generating an error.

Thanks for pointing that out.

On reflection, this is only there to allow for a single call to
ovs-vsctl instead of one per port.  It's easier to understand if the
code just dumps out a separate ovs-vsctl command per port, so I just
made that change instead.

> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks, I've applied these patches now.

Patch
diff mbox

diff --git a/tests/automake.mk b/tests/automake.mk
index d183a1d..95edda3 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -89,6 +89,7 @@  TESTSUITE_AT = \
 	tests/ovn.at \
 	tests/ovn-nbctl.at \
 	tests/ovn-sbctl.at \
+	tests/ovn-controller.at \
 	tests/ovn-controller-vtep.at
 
 SYSTEM_KMOD_TESTSUITE_AT = \
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
new file mode 100644
index 0000000..8627e59
--- /dev/null
+++ b/tests/ovn-controller.at
@@ -0,0 +1,75 @@ 
+AT_BANNER([ovn-controller])
+
+AT_SETUP([ovn-controller - ovn-bridge-mappings])
+ovn_init_db ovn-sb
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+    -- add-br br-phys \
+    -- add-br br-eth0 \
+    -- add-br br-eth1 \
+    -- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+# Waits until the OVS database contains exactly the specified patch ports.
+# Each argument should be of the form BRIDGE PORT PEER.
+check_patches () {
+    # Generate code to check that the set of patch ports is exactly as
+    # specified.
+    printf > query 'ovs-vsctl -f csv -d bare --no-headings --columns=name find interface type=patch | sort
+ovs-vsctl init'
+    for patch
+    do
+	echo $patch
+    done | cut -d' ' -f 2 | sort > expout
+
+    # Generate code to verify that the configuration of each patch
+    # port is correct.
+    for patch
+    do
+	set $patch; bridge=$1 port=$2 peer=$3
+        printf >>query ' \
+	    -- iface-to-br %s \
+	    -- get Interface %s type \
+	    -- get Interface %s options' $port $port $port
+        echo >>expout "$bridge
+patch
+{peer=$peer}"
+    done
+
+    # Run the query until we get the expected result (or until a timeout).
+    #
+    # (We use sed to drop all "s from output because ovs-vsctl quotes some
+    # of the port names but not others.)
+    AT_CAPTURE_FILE([query])
+    AT_CAPTURE_FILE([expout])
+    AT_CAPTURE_FILE([stdout])
+    OVS_WAIT_UNTIL([. ./query | sed 's/"//g' > stdout #"
+                    diff -u stdout expout >/dev/null])
+}
+
+# Initially there should be no patch ports.
+check_patches
+
+# Configure two ovn-bridge mappings to create two patch ports.
+AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0,physnet2:br-eth1])
+check_patches \
+    'br-eth0 patch-br-eth0-to-br-int patch-br-int-to-br-eth0' \
+    'br-int  patch-br-int-to-br-eth0 patch-br-eth0-to-br-int' \
+    'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \
+    'br-int  patch-br-int-to-br-eth1 patch-br-eth1-to-br-int'
+
+# Change the mapping and the patch ports should change.
+AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth2,physnet2:br-eth1])
+check_patches \
+    'br-eth2 patch-br-eth2-to-br-int patch-br-int-to-br-eth2' \
+    'br-int  patch-br-int-to-br-eth2 patch-br-eth2-to-br-int' \
+    'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \
+    'br-int  patch-br-int-to-br-eth1 patch-br-eth1-to-br-int'
+
+# Delete the mapping and the patch ports should go away.
+AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids ovn-bridge-mappings])
+check_patches
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index cb2e098..8f915fa 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -70,4 +70,5 @@  m4_include([tests/auto-attach.at])
 m4_include([tests/ovn.at])
 m4_include([tests/ovn-nbctl.at])
 m4_include([tests/ovn-sbctl.at])
+m4_include([tests/ovn-controller.at])
 m4_include([tests/ovn-controller-vtep.at])