[PATCH net] tc-testing: fix geneve options match in tunnel_key unit tests

[PATCH net] tc-testing: fix geneve options match in tunnel_key unit tests

[Hangbin Liu]

tc action print "geneve_opts" instead of "geneve_opt".
Fix the typo, or we will unable to match correct action output.

Fixes: cba54f9cf4ec ("tc-testing: add geneve options in tunnel_key unit tests")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../tc-testing/tc-tests/actions/tunnel_key.json    | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
index fbeb9197697d..f451915626eb 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
@@ -629,7 +629,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022 index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:80:00880022.*index 1",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -653,7 +653,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344 index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344.*index 1",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -677,7 +677,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 824212:80:00880022 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 824212:80:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 824212:80:00880022.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -701,7 +701,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:4224:00880022 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:4224:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:4224:00880022.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -725,7 +725,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:4288 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:4288.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:80:4288.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -749,7 +749,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:4288428822 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:4288428822.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:80:4288428822.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -773,7 +773,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022,0408:42: index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022,0408:42:.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opts 0102:80:00880022,0408:42:.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
-- 
2.25.4

Re: [PATCH net] tc-testing: fix geneve options match in tunnel_key unit tests

[Davide Caratti]

On Thu, 2020-06-18 at 16:37 +0800, Hangbin Liu wrote:
> tc action print "geneve_opts" instead of "geneve_opt".
> Fix the typo, or we will unable to match correct action output.
> 

hello Hangbin,

> Fixes: cba54f9cf4ec ("tc-testing: add geneve options in tunnel_key unit tests")

this Fixes: tag is suspicious, when a tdc test is added I would expect to
see it passing. If I well read the code, the problem has been introduced
in iproute2, with commit 

commit f72c3ad00f3b7869e90840d0098a83cb88224892
Author: Xin Long <lucien.xin@gmail.com>
Date:   Mon Apr 27 18:27:48 2020 +0800

    tc: m_tunnel_key: add options support for vxlan
    

that did:

[...]

static void tunnel_key_print_geneve_options(const char *name,
-                                           struct rtattr *attr)
+static void tunnel_key_print_geneve_options(struct rtattr *attr)
 {
        struct rtattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
        struct rtattr *i = RTA_DATA(attr);
        int ii, data_len = 0, offset = 0;
        int rem = RTA_PAYLOAD(attr);
+       char *name = "geneve_opts";
        char strbuf[rem * 2 + 1];
        char data[rem * 2 + 1];
        uint8_t data_r[rem];
@@ -421,7 +464,7 @@ static void tunnel_key_print_geneve_options(const char *name,
 
        open_json_array(PRINT_JSON, name);
        print_nl();
-       print_string(PRINT_FP, name, "\t%s ", "geneve_opt");
+       print_string(PRINT_FP, name, "\t%s ", name);
 

(just speculating, because I didn't try older versions of iproute2). But if my
hypothesis is correct, then the fix should be done in iproute2, WDYT?

thanks,
-- 
davide

Re: [PATCH net] tc-testing: fix geneve options match in tunnel_key unit tests

[Hangbin Liu]

On Thu, Jun 18, 2020 at 10:53:51AM +0200, Davide Caratti wrote:
> On Thu, 2020-06-18 at 16:37 +0800, Hangbin Liu wrote:
> > tc action print "geneve_opts" instead of "geneve_opt".
> > Fix the typo, or we will unable to match correct action output.
> > 
> 
> hello Hangbin,
> 
> > Fixes: cba54f9cf4ec ("tc-testing: add geneve options in tunnel_key unit tests")
> 
> this Fixes: tag is suspicious, when a tdc test is added I would expect to
> see it passing. If I well read the code, the problem has been introduced
> in iproute2, with commit 
> 
> commit f72c3ad00f3b7869e90840d0098a83cb88224892
> Author: Xin Long <lucien.xin@gmail.com>
> Date:   Mon Apr 27 18:27:48 2020 +0800
> 
>     tc: m_tunnel_key: add options support for vxlan
>     
> 
> that did:
> 
> [...]
> 
> static void tunnel_key_print_geneve_options(const char *name,
> -                                           struct rtattr *attr)
> +static void tunnel_key_print_geneve_options(struct rtattr *attr)
>  {
>         struct rtattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
>         struct rtattr *i = RTA_DATA(attr);
>         int ii, data_len = 0, offset = 0;
>         int rem = RTA_PAYLOAD(attr);
> +       char *name = "geneve_opts";
>         char strbuf[rem * 2 + 1];
>         char data[rem * 2 + 1];
>         uint8_t data_r[rem];
> @@ -421,7 +464,7 @@ static void tunnel_key_print_geneve_options(const char *name,
>  
>         open_json_array(PRINT_JSON, name);
>         print_nl();
> -       print_string(PRINT_FP, name, "\t%s ", "geneve_opt");
> +       print_string(PRINT_FP, name, "\t%s ", name);

Ah, yes, you are right.
>  
> 
> (just speculating, because I didn't try older versions of iproute2). But if my
> hypothesis is correct, then the fix should be done in iproute2, WDYT?

If you and Stephen are both agree. I'm OK to fix it on iproute2.

Thanks
Hangbin

[PATCHv2 net] tc-testing: update geneve options match in tunnel_key unit tests

[Hangbin Liu]

Since iproute2 commit f72c3ad00f3b ("tc: m_tunnel_key: add options
support for vxlan"), the geneve opt output use key word "geneve_opts"
instead of "geneve_opt". To make compatibility for both old and new
iproute2, let's accept both "geneve_opt" and "geneve_opts".

Suggested-by: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../tc-testing/tc-tests/actions/tunnel_key.json    | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
index fbeb9197697d..7357c58fa2dc 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
@@ -629,7 +629,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022 index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:80:00880022.*index 1",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -653,7 +653,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344 index 1",
         "expExitCode": "0",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:80:00880022,0408:42:0040007611223344,0111:02:1020304011223344.*index 1",
         "matchCount": "1",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -677,7 +677,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 824212:80:00880022 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 824212:80:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 824212:80:00880022.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -701,7 +701,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:4224:00880022 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:4224:00880022.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:4224:00880022.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -725,7 +725,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:4288 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:4288.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:80:4288.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -749,7 +749,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:4288428822 index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:4288428822.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:80:4288428822.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
@@ -773,7 +773,7 @@
         "cmdUnderTest": "$TC actions add action tunnel_key set src_ip 1.1.1.1 dst_ip 2.2.2.2 id 42 dst_port 6081 geneve_opts 0102:80:00880022,0408:42: index 1",
         "expExitCode": "255",
         "verifyCmd": "$TC actions get action tunnel_key index 1",
-        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt 0102:80:00880022,0408:42:.*index 1",
+        "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 1.1.1.1.*dst_ip 2.2.2.2.*key_id 42.*dst_port 6081.*geneve_opt[s]? 0102:80:00880022,0408:42:.*index 1",
         "matchCount": "0",
         "teardown": [
             "$TC actions flush action tunnel_key"
-- 
2.25.4

Re: [PATCHv2 net] tc-testing: update geneve options match in tunnel_key unit tests

[Simon Horman]

On Fri, Jun 19, 2020 at 11:24:45AM +0800, Hangbin Liu wrote:
> Since iproute2 commit f72c3ad00f3b ("tc: m_tunnel_key: add options
> support for vxlan"), the geneve opt output use key word "geneve_opts"
> instead of "geneve_opt". To make compatibility for both old and new
> iproute2, let's accept both "geneve_opt" and "geneve_opts".
> 
> Suggested-by: Davide Caratti <dcaratti@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

Re: [PATCHv2 net] tc-testing: update geneve options match in tunnel_key unit tests

[Davide Caratti]

thanks for the patch,

On Fri, 2020-06-19 at 11:24 +0800, Hangbin Liu wrote:
> Since iproute2 commit f72c3ad00f3b ("tc: m_tunnel_key: add options
> support for vxlan"), the geneve opt output use key word "geneve_opts"
> instead of "geneve_opt". To make compatibility for both old and new
> iproute2, let's accept both "geneve_opt" and "geneve_opts".
> 
> Suggested-by: Davide Caratti <dcaratti@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  .../tc-testing/tc-tests/actions/tunnel_key.json    | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

it has been successfully verified with the following command:

# for t in 4f20 e33d 0778 4ae8 4039 26a6 f44d ; do ./tdc.py -e $t ; done

using two tc binaries, one compiled from f72c3ad00f3b^ and the other one
from HEAD. I would have liked to see a list of OK with this command:

# ./tdc.cy -c tunnel_key

but (at least on my test setup) I see a failure here:

# ./tdc.py  -e 364d
Test 364d: Replace tunnel_key set action with all parameters and cookie

-----> prepare stage *** Could not execute: "$TC actions add action tunnel_key set src_ip 10.10.10.1 dst_ip 20.20.20.2 dst_port 3128 nocsum id 1 index 1 cooki
e aabbccddeeff112233445566778800a"

-----> prepare stage *** Error message: "Error: argument "aabbccddeeff112233445566778800a" is wrong: cookie must be a hex string

"                                      
returncode 255; expected [0]

-----> prepare stage *** Aborting test run.


<_io.BufferedReader name=3> *** stdout ***


<_io.BufferedReader name=5> *** stderr ***
"-----> prepare stage" did not complete successfully
Exception <class '__main__.PluginMgrTestFail'> ('setup', None, '"-----> prepare stage" did not complete successfully') (caught in test_runner, running test 2 
364d Replace tunnel_key set action with all parameters and cookie stage setup)
---------------                        
traceback                              
  File "./tdc.py", line 371, in test_runner
    res = run_one_test(pm, args, index, tidx)
  File "./tdc.py", line 272, in run_one_test
    prepare_env(args, pm, 'setup', "-----> prepare stage", tidx["setup"])
  File "./tdc.py", line 245, in prepare_env
    raise PluginMgrTestFail(
---------------                        
---------------                        

All test results:                      

1..1                                   
ok 1 364d - Replace tunnel_key set action with all parameters and cookie # skipped - "-----> prepare stage" did not complete successfully

I checked twice, and the problem only happens with the new iproute2 binary.
So, there seems to be an additional tdc breakage in the latest iproute2.
But the problem looks unrelated to your patch, and I follow-up in the next hours.

Tested-by: Davide Caratti <dcaratti@redhat.com>

Re: [PATCHv2 net] tc-testing: update geneve options match in tunnel_key unit tests

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Fri, 19 Jun 2020 11:24:45 +0800

> Since iproute2 commit f72c3ad00f3b ("tc: m_tunnel_key: add options
> support for vxlan"), the geneve opt output use key word "geneve_opts"
> instead of "geneve_opt". To make compatibility for both old and new
> iproute2, let's accept both "geneve_opt" and "geneve_opts".
> 
> Suggested-by: Davide Caratti <dcaratti@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied, thanks.