For every pytest module, create a copy of its system test directory and
run the test from that directory. This makes it easier to clean up
afterwards and makes it less error-prone when re-running (failed) tests.
Configure the logger to capture the module's log output into a separate
file available from the temporary directory. This is quite convenient
for exploring failures.
Cases where temporary directory should be kept are handled in a
follow-up commits.
This is basically the pytest re-implementation of the run.sh script.
The fixture is applied to every module and ensures complete test
setup/teardown, such as starting/stopping servers, detecting coredumps
etc.
Note that the fixture system_test_dir is not defined yet. It is omitted
now for review readability and it's added in follow-up commits.
Add fixtures for deriving system test name from the directory name and
for a module-specific logger.
Add fixtures to execute shell and perl scripts. Similar to how run.sh
calls commands, this functionality is also needed in pytest. The
fixtures take care of switching to a proper directory, logging
everything and handling errors.
Note that the fixture system_test_dir is not defined yet. It is omitted
now for review readability and it's added in follow-up commits.
This is basically a pytest re-implementation of the get_ports.sh script.
The main difference is that ports are assigned on a module basis, rather
than a directory basis. Module is the new atomic unit for parallel
execution, therefore it needs to have unique ports to avoid collisions.
Each module gets its ports through the env fixture which is updated with
ports and other module-specific variables.
Some system tests require extra programs and/or dependencies to be
compiled first. This is done via `make check` with the automake
framework when using check_* variables such as check_PROGRAMS.
To avoid running any tests via the automake framework, set the TESTS
env variable to empty string and utilize `make -e check` to override
default Makefile variables with environment ones. This ensures automake
will only compile the needed dependencies without running any tests.
Additional consideration needs to be taken for xdist. The compilation
command should be called just once before any tests are executed. To
achieve that, use the pytest_configure() hook and check that the
PYTEST_XDIST_WORKER env variable isn't set -- if it is, it indicates
we're in the spawned xdist worker and the compilation was already done
by the main pytest process that spawned the workers.
This is mostly done to have on-par functionality with legacy test
framework. In the future, we should get rid of the need to run "empty"
make -e check and perhaps compile test-stuff by default.
The commands executed by pytest during a system test need to have the
same environment variables set as if they were executed by the run.sh
shell script.
It was decided that for the moment, legacy way of executing system tests
with run.sh should be kept, which complicates things a bit. In order to
avoid duplicating the required variables in both conf.sh and pytest, it
was decided to use the existing conf.sh as the only authoritative
place for the variables.
It is necessary to process the environment variables from conf.sh right
when conftest.py is loaded, since they might be needed right away (e.g.
to test for feature support during test collection).
This solution is a bit hacky and is only meant to be used during the
transitory phase when both pytest and the legacy run.sh are both
supported. In the future, a superior pytest-only solution should be
used.
For discussion of other options, refer to
https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/6809#note_318889
Ensure pytest picks up our python test modules, since we're using
tests_*.py convention, which is different from the default.
Configure pytest logging to display the output from all tests (even the
ones that passed). This ensures we have sufficient amount of information
to debug test post-mortem just from the artifacts.
The legacy system test framework uses pytest to execute some tests.
Since it'd be quite difficult to convince pytest to decide whether to
include conftest.py (or which ones to include when launching from
subdir), it makes more sense to have a shared conftest.py which is used
by both the legacy test runner invocations of pytest and the new pytest
system test runner. It is ugly, but once we drop support for the legacy
runner, we'll get rid of it.
Properly scope the *port fixtures in order to ensure they'll work as
expected with the new pytest runner. Instead of using "session" (which
means the fixture is only evaluated once for the entire execution of
pytest), use "module" scope, which is evaluated separately for each
module. The legacy runner invoked pytest for each system test
separately, while the new pytest runner is invoked once for all system
tests -- therefore it requires the more fine-grained "module" scope to
for the fixtures to work properly.
Remove python shebang, as conftest.py isn't supposed to be an executable
script.
Thread sanitizer warns that parts of the qp-trie are accessed
both with and without the mutex; the unlocked accesses happen during
destruction, so they should be benign, but there's no harm locking
anyway to convince tsan it is clean.
Also, ensure .tsan-suppress and .tsan-suppress-extra are in sync.
RUNTIME_CHECK on the "wrap" variable avoids possible NULL dereference:
thread.c: In function 'thread_wrap':
thread.c:60:15: error: dereference of possibly-NULL 'wrap' [CWE-690] [-Werror=analyzer-possible-null-dereference]
60 | *wrap = (struct thread_wrap){
The RUNTIME_CHECK was there before
7d1ceaf35dbc25dfbca32deffa7f6ff8f452e75f.
The line summarising TSAN reports was misplaced in the ASAN territory
and thus never used.
I also made core dumps, assertion failures, and TSAN reports detection
independent of each other.
The last use of the cmocka_add_test_byname() helper macro was removed in
commit 63fe9312ff8f54bc79e399bdbd5aaa15cd3e5459. Remove the
<isc/cmocka.h> header that defines it.
tests/isc/ht_test.c triggers the following compiler warnings when built
against development versions of cmocka:
In file included from ht_test.c:24:
ht_test.c: In function ‘test_ht_full’:
ht_test.c:68:45: warning: passing argument 2 of ‘_assert_ptr_equal’ makes pointer from integer without a cast [-Wint-conversion]
68 | assert_ptr_equal((void *)i, (uintptr_t)f);
/usr/include/cmocka.h:1513:56: note: in definition of macro ‘assert_ptr_equal’
1513 | #define assert_ptr_equal(a, b) _assert_ptr_equal((a), (b), __FILE__, __LINE__)
| ^
/usr/include/cmocka.h:2907:36: note: expected ‘const void *’ but argument is of type ‘long unsigned int’
2907 | const void *b,
| ~~~~~~~~~~~~^
ht_test.c:163:45: warning: passing argument 2 of ‘_assert_ptr_equal’ makes pointer from integer without a cast [-Wint-conversion]
163 | assert_ptr_equal((void *)i, (uintptr_t)f);
/usr/include/cmocka.h:1513:56: note: in definition of macro ‘assert_ptr_equal’
1513 | #define assert_ptr_equal(a, b) _assert_ptr_equal((a), (b), __FILE__, __LINE__)
| ^
/usr/include/cmocka.h:2907:36: note: expected ‘const void *’ but argument is of type ‘long unsigned int’
2907 | const void *b,
| ~~~~~~~~~~~~^
These are caused by a change to the definitions of pointer assert
functions in cmocka's development branch [1]. Fix by casting the
affected variables to (void *) instead of (uintptr_t).
[1] https://git.cryptomilk.org/projects/cmocka.git/commit/?id=09621179af67535788a67957a910d9f17c975b45
Development versions of cmocka require the intmax_t and uintmax_t types
to be defined by the time the test code includes the <cmocka.h> header.
These types are defined in the <stdint.h> header, which is included by
the <inttypes.h> header, which in turn is already explicitly included by
some of the programs in the tests/ directory. Ensure all programs in
that directory that include the <cmocka.h> header also include the
<inttypes.h> header to future-proof the code while keeping the change
set minimal and the resulting code consistent. Also prevent explicitly
including the <stdint.h> header in those programs as it is included by
the <inttypes.h> header.
When the FIPS provider is available, RSASHA1 signing keys for zone
"example.com." are ignored if the zone is attempted to be signed with
the dnssec-signzone "-F" (FIPS mode) option:
"fatal: No signing keys specified or found"
The upforwd test for forwarding updates to a dead primary can continue
running a little bit past its end, causing update replies to be
recorded during a subsequent test case. Correct this by only looking
for update requests and replies for the specific domain name being
tested at any given time.
After the RCU changes were merged, the `upforwd` test started
consistenly failing when run under thread sanitizer. After some
investigation, it turned out that retry attempts were continuing after
the "update forwarding to dead primary" test. This caused mismatches
in the DNSTAP message counts for the subsequent tests, because they
were also counting retries.
Fix this problem by `wait`ing for the `nsupdate` processes to exit.
While investigating the bug, I replaced several fixed 15 second delays
with `wait_for_log`, so the test runs faster.
Move registration and deregistration of the main thread from
`isc_loopmgr_run()` into `isc__initialize()` / `isc__shutdown()`:
liburcu-qsbr fails an assertion if we try to use it from an
unregistered thread, and we need to be able to use it when the
event loops are not running.
Use `rcu_assign_pointer()` and `rcu_dereference()` in qp-trie
transactions so that they properly mark threads as online. The
RCU-protected pointer is no longer declared atomic because
liburcu does not (yet) use standard C atomics.
Fix the definition of `isc_qsbr_rcu_dereference()` to return
the referenced value, and to call the right function inside
liburcu.
Change the thread sanitizer suppressions to match any variant of
`rcu_*_barrier()`
An omission pointed out by the following report from Coverity:
/lib/isc/loop.c: 483 in isc_loopmgr_pause()
>>> CID 455002: Error handling issues (CHECKED_RETURN)
>>> Calling "uv_async_send" without checking return value (as is done elsewhere 5 out of 6 times).
483 uv_async_send(&loop->pause_trigger);
when reading on a streamdns socket failed due to timeout, but
the dispatch was still waiting for other responses, it would
resume reading by calling isc_nm_read() again. this caused
an assertion because the socket was already reading.
we now check that either the socket is reading, or that it was
already reading on the same handle.
Create and free per-CPU helper threads from the main thread and tell
thread sanitizer to suppress leaking threads. (We are not leaking
threads ourselves and we can safely ignore the Userspace-RCU thread
leaks.)
All the places the qp-trie code was using `call_rcu()` needed
`__tsan_release()` and `__tsan_acquire()` annotations, so
add a couple of wrappers to encapsulate this pattern.
With these wrappers, the tests run almost clean under thread
sanitizer. The remaining problems are due to `rcu_barrier()`
which can be suppressed using `.tsan-suppress`. It does not
suppress the whole of `liburcu`, because we would like thread
sanitizer to detect problems in `call_rcu()` callbacks, which
are called from `liburcu`.
The CI jobs have been updated to use `.tsan-suppress` by
default, except for a special-case job that needs the
additional suppressions in `.tsan-suppress-extra`.
We might be able to get rid of some of this after liburcu gains
support for thread sanitizer.
Note: the `rcu_barrier()` suppression is not entirely effective:
tsan sometimes reports races that originate inside `rcu_barrier()`
but tsan has discarded the stack so it does not have the
information required to suppress the report. These "races" can
be made much easier to reproduce by adding `atexit_sleep_ms=1000`
to `TSAN_OPTIONS`. The problem with tsan's short memory can be
addressed by increasing `history_size`: when it is large enough
(6 or 7) the `rcu_barrier()` stack usually survives long enough
for suppression to work.
