diff mbox

[ovs-dev,V2] tests: Add force/commit test to system-traffic.at

Message ID 1500392574-4462-1-git-send-email-gvrose8192@gmail.com
State Accepted
Delegated to: Joe Stringer
Headers show

Commit Message

Gregory Rose July 18, 2017, 3:42 p.m. UTC 
From: Joe Stringer <joe@ovn.org>

Add a new test check if the conntrack force direction change and
commit is working correctly.

This test was used to find and root cause BZ 18090854.

Signed-off-by: Joe Stringer <joe@ovn.org>
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
V2 - Fix commit message text
---
 tests/system-traffic.at | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Darrell Ball July 18, 2017, 5:16 p.m. UTC | #1 
On 7/18/17, 8:42 AM, "ovs-dev-bounces@openvswitch.org on behalf of Greg Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com> wrote:

    From: Joe Stringer <joe@ovn.org>

    
    Add a new test check if the conntrack force direction change and
    commit is working correctly.

You are adding a new check to an existing test (‘force commit’).
The existing test already checked for conntrack force direction change.

Did you intend to say just something like:

    “Update test to do additional conntrack force direction
    change checking to help find and root cause BZ 18090854.”

   You could go into more detail, but I am not sure it will help much.
   The difference with the additional check is a ct lookup has not occurred
   in the kernel at a point in time when it is checked.

Otherwise
Acked-by: Darrell Ball <dlu998@gmail.com>

    
    This test was used to find and root cause BZ 18090854.
    
    Signed-off-by: Joe Stringer <joe@ovn.org>

    Signed-off-by: Greg Rose <gvrose8192@gmail.com>

    ---
    V2 - Fix commit message text
    ---
     tests/system-traffic.at | 12 +++++++++++-
     1 file changed, 11 insertions(+), 1 deletion(-)
    
    diff --git a/tests/system-traffic.at b/tests/system-traffic.at
    index b2393f5..e83ad3a 100644
    --- a/tests/system-traffic.at
    +++ b/tests/system-traffic.at
    @@ -789,7 +789,7 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
     AT_DATA([flows.txt], [dnl
     priority=1,action=drop
     priority=10,arp,action=normal
    -priority=100,in_port=1,udp,action=ct(commit),controller
    +priority=100,in_port=1,udp,action=ct(force,commit),controller
     priority=100,in_port=2,ct_state=-trk,udp,action=ct(table=0)
     priority=100,in_port=2,ct_state=+trk+est,udp,action=ct(force,commit,table=1)
     table=1,in_port=2,ct_state=+trk,udp,action=controller
    @@ -826,6 +826,16 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], [],
     udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
     ])
     
    +ovs-appctl vlog/set dpif:dbg
    +
    +dnl OK, now send another packet from port 1 and see that it switches again
    +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"])
    +AT_CHECK([ovs-appctl revalidator/purge], [0])
    +
    +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
    +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
    +])
    +
     OVS_TRAFFIC_VSWITCHD_STOP
     AT_CLEANUP
     
    -- 
    1.8.3.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=tedsV7DMCGMLsGt5DgMZSp0DJjH_LgVBvzOAs09HYvg&s=dF5udUdvutEhdWLoOSCja3QHUMSbTeJ4_iB6hR5W8zw&e=
Gregory Rose July 18, 2017, 8:08 p.m. UTC | #2 
On 07/18/2017 10:16 AM, Darrell Ball wrote:
> 
> 
> On 7/18/17, 8:42 AM, "ovs-dev-bounces@openvswitch.org on behalf of Greg Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com> wrote:
> 
>      From: Joe Stringer <joe@ovn.org>
>      
>      Add a new test check if the conntrack force direction change and
>      commit is working correctly.
> 
> You are adding a new check to an existing test (‘force commit’).
> The existing test already checked for conntrack force direction change.

Oh.  OK, I just went from this change:

  +priority=100,in_port=1,udp,action=ct(force,commit),controller

And that looks like a new test for force and commit to me but
obviously I don't know the entire context.

