diff mbox series

[ovs-dev] tests: Really fix requested-chassis localport test

Message ID 20211108093803.91025-1-frode.nordahl@canonical.com
State Accepted
Headers show
Series [ovs-dev] tests: Really fix requested-chassis localport test | expand

Checks

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 fail github build: failed

Commit Message

Frode Nordahl Nov. 8, 2021, 9:38 a.m. UTC 
The first test case of adding localport without a
requested-chassis option is invalid as this would make the
chassis do claim/release thrashing leading to a unpredictable
test results.

Create the localport in NB DB prior to creating the ports on the
chassis.  The step by step ovn-nbctl CLI creation of the localport
would in reality create a VIF which is then changed to be a
localport, this does not represent how the CMS would create the
localport.

Create a second port representing a VM to ensure the localport
flows are generated.

Use OVS_WAIT_UNTIL instead of AT_CHECK, which should avoid race
conditions where the check is ran before the flow tables are
populated.

Print output so that it is easier to investigate in the event of a
failure.

Fixes: ad77db239d9a ("controller: Make use of Port_Binding:requested_chassis.")
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 tests/ovn.at | 66 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 22 deletions(-)

Comments

Dumitru Ceara Nov. 26, 2021, 12:51 p.m. UTC | #1 
On 11/8/21 10:38, Frode Nordahl wrote:
> The first test case of adding localport without a
> requested-chassis option is invalid as this would make the
> chassis do claim/release thrashing leading to a unpredictable
> test results.
> 
> Create the localport in NB DB prior to creating the ports on the
> chassis.  The step by step ovn-nbctl CLI creation of the localport
> would in reality create a VIF which is then changed to be a
> localport, this does not represent how the CMS would create the
> localport.
> 
> Create a second port representing a VM to ensure the localport
> flows are generated.
> 
> Use OVS_WAIT_UNTIL instead of AT_CHECK, which should avoid race
> conditions where the check is ran before the flow tables are
> populated.
> 
> Print output so that it is easier to investigate in the event of a
> failure.
> 
> Fixes: ad77db239d9a ("controller: Make use of Port_Binding:requested_chassis.")
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> ---

Hi Frode,

I confirm this fixes the flaky test (or at least I haven't seen another
failure with your patch applied while without I see them every now and
then).

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru
Numan Siddique Nov. 29, 2021, 6:25 p.m. UTC | #2 
On Fri, Nov 26, 2021 at 7:52 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 11/8/21 10:38, Frode Nordahl wrote:
> > The first test case of adding localport without a
> > requested-chassis option is invalid as this would make the
> > chassis do claim/release thrashing leading to a unpredictable
> > test results.
> >
> > Create the localport in NB DB prior to creating the ports on the
> > chassis.  The step by step ovn-nbctl CLI creation of the localport
> > would in reality create a VIF which is then changed to be a
> > localport, this does not represent how the CMS would create the
> > localport.
> >
> > Create a second port representing a VM to ensure the localport
> > flows are generated.
> >
> > Use OVS_WAIT_UNTIL instead of AT_CHECK, which should avoid race
> > conditions where the check is ran before the flow tables are
> > populated.
> >
> > Print output so that it is easier to investigate in the event of a
> > failure.
> >
> > Fixes: ad77db239d9a ("controller: Make use of Port_Binding:requested_chassis.")
> > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> > ---
>
> Hi Frode,
>
> I confirm this fixes the flaky test (or at least I haven't seen another
> failure with your patch applied while without I see them every now and
> then).
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks.  Applied.

Numan

>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/tests/ovn.at b/tests/ovn.at
index 6f6f5c2da..fd52ee752 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -28726,40 +28726,62 @@  ovn_start
 
 net_add n1
 
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0-localport
+check ovn-nbctl lsp-set-addresses sw0-localport "50:54:00:00:00:02 10.0.0.2"
+check ovn-nbctl lsp-set-type sw0-localport localport
+check ovn-nbctl lsp-set-options sw0-localport requested-chassis=""
+
+check ovn-nbctl lsp-add sw0 sw0-vm1
+check ovn-nbctl lsp-set-addresses sw0-vm1 "50:54:00:00:00:03 10.0.0.3"
+check ovn-nbctl lsp-set-options sw0-vm1 requested-chassis="hv1"
+
+check ovn-nbctl lsp-add sw0 sw0-vm2
+check ovn-nbctl lsp-set-addresses sw0-vm2 "50:54:00:00:00:04 10.0.0.4"
+check ovn-nbctl lsp-set-options sw0-vm2 requested-chassis="hv2"
+
 sim_add hv1
 as hv1
 ovs-vsctl add-br br-phys
 ovn_attach n1 br-phys 192.168.0.1
-ovs-vsctl add-port br-int lport \
-    -- set interface lport external_ids:iface-id=sw0-lport ofport-request=13
+ovs-vsctl add-port br-int localport \
+    -- set interface localport \
+       external_ids:iface-id=sw0-localport \
+       ofport-request=13
+ovs-vsctl add-port br-int vm1 \
+    -- set interface vm1 external_ids:iface-id=sw0-vm1
 
 sim_add hv2
 as hv2
 ovs-vsctl add-br br-phys
 ovn_attach n1 br-phys 192.168.0.2
-ovs-vsctl add-port br-int lport \
-    -- set interface lport external_ids:iface-id=sw0-lport ofport-request=13
-
-check ovn-nbctl ls-add sw0
-check ovn-nbctl lsp-add sw0 sw0-lport
-check ovn-nbctl lsp-set-addresses sw0-lport "50:54:00:00:00:01 10.0.0.2"
-check ovn-nbctl lsp-set-type sw0-lport localport
+ovs-vsctl add-port br-int localport \
+    -- set interface localport \
+       external_ids:iface-id=sw0-localport \
+       ofport-request=13
+ovs-vsctl add-port br-int vm2 \
+    -- set interface vm2 external_ids:iface-id=sw0-vm2
 
-# First confirm operation without requested-chassis
-check_row_count Port_Binding 1 logical_port=sw0-lport 'up=true'
+# Only the VIFs will get status UP
+wait_for_ports_up sw0-vm1 sw0-vm2
 
-AT_CHECK([as hv1 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [0], [])
-AT_CHECK([as hv2 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [0], [])
-
-# Set requested-chassis to one of the HVs
-check ovn-nbctl --wait=hv lsp-set-options sw0-lport requested-chassis="hv1"
-AT_CHECK([as hv1 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [0], [])
-AT_CHECK([as hv2 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [1], [])
+# Check that flows are generated for the localport
+OVS_WAIT_UNTIL([
+    as hv1 \
+    ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 \
+    | tee 1-hv1-br-int-table-0.txt \
+    && grep -q in_port=13 1-hv1-br-int-table-0.txt
+])
+OVS_WAIT_UNTIL([
+    as hv2 \
+    ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 \
+    | tee 1-hv2-br-int-table-0.txt \
+    && grep in_port=13 1-hv2-br-int-table-0.txt
+])
 
-# Set requested-chassis to empty string
-check ovn-nbctl --wait=hv lsp-set-options sw0-lport requested-chassis=""
-AT_CHECK([as hv1 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [0], [])
-AT_CHECK([as hv2 ovs-ofctl -O OpenFlow15 dump-flows br-int table=0 | grep -q in_port=13], [0], [])
+# Confirm that the controllers did not attempt to claim the localport
+AT_CHECK([grep -q "Claiming lport sw0-localport" hv1/ovn-controller.log], [1], [])
+AT_CHECK([grep -q "Claiming lport sw0-localport" hv2/ovn-controller.log], [1], [])
 
 OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP