Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support make check #9614

Closed
wants to merge 2 commits into from
Closed

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Traditionally make check is the mechanism used to invoke the
test suite for a C program built with autoconf/automake. Supporting
this functionality is desirable since it makes it easier for users and
new ZFS developers to get started working with the code.

Depends on #9613. Posted for review and discussion, but blocked
until #9613 is approved and merged.

Description

Add support for the traditional make check target. When invoked,
the following test wrappers will be run:

  • scripts/zloop.sh - Performs a 10 minute run using the default
    options. This test does not require escaleted priviledges.

  • scripts/zfs-test.sh - Perform a full run of the ZFS Test Suite.
    This normally will take several hours to complete and requires:

    1. The user sudo configured to not require a password.
    2. The ZFS kernel modules must already be loaded.
    3. All required system utilities are installed.
    4. /var/tmp/ has at least 12G of available free space.
    5. When running in-tree the zfs-helper.sh symlinks exist.

If these requirements are not met make check may skip the
ZTS run entirely, or individual test cases may fail.

Functionally, this commit updates the top-level Makefile.am
to set TESTS and TESTS_ENVIRONMENT for make check to run.

Improves the environment sanity checks performed by zfs-tests.sh
before starting testing. This includes verifing the kmods are
loaded (it is no longer done automatically), the requi

How Has This Been Tested?

In a properly configured VM will all of the required dependencies
installed:

sh autogen.sh
./configure
make -s -j$(nproc)

sudo ./scripts/zfs.sh`
sudo ./scripts/zfs-helpers.sh -iv`
make check
...
Executing zloop.sh for 600 seconds.
11/22 14:33:24 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 2 -r 0 -R 2 -v 4 -a 9 -T 86 -P 20 -s 512m -f /var/tmp/zloop-run 
11/22 14:35:15 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 2 -r 10 -R 2 -v 4 -a 12 -T 12 -P 11 -s 512m -f /var/tmp/zloop-run 
11/22 14:37:25 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 2 -r 0 -R 2 -v 5 -a 9 -T 34 -P 17 -s 512m -f /var/tmp/zloop-run 
11/22 14:38:38 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 1 -r 8 -R 2 -v 4 -a 9 -T 54 -P 13 -s 512m -f /var/tmp/zloop-run 
11/22 14:40:20 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 2 -r 0 -R 1 -v 2 -a 9 -T 43 -P 19 -s 512m -f /var/tmp/zloop-run 
11/22 14:41:59 /home/behlendo/src/git/zfs/bin/ztest -G -VVVVV -m 2 -r 0 -R 2 -v 4 -a 12 -T 83 -P 36 -s 512m -f /var/tmp/zloop-run 
zloop finished, 0 crashes found
PASS: scripts/zloop.sh
Test: /home/behlendo/src/git/zfs/tests/zfs-tests/tests/functional/libzfs/setup (run as root) [00:01] [PASS]
Test: /home/behlendo/src/git/zfs/tests/zfs-tests/tests/functional/libzfs/many_fds (run as root) [00:00] [PASS]
...
PASS: scripts/zfs-tests.sh
==================
All 2 tests passed
==================

Submitted to the CI to verify zfs-tests.sh on other distributions.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

On modern Linux systems there is no need to set a specific
`/proc/sys/kernel/core_pattern`, the `coredumpctl dump` command
can be used to extract the core using the specific name.

By removing the need to set the core_pattern zloop.sh can now
be run with normal user priviledges.  Previous work has already
been done to allow ztest to run without any of the kmods being
loaded.

Additionally, cleanup a few remaining shellcheck warnings in
the zloop.sh script.  Allow options to be passed as environment
variables.  Print a message when running for a fixed duration.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Add support for the traditional `make check` target.  When invoked,
the following test wrappers will be run:

  * scripts/zloop.sh - Performs a 10 minute run using the default
    options.  This test does not require escaleted priviledges.

  * scripts/zfs-test.sh - Perform a full run of the ZFS Test Suite.
    This normally will take several hours to complete and requires:

      1) The user sudo configured to not require a password.
      2) The ZFS kernel modules must already be loaded.
      3) All required system utilities are installed.
      4) /var/tmp/ has at least 12G of available free space.
      5) When running in-tree the zfs-helper.sh symlinks exist.

   If these requirements are not met `make check` may skip the
   ZTS run entirely, or individual test cases may fail.

Functionally, this commit updates the top-level Makefile.am
to set TESTS and TESTS_ENVIRONMENT for `make check` to run.

Improves the environment sanity checks performed by zfs-tests.sh
before starting testing.  This includes verifing the kmods are
loaded (it is no longer done automatically), the required utilities
are available, and passwordless sudo is configured.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Nov 22, 2019
@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #9614 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9614      +/-   ##
==========================================
+ Coverage   79.19%   79.21%   +0.01%     
==========================================
  Files         418      418              
  Lines      123531   123531              
==========================================
+ Hits        97828    97849      +21     
+ Misses      25703    25682      -21
Flag Coverage Δ
#kernel 79.88% <ø> (+0.03%) ⬆️
#user 66.86% <ø> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c46813...924afa1. Read the comment docs.

@loli10K
Copy link
Contributor

loli10K commented Nov 24, 2019

IIRC some users reported loss of data running the ZTS with their personal/production pools attached/imported (which in itself is a bad idea), we should probably document this feature as "possibly destructive", maybe print a warning or ask for confirmation before execution.

@Fabian-Gruenbichler
Copy link
Contributor

Fabian-Gruenbichler commented Nov 25, 2019 via email

@behlendorf
Copy link
Contributor Author

we should probably document this feature as "possibly destructive", maybe print a warning or ask for confirmation before execution.

Good idea, I'll see about adding some additional safe guards.

this seems a bit too much for my taste. 'make check' is run by default in Debian packaging

Yes, I wondered about that. My thought at the time was this wouldn't effect most CI setups since the ZTS would likely be skipped entirely since it does require some customization of the environment (passwordless sudo, kmods loaded, plus some additional mandatory packages).

Perhaps we should simply disable the ZTS entirely from make check unless the user explicitly opts in.

@@ -455,7 +464,7 @@ if [ "$(id -u)" = "0" ]; then
fi

if [ "$(sudo whoami)" != "root" ]; then
fail "Passwordless sudo access required."
skip "Passwordless sudo access required."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this ask for a password, if sudo is not configured? I think you can use sudo -n true and return 0 is sudo-with-password is OK.

@Fabian-Gruenbichler
Copy link
Contributor

Fabian-Gruenbichler commented Dec 12, 2019 via email

@behlendorf behlendorf closed this Jun 18, 2020
@behlendorf behlendorf deleted the fix-make-check branch April 19, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants