Tidy some scripts #454

chrissie-c · 2022-01-12T08:50:37Z

Errors reported by Centos covscan

/bin/bash in start.test is for echo -n

Errors reported by Centos covscan /bin/bash in start.test is for echo -n

jfriesse

Looks good so ACK by me

wferi · 2022-01-12T11:17:34Z

/bin/bash in start.test is for echo -n

Just out of curiosity: wouldn't something as simple as printf "$$-$RANDOM" suffice?

and echo -n works anyway

chrissie-c · 2022-01-12T12:43:38Z

@wferi yes, that makes a lot more sense

wenningerk · 2022-01-12T13:54:59Z

or for the paranoid use mktemp (in a common temp-file) to assure it is really unique ...

chrissie-c · 2022-01-12T14:38:10Z

@wenningerk libqb already does this for directories created by the client. This name is the unique name for the service (that clients connect to). These need to be unique for testing so we can run >1 instance at the same time. In normal running those names are well-known names (like cmap/votequorum).

wenningerk · 2022-01-12T15:21:11Z

@wenningerk libqb already does this for directories created by the client. This name is the unique name for the service (that clients connect to). These need to be unique for testing so we can run >1 instance at the same time. In normal running those names are well-known names (like cmap/votequorum).

Yep - running multiple tests (with multiple server instances) in parallel was what I was thinking of + the unlikely case that random numbers might be the same - but as said just for the paranoid ;-)
Meant one tmp-dir for all tests on a machine as to be able to sync via that mechanism. sbd tests are syncing creation of device-mappings (for a virtual block device) like this. But that didn't help much as having the image-files on the same physical drive seemed to bring performance-issues due to syncing the block-devices. Hope that is solved now since I moved to in-memory image-files.

chrissie-c · 2022-01-12T15:44:19Z

The server names are in the UNIX-domain socket namespace, not a filesystem. So mk[s]temp is no help here. There is the option for libqb to use filesystem sockets (which is only used AFAIK for inter-container communication). I have PR somewhere for testing that ;-)

wferi · 2022-01-12T15:57:43Z

tests/start.test

-NAME="$$-`date +%N`"
-echo -n "$NAME" > ipc-test-name
+
+printf "$$-$RANDOM" > ipc-test-name


Unfortunately $RANDOM is Bash-specific again, and you already reverted that part.
Speaking of paranoia, I should have proposed printf %s "$$-$RANDOM" anyway. :)

chrissie-c · 2022-01-12T16:05:56Z

Ahhh! In that case I'll put the date thing back in again. Thanks for checking.

chrissie-c · 2022-01-12T16:10:18Z

Having said that date+%N doesn't work on BSD anyway (it just prints 'N') so I'll do some work on that.

And this was going to be a simple PR :)

I changed %N to %s as BSD's date command doesn't support %N. Seconds + PID should be enough ....

wenningerk · 2022-01-12T22:27:26Z

Maybe the mktemp approach isn't that stupid after all - at least it exists on BSD ;-)
The nano-seconds are a special thing. They don't exist in the c-library as well.
Remember the pain that I had to implement them so that the alert-agents could
define a high-resolution-timestamp. @wferi: thanks for finding a bug with that
back then iirc.

chrissie-c · 2022-01-14T10:12:57Z

retest this please

When the PID numbers get big, the socket name overflows the allowed limit

wferi · 2022-01-14T13:58:02Z

This name is the unique name for the service (that clients connect to). These need to be unique for testing so we can run >1 instance at the same time. In normal running those names are well-known names (like cmap/votequorum).

Why not use the PID of the make process, though? You could even communicate it via the environment and get rid of ipc-test-name, start.test and the randomness: export IPC_TEST_NAME != printf 'what-%d-ever' "$$PPID".

wferi · 2022-01-16T15:35:02Z

On the other hand, isn't the current start.test method racy under parallel builds (make -j)?

chrissie-c · 2022-01-17T07:30:11Z

@wferi something racy in libqb? Surely not? ;-)

I'm not sure what it would race with though, the tests are run in series even with make -j

It's been seen to time out too early and fail the tests

wferi · 2022-01-17T09:15:18Z

the tests are run in series even with make -j

Well, this is what I get in start.log if I append sleep 0.1; ps xf to the end of start.test:

  PID TTY      STAT   TIME COMMAND
