Message ID | 20190905152709.111193-2-sdf@google.com |
---|---|
State | Superseded |
Delegated to: | BPF Maintainers |
Headers | show |
Series | selftests/bpf: move sockopt tests under test_progs | expand |
On Thu, Sep 5, 2019 at 7:40 PM Stanislav Fomichev <sdf@google.com> wrote: > > test__join_cgroup() combines the following operations that usually > go hand in hand and returns cgroup fd: > > * setup cgroup environment (make sure cgroupfs is mounted) > * mkdir cgroup > * join cgroup > > It also marks a test as a "cgroup cleanup needed" and removes cgroup > state after the test is done. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- First of all, thanks a lot for all these improvements to test_progs and converting existing tests to test_progs tests, it's great to see this consolidation! [...] > @@ -17,6 +18,7 @@ struct prog_test_def { > int error_cnt; > int skip_cnt; > bool tested; > + bool need_cgroup_cleanup; > > const char *subtest_name; > int subtest_num; > @@ -122,6 +124,39 @@ void test__fail(void) > env.test->error_cnt++; > } > > +int test__join_cgroup(const char *path) This doesn't seem to be testing-specific functionality, tbh. It's certainly useful helper, but I don't think it warrants test__ prefix. As for test->need_cgroup_cleanup field, this approach won't scale if we need other types of custom/optional clean up after test ends. Generic test framework code will need to know about every possible custom setup to be able to cleanup/undo it. I wonder if generalizing it to be able to add custom clean up code (some test frameworks have "teardown" overrides for this) would be cleaner and more maintainable solution. Something like: typedef void (* test_teardown_fn)(struct test *test, void *ctx); /* somewhere at the beginning of test: */ test__schedule_teardown(test_teardown_fn cb, void *ctx); [...] > + > + if (test->need_cgroup_cleanup) > + cleanup_cgroup_environment(); Then in generic framework we'll just process a list of callbacks and call each one with stored ctx per each callback (in case we need some custom data to be stored, of course). Thoughts? [...]
On 09/06, Andrii Nakryiko wrote: > On Thu, Sep 5, 2019 at 7:40 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > test__join_cgroup() combines the following operations that usually > > go hand in hand and returns cgroup fd: > > > > * setup cgroup environment (make sure cgroupfs is mounted) > > * mkdir cgroup > > * join cgroup > > > > It also marks a test as a "cgroup cleanup needed" and removes cgroup > > state after the test is done. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > First of all, thanks a lot for all these improvements to test_progs > and converting existing tests to test_progs tests, it's great to see > this consolidation! > > [...] > > > @@ -17,6 +18,7 @@ struct prog_test_def { > > int error_cnt; > > int skip_cnt; > > bool tested; > > + bool need_cgroup_cleanup; > > > > const char *subtest_name; > > int subtest_num; > > @@ -122,6 +124,39 @@ void test__fail(void) > > env.test->error_cnt++; > > } > > > > +int test__join_cgroup(const char *path) > > This doesn't seem to be testing-specific functionality, tbh. It's > certainly useful helper, but I don't think it warrants test__ prefix. I didn't like the mess we used to have: if (setup_cgroup_environment()) goto cleanup_obj; cgroup_fd = create_and_get_cgroup(CG_PATH); if (cgroup_fd < 0) goto cleanup_cgroup_env; if (join_cgroup(CG_PATH)) goto cleanup_cgroup; ... do the test cleanup_cgroup_environment(); All I really want to do in several tests is to create a temporary cgroup and join it (I don't even really care about the name most of the time). We can rename and move this test__join_cgroup into cgroup_helpers.h if you prefer, I don't really mind. I just want to avoid repeating those 10 lines over and over in each test that just wants to run in a cgroup. > As for test->need_cgroup_cleanup field, this approach won't scale if > we need other types of custom/optional clean up after test ends. > Generic test framework code will need to know about every possible > custom setup to be able to cleanup/undo it. > > I wonder if generalizing it to be able to add custom clean up code > (some test frameworks have "teardown" overrides for this) would be > cleaner and more maintainable solution. > > Something like: > > typedef void (* test_teardown_fn)(struct test *test, void *ctx); > > /* somewhere at the beginning of test: */ > test__schedule_teardown(test_teardown_fn cb, void *ctx); > > [...] > > > + > > + if (test->need_cgroup_cleanup) > > + cleanup_cgroup_environment(); > > Then in generic framework we'll just process a list of callbacks and > call each one with stored ctx per each callback (in case we need some > custom data to be stored, of course). > > Thoughts? Idk, I don't see the need to be too generic since we control both the tests and the framework. So putting something like test__join_cgroup and doing automatic cleanup looks fine to me if this is shared between several tests. If, at some point, it becomes unmanageable, we can think about refactoring; but until then, I'd not bother tbh.
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index c7595b4ed55d..e145954d3765 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -102,7 +102,7 @@ $(OUTPUT)/test_socket_cookie: cgroup_helpers.c $(OUTPUT)/test_sockmap: cgroup_helpers.c $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c $(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c -$(OUTPUT)/test_progs: trace_helpers.c +$(OUTPUT)/test_progs: cgroup_helpers.c trace_helpers.c $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c $(OUTPUT)/test_netcnt: cgroup_helpers.c @@ -196,7 +196,7 @@ $(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ | $(ALU32_BUILD_DIR) $(CC) $(TEST_PROGS_CFLAGS) $(CFLAGS) \ -o $(ALU32_BUILD_DIR)/test_progs_32 \ - test_progs.c test_stub.c trace_helpers.c prog_tests/*.c \ + test_progs.c test_stub.c cgroup_helpers.c trace_helpers.c prog_tests/*.c \ $(OUTPUT)/libbpf.a $(LDLIBS) $(ALU32_BUILD_DIR)/test_progs_32: $(PROG_TESTS_H) diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index e8616e778cb5..af75a1c7a458 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -2,6 +2,7 @@ /* Copyright (c) 2017 Facebook */ #include "test_progs.h" +#include "cgroup_helpers.h" #include "bpf_rlimit.h" #include <argp.h> #include <string.h> @@ -17,6 +18,7 @@ struct prog_test_def { int error_cnt; int skip_cnt; bool tested; + bool need_cgroup_cleanup; const char *subtest_name; int subtest_num; @@ -122,6 +124,39 @@ void test__fail(void) env.test->error_cnt++; } +int test__join_cgroup(const char *path) +{ + int fd; + + if (!env.test->need_cgroup_cleanup) { + if (setup_cgroup_environment()) { + fprintf(stderr, + "#%d %s: Failed to setup cgroup environment\n", + env.test->test_num, env.test->test_name); + return -1; + } + + env.test->need_cgroup_cleanup = true; + } + + fd = create_and_get_cgroup(path); + if (fd < 0) { + fprintf(stderr, + "#%d %s: Failed to create cgroup '%s' (errno=%d)\n", + env.test->test_num, env.test->test_name, path, errno); + return fd; + } + + if (join_cgroup(path)) { + fprintf(stderr, + "#%d %s: Failed to join cgroup '%s' (errno=%d)\n", + env.test->test_num, env.test->test_name, path, errno); + return -1; + } + + return fd; +} + struct ipv4_packet pkt_v4 = { .eth.h_proto = __bpf_constant_htons(ETH_P_IP), .iph.ihl = 5, @@ -530,6 +565,9 @@ int main(int argc, char **argv) fprintf(env.stdout, "#%d %s:%s\n", test->test_num, test->test_name, test->error_cnt ? "FAIL" : "OK"); + + if (test->need_cgroup_cleanup) + cleanup_cgroup_environment(); } stdio_restore(); printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index c8edb9464ba6..e518bd5da3e2 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -71,6 +71,7 @@ extern void test__force_log(); extern bool test__start_subtest(const char *name); extern void test__skip(void); extern void test__fail(void); +extern int test__join_cgroup(const char *path); #define MAGIC_BYTES 123
test__join_cgroup() combines the following operations that usually go hand in hand and returns cgroup fd: * setup cgroup environment (make sure cgroupfs is mounted) * mkdir cgroup * join cgroup It also marks a test as a "cgroup cleanup needed" and removes cgroup state after the test is done. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/testing/selftests/bpf/Makefile | 4 +-- tools/testing/selftests/bpf/test_progs.c | 38 ++++++++++++++++++++++++ tools/testing/selftests/bpf/test_progs.h | 1 + 3 files changed, 41 insertions(+), 2 deletions(-)