Abort functional tests early upon low drive space #23100

katesalazar · 2021-09-26T15:20:35Z

This change rushes a fix for #23099.

This approach works for test/functional/test_runner.py in my system,
changes to test/fuzz/test_runner.py are untested at the time of writing
(I don't know how to run this command).

laanwj · 2021-09-27T10:49:50Z

Concept ACK

test/fuzz/test_runner.py

test/functional/test_runner.py

meshcollider · 2021-10-04T02:24:34Z

@katesalazar just a few notes:

Please squash your commits
MIN_FREE_SPACE_BEFORE_TEST is currently unused, so that needs to be corrected (unless you decide to remove it - but it think it is better to check there is enough space before starting each test, rather than checking after each).
I agree with Marco's comments about the 16M (and note that he is the test framework maintainer, so please do take his advice seriously 🙂)

katesalazar · 2021-10-04T05:19:20Z

@meshcollider wrote:

I agree with Marco's comments about the 16M (and note that he is the test framework maintainer, so please do take his advice seriously 🙂)

Does agreeing come simply from trusting Marco, or
does it come from real positive a posteriori knowledge of the technology,
you have?

maflcko · 2021-10-04T07:03:53Z

Again, no one asked you to trust me. Everything is in the source code, which you can read or execute yourself: #23100 (comment)

katesalazar · 2021-10-04T08:29:51Z

You keep not proving your point,
I am not applying your suggestion myself.
It's better you commit your unproven magic numbers under your name
in a subsequent PR or an alternative PR.

fanquake · 2021-10-05T06:02:51Z

I am not applying your suggestion myself.
It's better you commit your unproven magic numbers under your name

That's fine, you don't have to do anything you don't want to. However, this Pull Request is not going to be merged with the review comments left unaddressed, or the TODO left in the code. So you either need to address everything, or you can close this PR.

katesalazar · 2021-10-05T06:45:51Z

I've watched example_test.py failing to run successfully,
apparently refusing to start under ~10M free space.
It's likely I'll investigate and end up adding Marco's suggestion.

katesalazar · 2021-10-05T07:10:41Z

See also:

bitcoin/src/util/system.cpp

Line 146 in 9e530c6

constexpr uint64_t min_disk_space = 52428800; // 50 MiB

In:

bitcoin/src/util/system.cpp

Lines 144 to 150 in 9e530c6

    
           bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes) 
        
           { 
        
               constexpr uint64_t min_disk_space = 52428800; // 50 MiB 
        
               uint64_t free_bytes_available = fs::space(dir).available; 
        
               return free_bytes_available >= min_disk_space + additional_bytes; 
        
           }

katesalazar · 2021-10-05T07:17:09Z

I think it can refuse to boot with 'Disk space too low' with more than
16M free, tried using 30M, it seems it refused to boot.

test/functional/test_runner.py

fanquake · 2021-10-07T05:33:33Z

test/functional/test_runner.py

@@ -27,6 +27,11 @@
 import logging
 import unittest

+# Should (in general but not always) fail testing and immediately stop if free space falls below.
+# Constant value 50M comes from `bool CheckDiskSpace(2)` at
+# `src/util/system.h` via uses in `src/node/blockstorage.cpp`.


Please don't put source file names / C++ implementation details here, as they are likely to just become outdated in regards to the code. Also, comments like "in general but not always" are not really useful, unless they are elaborated on (and that would be overkill here), as the reader is just left wondering about the "but not always" conditions.

meshcollider · 2021-10-08T12:08:27Z

Please squash your commits again :)

It is good practice to squash after every fixup so that the PR is immediately ready to review and merge. Otherwise, squashing later will invalidate the ACKs on that commit hash.

yanmaani · 2021-10-08T15:26:55Z

Conceptually it'd be cleaner if you could detect Bitcoin Core complaining about free space once and then abort the test, but I don't know if that's feasible.

katesalazar · 2021-10-08T20:52:07Z

It is good practice to squash after every fixup so that the PR is immediately ready to review and merge. Otherwise, squashing later will invalidate the ACKs on that commit hash.

Understood and grabbed this advice for urgent changes.
Which you won't see me taking care of in the next maybe 5 or 10 years,
for reasons.

I'll squash on demand, because that's the rules.
But squashing is a lossy* process, on Git and on GitHub, and I don't think
I should be doing it lightly.

* Let's imagine for a while the references log does not exist.

Conceptually it'd be cleaner if you could detect Bitcoin Core complaining about free space once and then abort the test, but I don't know if that's feasible.

Could spawn a thread which monitors outputs looking for "Disk space too low".
Probably that output and more cases, "No space left on drive" (IDK exactly)...

That is a great idea if a test can take a very long time to naturally abort once
is theoretically clear it will fail. I would doubt that's the case, but IDK.

katesalazar · 2021-10-08T21:07:50Z

BTW I very much appreciate your patience with my
bi-polar bear things, thank you.

katesalazar · 2021-10-09T15:19:14Z

dc43901
is an amend of (only) the commit message of
646029c
(which passed CI checks).