Joe originated this patch and asked me to send it along with my
other bug fix commit.  Clearly I need some help on what this
patch is actually doing.  Let me see what he thinks.

Thanks,

- Greg

> 
> Did you intend to say just something like:
> 
>      “Update test to do additional conntrack force direction
>      change checking to help find and root cause BZ 18090854.”
> 
>     You could go into more detail, but I am not sure it will help much.
>     The difference with the additional check is a ct lookup has not occurred
>     in the kernel at a point in time when it is checked.
> 
> Otherwise
> Acked-by: Darrell Ball <dlu998@gmail.com>
>      
>      This test was used to find and root cause BZ 18090854.
>      
>      Signed-off-by: Joe Stringer <joe@ovn.org>
>      Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>      ---
>      V2 - Fix commit message text
>      ---
>       tests/system-traffic.at | 12 +++++++++++-
>       1 file changed, 11 insertions(+), 1 deletion(-)
>      
>      diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>      index b2393f5..e83ad3a 100644
>      --- a/tests/system-traffic.at
>      +++ b/tests/system-traffic.at
>      @@ -789,7 +789,7 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>       AT_DATA([flows.txt], [dnl
>       priority=1,action=drop
>       priority=10,arp,action=normal
>      -priority=100,in_port=1,udp,action=ct(commit),controller
>      +priority=100,in_port=1,udp,action=ct(force,commit),controller
>       priority=100,in_port=2,ct_state=-trk,udp,action=ct(table=0)
>       priority=100,in_port=2,ct_state=+trk+est,udp,action=ct(force,commit,table=1)
>       table=1,in_port=2,ct_state=+trk,udp,action=controller
>      @@ -826,6 +826,16 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], [],
>       udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
>       ])
>       
>      +ovs-appctl vlog/set dpif:dbg
>      +
>      +dnl OK, now send another packet from port 1 and see that it switches again
>      +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"])
>      +AT_CHECK([ovs-appctl revalidator/purge], [0])
>      +
>      +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
>      +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
>      +])
>      +
>       OVS_TRAFFIC_VSWITCHD_STOP
>       AT_CLEANUP
>       
>      --
>      1.8.3.1
>      
>      _______________________________________________
>      dev mailing list
>      dev@openvswitch.org
>      https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=tedsV7DMCGMLsGt5DgMZSp0DJjH_LgVBvzOAs09HYvg&s=dF5udUdvutEhdWLoOSCja3QHUMSbTeJ4_iB6hR5W8zw&e=
>      
> 
> 
>
Joe Stringer July 18, 2017, 8:23 p.m. UTC | #3 
On 18 July 2017 at 13:08, Greg Rose <gvrose8192@gmail.com> wrote:
> On 07/18/2017 10:16 AM, Darrell Ball wrote:
>>
>>
>>
>> On 7/18/17, 8:42 AM, "ovs-dev-bounces@openvswitch.org on behalf of Greg
>> Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com>
>> wrote:
>>
>>      From: Joe Stringer <joe@ovn.org>
>>           Add a new test check if the conntrack force direction change and
>>      commit is working correctly.
>>
>> You are adding a new check to an existing test (‘force commit’).
>> The existing test already checked for conntrack force direction change.
>
>
> Oh.  OK, I just went from this change:
>
>  +priority=100,in_port=1,udp,action=ct(force,commit),controller
>
> And that looks like a new test for force and commit to me but
> obviously I don't know the entire context.
>
> Joe originated this patch and asked me to send it along with my
> other bug fix commit.  Clearly I need some help on what this
> patch is actually doing.  Let me see what he thinks.

In autotest parlance, the part between AT_SETUP and AT_CLEANUP is a
test, so you updated the test (which, yes, tests something slightly
more).

