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

test: Handle functional test disk-full error #29335

Merged
merged 1 commit into from May 8, 2024

Conversation

BrandonOdiwuor
Copy link
Contributor

Fixes: #23099

Handle disk-full more gracefully in functional tests

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 28, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK cbergqvist, tdb3, itornaza, achow101
Concept ACK epiccurious
Stale ACK maflcko, kevkevinpal, davidgumberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29387 (test: fix RPC coverage check by BrandonOdiwuor)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Jan 28, 2024
test/functional/test_runner.py Outdated Show resolved Hide resolved
test/functional/test_runner.py Outdated Show resolved Hide resolved
@kevkevinpal
Copy link
Contributor

Concept ACK ebb842a

@epiccurious
Copy link

epiccurious commented Jan 31, 2024

Tested ACK.

Aborts as expected with low disk space.

Temporary test directory at /tmp/test_runner_₿_🏃_20240131_100802
Running Unit Tests for Test Framework Modules
.....................
----------------------------------------------------------------------
Ran 21 tests in 22.349s

OK
Tests aborted: Insufficient space available in directory: /tmp/test_runner_₿_🏃_20240131_100802

@epiccurious
Copy link

Right now, the test aborts for low disk space after running the Unit Tests for Test Framework Modules.

Instead, does it make sense to abort for low disk space before?

@BrandonOdiwuor
Copy link
Contributor Author

@epiccurious moved aborting the tests before the Unit Tests for Test Framework Modules.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

test/functional/test_runner.py Outdated Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Feb 22, 2024

review ACK 16baf86

@DrahtBot DrahtBot requested review from kevkevinpal and removed request for maflcko and kevkevinpal February 22, 2024 09:11
@kevkevinpal
Copy link
Contributor

tested ACK 16baf86

This may be out of scope for this PR but I have filled up an SD card almost full and when I run the following,

I get the correct response

./test/functional/test_runner.py -t /Volumes/32GBSDCARD/

Temporary test directory at /Volumes/32GBSDCARD//test_runner_₿_🏃_20240222_215044
Tests aborted: Insufficient space available in directory: /Volumes/32GBSDCARD//test_runner_₿_🏃_20240222_215044

But when I run an individual test for example

./test/functional/mempool_accept.py --tmpdir /Volumes/32GBSDCARD/
I get the following error

Traceback (most recent call last):
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 130, in main
    self.setup()
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 266, in setup
    os.makedirs(self.options.tmpdir, exist_ok=False)
  File "<frozen os>", line 225, in makedirs
FileExistsError: [Errno 17] File exists: '/Volumes/32GBSDCARD'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 148, in main
    self.log.exception("Unexpected exception caught during testing")
    ^^^^^^^^
AttributeError: 'MempoolAcceptanceTest' object has no attribute 'log'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/kevkevin/DEVDIR/bitcoin/./test/functional/mempool_accept.py", line 376, in <module>
    MempoolAcceptanceTest().main()
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 154, in main
    exit_code = self.shutdown()
                ^^^^^^^^^^^^^^^
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 304, in shutdown
    if self.success == TestStatus.FAILED and self.options.pdbonfailure:
       ^^^^^^^^^^^^
AttributeError: 'MempoolAcceptanceTest' object has no attribute 'success'

The disk that I used

df -h
/dev/disk8s1 30Gi 29Gi 160Mi 100% 2.5k 1.6M 0% /Volumes/32GBSDCARD

@DrahtBot DrahtBot removed the request for review from kevkevinpal February 23, 2024 03:56
@itornaza
Copy link

itornaza commented Apr 2, 2024

tested ACK 2f7fcd5

The user is notified when appropriate that there might be insufficient free space to run the tests. Moreover, if a test fails due to insufficient space, the functional test runner halts. Also, there is a relevant warning when the --nocleanup flag is used and hence the free disk requirements get bigger.

Note: All tests run on macos 14.4.1 (23E224)

Standard tests

  • Did a code review and checked that there is adequate space on the file system to run the tests
  • Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
  • Run unit tests with make check and all tests pass
  • Run all functional tests with no extra flags and all tests pass and no warnings about free space
  • Run all functional tests with --extended and all tests pass as well

Manual tests

Used -j 13 for the number of jobs to be run in parallel since the M2 Pro Max chip supports them. Calculated the required space to run the tests with the 13 parallel jobs as described in the pr:

1.1 * 1024 + (13 - 1) * 100 MB = 2326 MB

and reated an image at /Volumes/tmp with the a little less space set to 2.2 GB, with the df -h /Volumes/tmp command returning:

Filesystem      Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk4s2   2.2Gi    16Mi   2.2Gi     1%       4  4.3G    0%   /Volumes/tmp

Running the functional tests with ./test/functional/test_runner.py -j13 -t /Volumes/tmp I get the warning up front and all tests are executed.

Temporary test directory at /Volumes/tmp/test_runner_₿_🏃_20240402_192153
WARNING! There may be insufficient free space in /Volumes/tmp/test_runner_₿_🏃_20240402_192153 to run the Bitcoin functional test suite. Running the test suite with fewer than 2326.0 MB of free space might cause tests to fail.

Kept reducing the available space in the /Volumes/tmp partition, and a test eventually fails at 1.8 GB of free space

Filesystem      Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk4s1   1.8Gi    12Mi   1.8Gi     1%      13  4.3G    0%   /Volumes/tmp

with the following error:

Early exiting after test failure due to disk running out of space in /Volumes/tmp/test_runner_₿_🏃_20240402_202655