meshcollider · 2021-10-10T10:25:35Z

But squashing is a lossy* process, on Git and on GitHub, and I don't think I should be doing it lightly.

You should, because in general people don't review PRs that look WIP - and unsquashed fixup commits immediately signal WIP. PRs are required to be clean and tidy for merge. There's no need to worry about loss because the new commit should always be an improvement over the old. Feel free to keep a backup of your old commits on a separate branch locally though if you want to.

katesalazar · 2021-10-19T07:13:05Z

@meshcollider wrote:

utACK dc43901

I pushed 4b41e9d on top of dc43901.

I don't know what's the software allowing to push an
4b41e9d ACK (would that be the case) on top of an
dc43901 ACK, more importantly I don't know what's the
software allowing to automatically drafting a replay of an
4b41e9d ACK (would that be the case) on the rev
resulting from squashing 4b41e9d in dc43901.

As the code should be the same and only the commit message should
have to be reviewed, a tool should automatically draft such ACK on the
squashed rev for accelerating the review process.

If that's a manual process as I suspect, then this tool fails to provide
the features this project needs.

fanquake · 2021-10-19T07:16:36Z

@katesalazar I don't understand your previous comment. In any case, you need to squash your commits. You should just be doing that by default.

katesalazar · 2021-10-19T07:28:59Z

@fanquake wrote:

I don't understand your previous comment.

Don't worry, I'm surely missing some task you do when you review.

Let's suppose there is a rev A changing 1000 lines, it's acknowledged.
A change happens and rev B has to be submitted changing 1 single line.

Submitting B on top of A is straight forward and safe.

You want me to directly push B fixed up in A, i.e. rev C, so that
C(codebase) = B(A(codebase)) = (B∘A)(codebase).

I just need you to teach me what's the command that tells you that
I didn't sneak a rogue change in C changing some 10-ish lines unexpectedly.

How do you compare A to C and get the equivalent to B without me even
ever pushing B?

katesalazar · 2021-10-19T07:33:43Z

How do you compare A to C and get the equivalent to B without me even
ever pushing B?

Clarification,
I know git diff A C gives it; but my question is, do you really do this to
see B? Or do you just read all over all of the changes in A once again
when C is pushed?

fanquake · 2021-10-19T07:35:35Z

You probably want git range-diff. I'd suggest reading our productivity docs.

katesalazar · 2021-10-19T09:14:12Z

You probably want git range-diff. I'd suggest reading our productivity docs.

Oh, we stand on the shoulders of giants!

But even if range-diff works, how would you make sure that the old
feature branch tip revision (rev) isn't removed by the Git garbage
collector when you finally range-diff a feature branch tip against
the previously reviewed rev which is no longer part of a branch AKA
reference (ref)?

A reviewed rev is worth a lot more an asset than an unreviewed rev.
You would want to make sure that the garbage collector doesn't
remove a reviewed rev orphaned from a reference until the new tip
is reviewed.

Don't you need to reconfigure the references log or the garbage
collector so that they use a larger buffer or timeframe?

Or what you do is to just never update your local copy of a feature
branch ref unless you are going to review the new feature branch
tip right away, hence the previously reviewed rev orphaned from
the pull request ref becomes obsolete and discardable in the single
step of reviewing the new feature branch tip?

maflcko · 2021-10-19T09:39:21Z

You can create a branch or tag to make sure git doesn't gc it

katesalazar · 2021-10-19T09:51:32Z

Understood. Good enough, thanks.

Sjors · 2021-12-09T10:45:43Z

Concept ACK. I tend to run functional tests on a RAM disk which occasionally runs out of space, resulting in weird behavior. Hopefully this PR helps.

katesalazar · 2021-12-10T20:15:45Z

save dev time good :) code review ack is invested time ;D thanks for the necromancy, cheers

…

On Thu, Dec 9, 2021 at 11:45 AM Sjors Provoost ***@***.***> wrote: Concept ACK. I tend to run functional tests on a RAM disk which occasionally runs out of space, resulting in weird behavior. Hopefully this PR helps. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMRS4WZBYMCU4X3DRKAEJRDUQCCGHANCNFSM5EY4R27A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

katesalazar · 2022-01-22T07:39:42Z

test/functional/test_runner.py

-            shutil_disk_usage_stat_ = shutil.disk_usage(testdir)
+            try:  # XXX
+              shutil_disk_usage_stat_ = shutil.disk_usage(testdir)
+            except FileNotFoundError:  # XXX


have to investigate why this fixed CI

As an exception, last functional tests to be run do not impose drive space requirements. Some neighboring lines do not follow PEP8 strictly, so neither does some of the lines in this change. Includes many suggestions courtesy of MarcoFalke and fanquake and theStack.

This reverts commit 4bc38b1.

This reverts commit 4af6367.

fanquake · 2022-04-26T14:24:32Z

have to investigate why this fixed CI

What is the status of this? Are you still working on it? It's not mergable in it's current state.

katesalazar · 2022-04-26T14:50:47Z

What is the status of this? Are you still working on it?