Don't worry about re-sending though, I can fix this up when I apply this.
Gregory Rose July 18, 2017, 8:41 p.m. UTC | #4 
On 07/18/2017 01:23 PM, Joe Stringer wrote:
> On 18 July 2017 at 13:08, Greg Rose <gvrose8192@gmail.com> wrote:
> > On 07/18/2017 10:16 AM, Darrell Ball wrote:
> >>
> >>
> >>
> >> On 7/18/17, 8:42 AM, "ovs-dev-bounces@openvswitch.org on behalf of Greg
> >> Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com>
> >> wrote:
> >>
> >>       From: Joe Stringer <joe@ovn.org>
> >>            Add a new test check if the conntrack force direction change and
> >>       commit is working correctly.
> >>
> >> You are adding a new check to an existing test (‘force commit’).
> >> The existing test already checked for conntrack force direction change.
> >
> >
> > Oh.  OK, I just went from this change:
> >
> >   +priority=100,in_port=1,udp,action=ct(force,commit),controller
> >
> > And that looks like a new test for force and commit to me but
> > obviously I don't know the entire context.
> >
> > Joe originated this patch and asked me to send it along with my
> > other bug fix commit.  Clearly I need some help on what this
> > patch is actually doing.  Let me see what he thinks.
>
> In autotest parlance, the part between AT_SETUP and AT_CLEANUP is a
> test, so you updated the test (which, yes, tests something slightly
> more).
>
> Don't worry about re-sending though, I can fix this up when I apply this.
>
Thank you!

- Greg
Joe Stringer July 24, 2017, 9:15 p.m. UTC | #5 
On 18 July 2017 at 13:41, Greg Rose <gvrose8192@gmail.com> wrote:
> On 07/18/2017 01:23 PM, Joe Stringer wrote:
>>
>> On 18 July 2017 at 13:08, Greg Rose <gvrose8192@gmail.com> wrote:
>> > On 07/18/2017 10:16 AM, Darrell Ball wrote:
>> >>
>> >>
>> >>
>> >> On 7/18/17, 8:42 AM, "ovs-dev-bounces@openvswitch.org on behalf of Greg
>> >> Rose" <ovs-dev-bounces@openvswitch.org on behalf of
>> >> gvrose8192@gmail.com>
>> >> wrote:
>> >>
>> >>       From: Joe Stringer <joe@ovn.org>
>> >>            Add a new test check if the conntrack force direction change
>> >> and
>> >>       commit is working correctly.
>> >>
>> >> You are adding a new check to an existing test (‘force commit’).
>> >> The existing test already checked for conntrack force direction change.
>> >
>> >
>> > Oh.  OK, I just went from this change:
>> >
>> >   +priority=100,in_port=1,udp,action=ct(force,commit),controller
>> >
>> > And that looks like a new test for force and commit to me but
>> > obviously I don't know the entire context.
>> >
>> > Joe originated this patch and asked me to send it along with my
>> > other bug fix commit.  Clearly I need some help on what this
>> > patch is actually doing.  Let me see what he thinks.
>>
>> In autotest parlance, the part between AT_SETUP and AT_CLEANUP is a
>> test, so you updated the test (which, yes, tests something slightly
>> more).
>>
>> Don't worry about re-sending though, I can fix this up when I apply this.
>>
> Thank you!

Thanks, I applied this patch to master.
diff mbox

Patch

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b2393f5..e83ad3a 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -789,7 +789,7 @@  ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
 AT_DATA([flows.txt], [dnl
 priority=1,action=drop
 priority=10,arp,action=normal
-priority=100,in_port=1,udp,action=ct(commit),controller
+priority=100,in_port=1,udp,action=ct(force,commit),controller
 priority=100,in_port=2,ct_state=-trk,udp,action=ct(table=0)
 priority=100,in_port=2,ct_state=+trk+est,udp,action=ct(force,commit,table=1)
 table=1,in_port=2,ct_state=+trk,udp,action=controller
@@ -826,6 +826,16 @@  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], [],
 udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
 ])
 
+ovs-appctl vlog/set dpif:dbg
+
+dnl OK, now send another packet from port 1 and see that it switches again
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"])
+AT_CHECK([ovs-appctl revalidator/purge], [0])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
+udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
+])
+
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP