[PATCHv2 bpf-next] selftests/bpf: run mptcp in a dedicated netns

[PATCHv2 bpf-next] selftests/bpf: run mptcp in a dedicated netns

[Hangbin Liu]

The current mptcp test is run in init netns. If the user or default
system config disabled mptcp, the test will fail. Let's run the mptcp
test in a dedicated netns to avoid none kernel default mptcp setting.

Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: remove unneed close_cgroup_fd goto label.
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 27 +++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 59f08d6d1d53..dbe2bcfd3b38 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -7,6 +7,16 @@
 #include "network_helpers.h"
 #include "mptcp_sock.skel.h"
 
+#define SYS(fmt, ...)						\
+	({							\
+		char cmd[1024];					\
+		snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
+		if (!ASSERT_OK(system(cmd), cmd))		\
+			goto fail;				\
+	})
+
+#define NS_TEST "mptcp_ns"
+
 #ifndef TCP_CA_NAME_MAX
 #define TCP_CA_NAME_MAX	16
 #endif
@@ -138,12 +148,20 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 
 static void test_base(void)
 {
+	struct nstoken *nstoken = NULL;
 	int server_fd, cgroup_fd;
 
 	cgroup_fd = test__join_cgroup("/mptcp");
 	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
 		return;
 
+	SYS("ip netns add %s", NS_TEST);
+	SYS("ip -net %s link set dev lo up", NS_TEST);
+
+	nstoken = open_netns(NS_TEST);
+	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
+		goto fail;
+
 	/* without MPTCP */
 	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
 	if (!ASSERT_GE(server_fd, 0, "start_server"))
@@ -157,13 +175,18 @@ static void test_base(void)
 	/* with MPTCP */
 	server_fd = start_mptcp_server(AF_INET, NULL, 0, 0);
 	if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
-		goto close_cgroup_fd;
+		goto fail;
 
 	ASSERT_OK(run_test(cgroup_fd, server_fd, true), "run_test mptcp");
 
 	close(server_fd);
 
-close_cgroup_fd:
+fail:
+	if (nstoken)
+		close_netns(nstoken);
+
+	system("ip netns del " NS_TEST " >& /dev/null");
+
 	close(cgroup_fd);
 }
 
-- 
2.38.1

Re: selftests/bpf: run mptcp in a dedicated netns: Tests Results

[MPTCP CI]

Hi Hangbin,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6016037968150528
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6016037968150528/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6578987921571840
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6578987921571840/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4749400572952576
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4749400572952576/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5453088014729216
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5453088014729216/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/897f90e8a312


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCHv2 bpf-next] selftests/bpf: run mptcp in a dedicated netns

[Matthieu Baerts]

Hi Hangbin,

On 19/02/2023 08:01, Hangbin Liu wrote:
> The current mptcp test is run in init netns. If the user or default
> system config disabled mptcp, the test will fail. Let's run the mptcp
> test in a dedicated netns to avoid none kernel default mptcp setting.
> 
> Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v2: remove unneed close_cgroup_fd goto label.

Thank you for the update!

Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: selftests/bpf: run mptcp in a dedicated netns: Tests Results

[MPTCP CI]

Hi Hangbin,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5430059792596992
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5430059792596992/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6211361403830272
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6211361403830272/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5852272257662976
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5852272257662976/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4726372350820352
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4726372350820352/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f0fe5983fd4a


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCHv2 bpf-next] selftests/bpf: run mptcp in a dedicated netns

[Martin KaFai Lau]

On 2/18/23 11:01 PM, Hangbin Liu wrote:
> The current mptcp test is run in init netns. If the user or default
> system config disabled mptcp, the test will fail. Let's run the mptcp
> test in a dedicated netns to avoid none kernel default mptcp setting.
> 
> Suggested-by: Martin KaFai Lau <martin.lau@linux.dev>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v2: remove unneed close_cgroup_fd goto label.
> ---
>   .../testing/selftests/bpf/prog_tests/mptcp.c  | 27 +++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 59f08d6d1d53..dbe2bcfd3b38 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -7,6 +7,16 @@
>   #include "network_helpers.h"
>   #include "mptcp_sock.skel.h"
>   
> +#define SYS(fmt, ...)						\
> +	({							\
> +		char cmd[1024];					\
> +		snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
> +		if (!ASSERT_OK(system(cmd), cmd))		\
> +			goto fail;				\
> +	})
> +
> +#define NS_TEST "mptcp_ns"
> +
>   #ifndef TCP_CA_NAME_MAX
>   #define TCP_CA_NAME_MAX	16
>   #endif
> @@ -138,12 +148,20 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
>   
>   static void test_base(void)
>   {
> +	struct nstoken *nstoken = NULL;
>   	int server_fd, cgroup_fd;
>   
>   	cgroup_fd = test__join_cgroup("/mptcp");
>   	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
>   		return;
>   
> +	SYS("ip netns add %s", NS_TEST);
> +	SYS("ip -net %s link set dev lo up", NS_TEST);
> +
> +	nstoken = open_netns(NS_TEST);
> +	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
> +		goto fail;
> +
>   	/* without MPTCP */
>   	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
>   	if (!ASSERT_GE(server_fd, 0, "start_server"))
> @@ -157,13 +175,18 @@ static void test_base(void)
>   	/* with MPTCP */
>   	server_fd = start_mptcp_server(AF_INET, NULL, 0, 0);
>   	if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
> -		goto close_cgroup_fd;
> +		goto fail;
>   
>   	ASSERT_OK(run_test(cgroup_fd, server_fd, true), "run_test mptcp");
>   
>   	close(server_fd);
>   
> -close_cgroup_fd:
> +fail:
> +	if (nstoken)
> +		close_netns(nstoken);
> +
> +	system("ip netns del " NS_TEST " >& /dev/null");

It needs to be "&>", like the fix in commit 98e13848cf43 ("selftests/bpf: Fix 
decap_sanity_ns cleanup").

Since it needs to respin, could you help and take this chance to put the above 
SYS() macro into the test_progs.h. Other selftests are doing similar thing also. 
If possible, it may be easier to have a configurable "goto_label" as the first arg.

Re: [PATCHv2 bpf-next] selftests/bpf: run mptcp in a dedicated netns

[Hangbin Liu]

On Wed, Feb 22, 2023 at 03:44:17PM -0800, Martin KaFai Lau wrote:
> > +	system("ip netns del " NS_TEST " >& /dev/null");
> 
> It needs to be "&>", like the fix in commit 98e13848cf43 ("selftests/bpf:
> Fix decap_sanity_ns cleanup").

:Shame, Didn't notice this when do copy/paste...

> 
> Since it needs to respin, could you help and take this chance to put the
> above SYS() macro into the test_progs.h. Other selftests are doing similar
> thing also. If possible, it may be easier to have a configurable
> "goto_label" as the first arg.

OK, I will fix it.

Thanks
Hangbin