This was finished at some point, but I failed to keep up
with changes in the master branch.

It's not mergable in it's current state.

I don't have the time, hardware and drive I'd want
in order to clean this PR up (4bc38b1)
but I'm happy to let it go whenever you need so.

fanquake · 2022-04-26T14:56:46Z

I don't have the time, hardware and drive I'd want
in order to clean this PR up

That's fine. We'll close it as "Up for Grabs".

DrahtBot added the Tests label Sep 26, 2021

maflcko reviewed Sep 27, 2021

View reviewed changes

test/fuzz/test_runner.py Outdated Show resolved Hide resolved

maflcko reviewed Sep 27, 2021

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

maflcko reviewed Sep 27, 2021

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

maflcko reviewed Sep 29, 2021

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

katesalazar changed the title ~~Abort tests early upon filling the test logs drive~~ Abort functional tests early upon low drive space Oct 5, 2021

katesalazar force-pushed the 20210926 branch 2 times, most recently from cd1b1dc to 471b970 Compare October 5, 2021 08:22

maflcko reviewed Oct 5, 2021

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

katesalazar force-pushed the 20210926 branch from 471b970 to 86c6620 Compare October 5, 2021 09:30

maflcko reviewed Oct 5, 2021

View reviewed changes

test/functional/test_runner.py Show resolved Hide resolved

katesalazar mentioned this pull request Oct 5, 2021

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

fanquake reviewed Oct 7, 2021

View reviewed changes

katesalazar force-pushed the 20210926 branch from 8095f1f to 646029c Compare October 8, 2021 21:03

katesalazar force-pushed the 20210926 branch from 646029c to dc43901 Compare October 9, 2021 06:32

katesalazar force-pushed the 20210926 branch from 4b41e9d to 58e1ff6 Compare October 19, 2021 07:52

katesalazar force-pushed the 20210926 branch from 58e1ff6 to 24627a1 Compare January 9, 2022 11:34

katesalazar marked this pull request as draft January 21, 2022 08:10

katesalazar force-pushed the 20210926 branch 3 times, most recently from d30a78e to 2c98a20 Compare January 21, 2022 13:30

katesalazar commented Jan 22, 2022

View reviewed changes

katesalazar added 2 commits January 22, 2022 15:11

XXX must investigate why this fixes CI XXX

4bc38b1

katesalazar force-pushed the 20210926 branch from 2c98a20 to 4bc38b1 Compare January 22, 2022 16:20

katesalazar added 2 commits January 23, 2022 23:11

Revert "XXX must investigate why this fixes CI XXX"

4af6367

This reverts commit 4bc38b1.

Revert "Revert "XXX must investigate why this fixes CI XXX""

3a1e705

This reverts commit 4af6367.

katesalazar force-pushed the 20210926 branch from 6552748 to 3a1e705 Compare January 24, 2022 07:54

fanquake added the Up for grabs label Apr 26, 2022

fanquake closed this Apr 26, 2022

bitcoin locked and limited conversation to collaborators Apr 26, 2023

maflcko removed the Up for grabs label Jan 29, 2024

Abort functional tests early upon low drive space #23100

Abort functional tests early upon low drive space #23100

Uh oh!

Conversation

katesalazar commented Sep 26, 2021

Uh oh!

laanwj commented Sep 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meshcollider commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katesalazar commented Oct 4, 2021

Uh oh!

maflcko commented Oct 4, 2021

Uh oh!

katesalazar commented Oct 4, 2021

Uh oh!

fanquake commented Oct 5, 2021

Uh oh!

katesalazar commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katesalazar commented Oct 5, 2021

Uh oh!

katesalazar commented Oct 5, 2021

Uh oh!

Uh oh!

Uh oh!

fanquake Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Oct 8, 2021

Uh oh!

yanmaani commented Oct 8, 2021

Uh oh!

katesalazar commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katesalazar commented Oct 8, 2021

Uh oh!

katesalazar commented Oct 9, 2021

Uh oh!

meshcollider commented Oct 10, 2021

Uh oh!

katesalazar commented Oct 19, 2021

Uh oh!

fanquake commented Oct 19, 2021

Uh oh!

katesalazar commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katesalazar commented Oct 19, 2021

Uh oh!

fanquake commented Oct 19, 2021

Uh oh!

katesalazar commented Oct 19, 2021

Uh oh!

maflcko commented Oct 19, 2021

Uh oh!

katesalazar commented Oct 19, 2021

Uh oh!

Sjors commented Dec 9, 2021

Uh oh!

katesalazar commented Dec 10, 2021 via email

Uh oh!

katesalazar Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake commented Apr 26, 2022

Uh oh!

katesalazar commented Apr 26, 2022

Uh oh!

fanquake commented Apr 26, 2022

Uh oh!

Uh oh!

meshcollider commented Oct 4, 2021 •

edited

Loading

katesalazar commented Oct 5, 2021 •

edited

Loading

katesalazar commented Oct 8, 2021 •

edited

Loading

katesalazar commented Oct 19, 2021 •

edited

Loading