Re: [PATCH bpf-next 07/10] bpf: selftests: Restore netns after each test

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jun 26, 2020 at 5:23 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Jun 26, 2020 at 03:45:04PM -0700, Andrii Nakryiko wrote:
> > On Fri, Jun 26, 2020 at 10:56 AM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > It is common for networking tests creating its netns and making its own
> > > setting under this new netns (e.g. changing tcp sysctl).  If the test
> > > forgot to restore to the original netns, it would affect the
> > > result of other tests.
> > >
> > > This patch saves the original netns at the beginning and then restores it
> > > after every test.  Since the restore "setns()" is not expensive, it does it
> > > on all tests without tracking if a test has created a new netns or not.
> > >
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_progs.c | 21 +++++++++++++++++++++
> > >  tools/testing/selftests/bpf/test_progs.h |  2 ++
> > >  2 files changed, 23 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index 54fa5fa688ce..b521ce366381 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -121,6 +121,24 @@ static void reset_affinity() {
> > >         }
> > >  }
> > >
> > > +static void save_netns(void)
> > > +{
> > > +       env.saved_netns_fd = open("/proc/self/ns/net", O_RDONLY);
> > > +       if (env.saved_netns_fd == -1) {
> > > +               perror("open(/proc/self/ns/net)");
> > > +               exit(-1);
> > > +       }
> > > +}
> > > +
> > > +static void restore_netns(void)
> > > +{
> > > +       if (setns(env.saved_netns_fd, CLONE_NEWNET) == -1) {
> > > +               stdio_restore();
> > > +               perror("setns(CLONE_NEWNS)");
> > > +               exit(-1);
> > > +       }
> > > +}
> > > +
> > >  void test__end_subtest()
> > >  {
> > >         struct prog_test_def *test = env.test;
> > > @@ -643,6 +661,7 @@ int main(int argc, char **argv)
> > >                 return -1;
> > >         }
> > >
> > > +       save_netns();
> >
> > you should probably do this also after each sub-test in test__end_subtest()?
> You mean restore_netns()?

oops, yeah :)

>
> It is a tough call.
> Some tests may only want to create a netns at the beginning for all the subtests
> to use (e.g. sk_assign.c).  restore_netns() after each subtest may catch
> tester in surprise that the netns is not in its full control while its
> own test is running.

Wouldn't it be better to update such self-tests to setns on each
sub-test properly? It should be a simple code re-use exercise, unless
I'm missing some other implications of having to do it before each
sub-test?

The idea behind sub-test is (at least it was so far) that it's
independent from other sub-tests and tests, and it's only co-located
with other sub-tests for the purpose of code reuse and logical
grouping. Which is why we reset CPU affinity, for instance.

>
> I think an individual test should have managed the netns properly within its
> subtests already if it wants a correct test result.  It can unshare at the
> beginning of each subtest to get a clean state (e.g. in patch 8).
> test_progs.c only ensures a config made by an earlier test does
> not affect the following tests.

It's true that it gives more flexibility for test setup, but if we go
that way, we should do it consistently for CPU affinity resetting and
whatever else we do per-subtest. I wonder what your answers would be
for the above questions. We can go either way, just let's be
consistent.