[PATCH 0/2] commandtest: Make 'test4' more reliable
See 2/2 for the explanation. Peter Krempa (2): vircommand: REPRODUCER: Add artificial timeout before exitting daemonization helper tests: commandtest: Make 'test4' checking daemonization more reliable src/util/vircommand.c | 1 + tests/commanddata/test4.log | 1 + tests/commandhelper.c | 17 ++++++++++++++++- tests/commandtest.c | 3 ++- 4 files changed, 20 insertions(+), 2 deletions(-) -- 2.26.2
Let the intermediate process linger for a moment to reproduce the sporradic failure in 'commandtest'. --- src/util/vircommand.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5ce69ef241..f6da8b5cf1 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -783,6 +783,7 @@ virExec(virCommandPtr cmd) _("Unable to wait for child process")); _exit(EXIT_FAILURE); } + g_usleep(100*1000); _exit(EXIT_SUCCESS); } } -- 2.26.2
On a Tuesday in 2020, Peter Krempa wrote:
Let the intermediate process linger for a moment to reproduce the sporradic failure in 'commandtest'. --- src/util/vircommand.c | 1 + 1 file changed, 1 insertion(+)
This breaks commandtest for me: TEST: commandtest 5) Command Exec test4 test ... Offset 163 Expect [yes] Actual [no] ... FAILED
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5ce69ef241..f6da8b5cf1 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -783,6 +783,7 @@ virExec(virCommandPtr cmd) _("Unable to wait for child process")); _exit(EXIT_FAILURE); } + g_usleep(100*1000); _exit(EXIT_SUCCESS); } }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The 'commandhelper' checks effectively whether the parent process is still around to report whether it was daemonized or not. This creates a unlikely race condition in cases when we do actually daemonize the process as the intermediate process used for the daemonization might not have terminated yet which would report wrong result leading to test failure. For now there's just 'test4' which actually daemonizes the process. Add an argument '--check-daemonize' which asks for retires of the daemonization check in cases where we expect that the commandhelper is going to be daemonized and use it in 'test4' to make the test more reliable. I've observed the test failure sporradically when my box is more loaded e.g. while building two trees at once. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/commanddata/test4.log | 1 + tests/commandhelper.c | 17 ++++++++++++++++- tests/commandtest.c | 3 ++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log index 24a37a7e96..4fd40cc474 100644 --- a/tests/commanddata/test4.log +++ b/tests/commanddata/test4.log @@ -1,3 +1,4 @@ +ARG:--check-daemonize ENV:DISPLAY=:0.0 ENV:HOME=/home/test ENV:HOSTNAME=test diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 4266e11902..b366483771 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -72,6 +72,8 @@ int main(int argc, char **argv) { char *buffers[3] = {NULL, NULL, NULL}; size_t buflen[3] = {0, 0, 0}; char c; + bool daemonize_check = false; + size_t daemonize_retries = 3; if (!log) return ret; @@ -83,6 +85,8 @@ int main(int argc, char **argv) { sscanf(argv[i], "%u%c", &readfds[numreadfds++], &c) != 1) { printf("Could not parse fd %s\n", argv[i]); goto cleanup; + } else if (STREQ(argv[i], "--check-daemonize")) { + daemonize_check = true; } } @@ -126,7 +130,18 @@ int main(int argc, char **argv) { fprintf(log, "FD:%zu\n", i); } - fprintf(log, "DAEMON:%s\n", getpgrp() != getppid() ? "yes" : "no"); + while (true) { + bool daemonized = getpgrp() != getppid(); + + if (daemonize_check && !daemonized && daemonize_retries-- > 0) { + usleep(100*1000); + continue; + } + + fprintf(log, "DAEMON:%s\n", daemonized ? "yes" : "no"); + break; + } + if (!(cwd = getcwd(NULL, 0))) goto cleanup; if (strlen(cwd) > strlen(".../commanddata") && diff --git a/tests/commandtest.c b/tests/commandtest.c index f0e60ee5fe..9bef825239 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -255,7 +255,8 @@ static int test3(const void *unused G_GNUC_UNUSED) */ static int test4(const void *unused G_GNUC_UNUSED) { - virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + virCommandPtr cmd = virCommandNewArgList(abs_builddir "/commandhelper", + "--check-daemonize", NULL); char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); pid_t pid; int ret = -1; -- 2.26.2
On a Tuesday in 2020, Peter Krempa wrote:
The 'commandhelper' checks effectively whether the parent process is still around to report whether it was daemonized or not.
This creates a unlikely race condition in cases when we do actually daemonize the process as the intermediate process used for the daemonization might not have terminated yet which would report wrong result leading to test failure.
For now there's just 'test4' which actually daemonizes the process.
Add an argument '--check-daemonize' which asks for retires of the
:%s/retires/retries/g
daemonization check in cases where we expect that the commandhelper is going to be daemonized and use it in 'test4' to make the test more reliable.
I've observed the test failure sporradically when my box is more loaded
:%s/sporradically/sporadically/g ( justone r, like the islands https://en.wikipedia.org/wiki/Sporades ) Also: :%s/more loaded/under load/g or :%s/more loaded/under heavy load/g
e.g. while building two trees at once.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/commanddata/test4.log | 1 + tests/commandhelper.c | 17 ++++++++++++++++- tests/commandtest.c | 3 ++- 3 files changed, 19 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Thank you for fixing this. Jano
participants (2)
-
Ján Tomko -
Peter Krempa