--nocleanup

Testing the with the ./test/functional/test_runner.py -j13 -t /Volumes/tmp --nocleanup requirement for a minimum of 12 GB, with a partition of 11 GB and the df -h displaying:

Filesystem      Size    Used   Avail Capacity iused ifree %iused  Mounted on
/dev/disk4s2    11Gi    27Mi    11Gi     1%       4  4.3G    0%   /Volumes/tmp

I get the expected insufficient free space warning and all tests are executed.

WARNING! There may be insufficient free space in /Volumes/tmp/test_runner_₿_🏃_20240402_203805 to run the functional test suite with --nocleanup. A minimum of 12 GB of free space is required.

Tested down to at 5.5 GB and eventually a test fails halting the test runner as well.

Early exiting after test failure due to disk running out of space in /Volumes/tmp/test_runner_₿_🏃_20240402_210436

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

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

Nice work. Adding free storage awareness is a great addition.

ACK for 2f7fcd5.
Left a nit inline.

Used a small ramdisk (1.2GB) with high jobs count (16) to force space exhaustion. As expected, received the initial warning Running the test suite with fewer than 2626.0 MB of free space might cause tests to fail. and also received the exiting error Early exiting after test failure due to disk running out of space...

test/functional/test_runner.py Outdated Show resolved Hide resolved
fanquake added a commit that referenced this pull request Apr 9, 2024
…not exist

d4e36ae test: Update --tmpdir doc string to say directory must not exist (kevkevin)

Pull request description:

  The error message given if passing an existing dir to --tmpdir is confusing so this makes it clear that the directory must not already exist

  This change is motivated by this comment #29335 (comment)

ACKs for top commit:
  maflcko:
    lgtm ACK d4e36ae
  davidgumberg:
    ACK d4e36ae

Tree-SHA512: fb31fd079767abbf94076615817943f35f5c9262fc97e65c631a18d33b3a343fe6a2d151613256e632d2b372ab2de0435f4712309b4a77ed3c663fd93a7dcdd1
@BrandonOdiwuor
Copy link
Contributor Author

Could you please take another look at this after incorporating the suggested changes

@achow101 @maflcko @kevkevinpal @davidgumberg @epiccurious @tdb3 @itornaza

@itornaza
Copy link

tested re-ACK for 858fa78

No size restrictions

  • Did a code review and checked that there is adequate space on the file system to run the tests
  • Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
  • Ran unit tests with make check and all tests pass
  • Ran all functional tests with no extra flags, all tests pass and no warnings about free space are displayed
  • Ran all functional tests with --extended and all tests pass as well

Limited test space enforced using a ramdisk

Ran the functional tests with the following options:

  • ./test/functional/test_runner.py -t /Volumes/ramdisk with a ramdisk of 512 MB
  • ./test/functional/test_runner.py -t /Volumes/ramdisk --nocleanup with a ramdisk of 4 GB

The expected warnings are displayed upfront:

WARNING! There may be insufficient free space in /Volumes/ramdisk/test_runner_₿_🏃_20240411_211144 to run the Bitcoin functional test suite. Running the test suite with fewer than 1426.0 MB of free space might cause tests to fail.

and for the --nocleanup case:

WARNING! There may be insufficient free space in /Volumes/ramdisk/tmp/test_runner_₿_🏃_20240411_213828 to run the functional test suite with --nocleanup. A minimum of 12 GB of free space is required.

eventually when a test fails due to lack of disk space, the test harness exits and the new error message is displayed and is indeed much more informative!

Early exiting after test failure due to insuffient free space in /Volumes/ramdisk/tmp/test_runner_₿_🏃_20240411_212332
Test execution data left in /Volumes/ramdisk/tmp/test_runner_₿_🏃_20240411_212332.
Additional storage is needed to execute testing.

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

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

re-ACK for 858fa78

Thanks for incorporating the suggestion.

Used a small ramdisk (1.2GB) with high jobs count (20) to force space exhaustion. As expected, received the initial warning Running the test suite with fewer than 3026.0 MB of free space might cause tests to fail. and also received the exiting error Early exiting after test failure due to disk running out of space...Test execution data left in ...Additional storage is needed to execute testing with the new text added.

@davidgumberg
Copy link
Contributor

davidgumberg commented Apr 13, 2024

reACK 858fa78

Looks great, tested on a tmpfs 1 GiB in size, and the test runner prints the intended warning, and fails immediately when a test fails due to disk full.

@achow101
Copy link
Member

ACK 7c0c599

Copy link
Contributor

@cbergqvist cbergqvist left a comment

Choose a reason for hiding this comment

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

reACK 357ad11. Looks good!

Tested on RAM disk of 1GB, 1430MB (doas mount -t tmpfs -o size=1430m tmpfs /mnt/tmp/) and varied the --jobs and --nocleanup, observing expected warnings + error on disk full.

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

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

re ACK for 357ad11

Retested #29335 (review) (and received the same results, which warn the user of insufficient free space and early exit due to insufficient space).

@itornaza
Copy link

itornaza commented May 4, 2024

re-ACK 357ad11

Ran again all the tests as in #29335 (comment) and I get the same results as expected on disk space in the cases it is being insufficient.

@achow101
Copy link
Member

achow101 commented May 8, 2024

reACK 357ad11

@achow101 achow101 merged commit 8a45f57 into bitcoin:master May 8, 2024
16 checks passed
@BrandonOdiwuor BrandonOdiwuor deleted the test-disk-full branch May 9, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle disk-full more gracefully in functional tests