31857 pts/0    S      0:00 /bin/bash
  795 pts/0    S+     0:00  \_ make -j 16 check
  813 pts/0    S+     0:00      \_ make check-TESTS
  827 pts/0    S+     0:00          \_ /bin/bash -c set +e; bases='start.log array.log map.log rb.log list.log log.log blackbox-segfault.sh.log loop.log ipc.log resources.log'; bases=`for i in $bases; do echo $i; done | sed 's/\.log$//'`; bases=`echo $bases`; \ log_list=`for i in $bases; do echo $i.log; done`; \ trs_list=`for i in $bases; do echo $i.trs; done`; \ log_list=`echo $log_list`; trs_list=`echo $trs_list`; \ make  test-suite.log TEST_LOGS="$log_list"; \ exit $?;
  836 pts/0    S+     0:00              \_ make test-suite.log TEST_LOGS=start.log array.log map.log rb.log list.log log.log blackbox-segfault.sh.log loop.log ipc.log resources.log
  845 pts/0    S+     0:00                  \_ /bin/bash ../build-aux/test-driver --test-name start.test --log-file start.log --trs-file start.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./start.test
  880 pts/0    S+     0:00                  |   \_ /bin/sh ./start.test
 1121 pts/0    R+     0:00                  |       \_ ps xf
  859 pts/0    S+     0:00                  \_ /bin/bash ../build-aux/test-driver --test-name log.test --log-file log.log --trs-file log.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./log.test
  890 pts/0    S+     0:00                  |   \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/log.test
 1023 pts/0    Sl     0:00                  |       \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/log.test
  860 pts/0    S+     0:00                  \_ /bin/bash ../build-aux/test-driver --test-name blackbox-segfault.sh --log-file blackbox-segfault.sh.log --trs-file blackbox-segfault.sh.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./blackbox-segfault.sh
  897 pts/0    S+     0:00                  |   \_ /bin/sh ./blackbox-segfault.sh
  862 pts/0    S+     0:00                  \_ /bin/bash ../build-aux/test-driver --test-name loop.test --log-file loop.log --trs-file loop.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./loop.test
  893 pts/0    S+     0:00                  |   \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/loop.test
 1085 pts/0    S      0:00                  |       \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/loop.test
  863 pts/0    S+     0:00                  \_ /bin/bash ../build-aux/test-driver --test-name ipc.test --log-file ipc.log --trs-file ipc.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./ipc.test
  891 pts/0    S+     0:00                      \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/ipc.test
 1003 pts/0    S      0:00                          \_ /build/libqb-FX3vsk/libqb-2.0.4/tests/.libs/ipc.test
 1007 pts/0    Z      0:00                              \_ [ipc.test] <defunct>
[...]

Doesn't this mean that the file reads in the actual tests race with the write in start.test?

chrissie-c · 2022-01-17T09:22:12Z

oh good, another bug in the test suite!

chrissie-c · 2022-01-17T09:44:49Z

I think, for the moment, I'm going to leave that - it's outside the scope of this PR and I don't currently have the motivation to muck around with Makefile.am files (TBH I never have the motivation to much about with Makefile.am files)

jfriesse

Looks good (as long as we ignore parallel builds - and we do) so ACK

chrissie-c · 2022-01-19T12:50:18Z

Thanks, I will accept patches to make parallel builds work properly ;-)

wferi · 2022-01-21T20:30:49Z

Speaking of parallel test, has the ipc-test-name to be shared among the tests? Or can each test use a different ipc-test-name as long as they are unique?

fabbione · 2022-01-21T21:47:08Z

@wferi I remember looking at it at some point.. I don´t remember the details of the implementation, but I do offer one piece of advice. Some of the libqb tests are very CPU intensive and they have timeouts and alike.. running too many of them in parallel might cause too many false errors. I have experienced this in CI when running multiple libqb builds in parallel (not even the test suite), very often they would fail due to timeouts. CI now serializes libqb builds to avoid that false error.

chrissie-c · 2022-01-24T07:41:20Z

@wferi In theory different tests could use a different ipc-test-name. In practice though, only test.ipc actually does any IPC testing so it's not really relevant :)

Tidy some scripts

db7a5d4

Errors reported by Centos covscan /bin/bash in start.test is for echo -n

jfriesse approved these changes Jan 12, 2022

View reviewed changes

Revert start.test part as bash is not installed on *BSD

b326674

and echo -n works anyway

Use printf - OF COURSE it should use printf

867c66a

wferi reviewed Jan 12, 2022

View reviewed changes

Revert the DATE call (with change) because $RANDOM is a bashism

eb60ad2

I changed %N to %s as BSD's date command doesn't support %N. Seconds + PID should be enough ....

Shrink the name of the dlock tests as they cause random failures

880c7b7

When the PID numbers get big, the socket name overflows the allowed limit

Increase timeout of thread check.

3ddb280

It's been seen to time out too early and fail the tests

chrissie-c requested a review from jfriesse January 19, 2022 09:16

jfriesse approved these changes Jan 19, 2022

View reviewed changes

chrissie-c merged commit 73472e0 into ClusterLabs:main Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy some scripts #454

Tidy some scripts #454

chrissie-c commented Jan 12, 2022

jfriesse left a comment

wferi commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022 •

edited

Loading

chrissie-c commented Jan 12, 2022

wferi Jan 12, 2022

chrissie-c commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022

chrissie-c commented Jan 14, 2022

wferi commented Jan 14, 2022

wferi commented Jan 16, 2022

chrissie-c commented Jan 17, 2022

wferi commented Jan 17, 2022

chrissie-c commented Jan 17, 2022

chrissie-c commented Jan 17, 2022

jfriesse left a comment

chrissie-c commented Jan 19, 2022

wferi commented Jan 21, 2022

fabbione commented Jan 21, 2022

chrissie-c commented Jan 24, 2022

Tidy some scripts #454

Tidy some scripts #454

Conversation

chrissie-c commented Jan 12, 2022

jfriesse left a comment

Choose a reason for hiding this comment

wferi commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022 • edited Loading

chrissie-c commented Jan 12, 2022

wferi Jan 12, 2022

Choose a reason for hiding this comment

chrissie-c commented Jan 12, 2022

chrissie-c commented Jan 12, 2022

wenningerk commented Jan 12, 2022

chrissie-c commented Jan 14, 2022

wferi commented Jan 14, 2022

wferi commented Jan 16, 2022

chrissie-c commented Jan 17, 2022

wferi commented Jan 17, 2022

chrissie-c commented Jan 17, 2022

chrissie-c commented Jan 17, 2022

jfriesse left a comment

Choose a reason for hiding this comment

chrissie-c commented Jan 19, 2022

wferi commented Jan 21, 2022

fabbione commented Jan 21, 2022

chrissie-c commented Jan 24, 2022

wenningerk commented Jan 12, 2022 •

edited

Loading