test: enable reindex readonly test on *BSD #28660

pinheadmz · 2023-10-16T20:57:56Z

OpenBSD and FreeBSD don't have chattr but they do have chflags, use that method to make the block file immutable for the reindex_readonly test.

Written and tested on a VPS running FreeBSD:

FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64

DrahtBot · 2023-10-16T20:57:59Z

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	maflcko, theStack, jonatack

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:

#28116 (test: update tool_wallet coverage for unexpected writes to wallet by jonatack)

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.

jonatack

Concept ACK. I'm extracting this to a test utility in #28116, if by any chance you prefer to build on that and I'll pull in the commit there.

test/functional/feature_reindex_readonly.py

theStack · 2023-10-16T23:49:38Z

Concept ACK, will test on OpenBSD 7.4 in a bit.

maflcko

lgtm ACK

maflcko · 2023-10-17T10:37:16Z

test/functional/feature_reindex_readonly.py

@@ -35,6 +35,7 @@ def reindex_readonly(self):
        filename.chmod(stat.S_IREAD)

        used_chattr = False
+        used_chflags = False


Nit: Could replace the two boolean with a single undo_immutable, which holds an optional lambda to run. (a no-op by default, and set to None if the test should terminate early, and the respective undo command if an util helper was used?)

This should reduce the number of symbols, and reduce the mix of if-else vs early-return. The diff below would look like:

diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py index 26531f472b..77716b371e 100755 --- a/test/functional/feature_reindex_readonly.py +++ b/test/functional/feature_reindex_readonly.py @@ -53,12 +53,11 @@ class BlockstoreReindexTest(BitcoinTestFramework): return self.log.debug("Attempt to restart and reindex the node with the unwritable block file") - with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]): + if undo_immutable: + with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]): self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'], expected_msg="Error: A fatal internal error occurred, see debug.log for details") - - if used_chattr: - subprocess.check_call(['chattr', '-i', filename]) + undo_immutable() filename.chmod(0o777)

maflcko · 2023-10-17T14:54:41Z

test/functional/feature_reindex_readonly.py

+                    expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+            undo_immutable()
+        else:
+            return


nit: I think you can now remove the early return, because it should be fine and clearer to run the next line unconditionally.

heh thanks. Having an issue on macos now...

heh thanks. Having an issue on macos now...

The last push (5797225) seems to run fine on arm64 macOS for me.

macOS should be unaffected by this and execute lambda: None, no?

@jonatack as root?

if platform.system() in ["FreeBSD", "OpenBSD", "Darwin"]: there we go ;-)

This is very confusing. The last commit should be a refactor. And the first commit shouldn't affect macOS. This is a bug on master already, right?

Latest push e7db19f works for me as root.

@maflcko the first commit does not affect macos. the second commit is a refactor that also fixes a macos/root bug on master. Want me to separate the macos out to commit 3?

@maflcko yes, running this test as root blocks for me on master as well.

maflcko

lgtm ACK

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

maflcko · 2023-10-17T15:11:41Z

test/functional/feature_reindex_readonly.py

-            self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
-                expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+                    undo_immutable = False
+        if platform.system() in ["FreeBSD", "OpenBSD", "Darwin"]:


What about netbsd? I think you can just run if chflags --version or similar here?

pinheadmz · 2023-10-17T15:24:37Z

squashed and replaced all the platform queries with try/catch tests for the commands themselves.

maflcko · 2023-10-20T09:07:39Z

lgtm code ACK cbd5bf0

theStack · 2023-10-20T11:39:30Z

Is there any good reason to abuse try/catch for regular logic control flow, in this case the OS detection (previously done with platform.system(), which makes much more sense IMHO)? For example, I'm running OpenBSD 7.4 with the package e2fsprogs installed, which has a binary /usr/local/bin/chattr installed, so the Linux branch is executed. That's a bit unexpected, though on the other hand one could argue it's fine, as the test still succeeds and serves its purpose. At least it works as long as we only need the binaries and there is no other OS-specific tests going on inside the branches.

pinheadmz · 2023-10-20T11:58:18Z

@theStack the issue was hard-coding all the OSs. Every flavor of *BSD plus macOS has chattr, Linux has chflags and Windows, well... chmod seems to work against root on Windows. The test isn't trying to assert that certain behaviors are expected on certain OSes, we just want that file to be immutable "by any means necessary"

maflcko · 2023-10-20T12:03:14Z

previously done with platform.system(), which makes much more sense IMHO

Not sure. This would force us to maintain a list of all operating systems in this test. FreeBSD, NetBSD, OpenBSD, macOS, ...
Seems easier to just write code that only checks for the program the test needs?

maflcko · 2023-10-20T12:04:39Z

I'm running OpenBSD 7.4 with the package e2fsprogs installed, which has a binary /usr/local/bin/chattr installed, so the Linux branch is executed.

If this package can be installed on all BSD and macOS, I think we can also remove the BSD branch, and just require this package?

pinheadmz · 2023-10-20T12:18:17Z

I don't think chattr is available for macOS

pinheadmz · 2023-10-20T12:19:34Z

But, we could make this a utility function that returns an "undo" lambda? And then get the weird code out of the test itself?

theStack · 2023-10-20T12:29:06Z

@theStack the issue was hard-coding all the OSs. Every flavor of *BSD plus macOS has chattr, Linux has chflags and Windows, well... chmod seems to work against root on Windows. The test isn't trying to assert that certain behaviors are expected on certain OSes, we just want that file to be immutable "by any means necessary"

previously done with platform.system(), which makes much more sense IMHO

Not sure. This would force us to maintain a list of all operating systems in this test. FreeBSD, NetBSD, OpenBSD, macOS, ... Seems easier to just write code that only checks for the program the test needs?

Yeah, fair enough, I agree that maintaining this list is also not ideal, and just checking for the programs we need is fine. What I particularly don't like about catching the generic Exception as control flow instrument is that there are a whole lot of other reasons why exceptions could be thrown (when something actually goes wrong, like out of disk space/memory, or whatever else), and then we wouldn't even notice it, as in the end everything is swallowed by the final except Exception: pass and the test just passes fine. That seems a bit fragile. But no strong opinion, I'm happy to ACK once it works on OpenBSD (see below).

theStack · 2023-10-20T12:02:56Z

test/functional/feature_reindex_readonly.py

-        with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
-            self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
-                expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+                    undo_immutable = False


nit, might be good not to mix data types (probably a leftover from an earlier version of the PR?) :

Suggested change

undo_immutable = False

undo_immutable = lambda: None

(also in the macOS/BSD branch below)

I don't think this works. This will break running the test in a Linux container without --cap-add LINUX_IMMUTABLE, no?

See also the log line immediately above this line.

Oops, indeed. In my mind lambda: None was falsy and thought the if condition below wouldn't be executed, my bad.

It may be possible to type lambda: pass, which should be equal to lambda: None?

It may be possible to type lambda: pass, which should be equal to lambda: None?

Unfortunately not:

$ python3 Python 3.10.11 (main, Apr 13 2023, 01:52:20) [Clang 13.0.0 ] on openbsd7 Type "help", "copyright", "credits" or "license" for more information. >>> x = lambda: pass File "<stdin>", line 1 x = lambda: pass ^^^^ SyntaxError: invalid syntax

I wonder what type checkers like mypy would consider as the correct falsy value for a lambda. Maybe None would be slightly better than False? (or maybe both would be rejected, idk). Anyways, False seems to do its job.

test/functional/feature_reindex_readonly.py

theStack · 2023-10-20T12:57:22Z

Here's some more practical example why catching the generic Exception is not a good idea. With the following patch, the test passes just fine, as we don't notice the error which would normally be thrown if we try to call a method that doesn't exist:

diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
index b276be9c03..9f3b642338 100755
--- a/test/functional/feature_reindex_readonly.py
+++ b/test/functional/feature_reindex_readonly.py
@@ -36,7 +36,7 @@ class BlockstoreReindexTest(BitcoinTestFramework):
         undo_immutable = lambda: None
         # Linux
         try:
-            subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+            subprocess.run_thismethoddoesnotevenexist(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
             try:
                 subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
                 undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename])

This should throw an AttributeError: module 'subprocess' has no attribute 'run_thismethoddoesnotevenexist', and it indeed does, but we don't notice it, as we fall in the except Exception branch. But enough ranting about that topic from my side for now :)

maflcko · 2023-10-23T09:35:48Z

This should be rfm, except for https://github.com/bitcoin/bitcoin/pull/28660/files#r1366893829, which can either be done here, or I'd do in a follow-up?

maflcko · 2023-10-23T15:07:06Z

re-cr-lgtm-ACK 5a0688a

theStack

ACK 5a0688a

Tested in OpenBSD 7.4, once with and once without the e2fsprogs package installed, to trigger both code paths:

$ which chattr && which chflags
/usr/local/bin/chattr
/usr/bin/chflags
$ ./test/functional/feature_reindex_readonly.py
2023-10-23T15:41:09.438000Z TestFramework (INFO): PRNG seed is: 435157676393106655
2023-10-23T15:41:09.493000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_2uz_nii5
2023-10-23T15:41:10.354000Z TestFramework (INFO): Made file immutable with chattr
2023-10-23T15:41:10.363000Z TestFramework (INFO): Attempt to restart and reindex the node with the unwritable block file
2023-10-23T15:41:10.966000Z TestFramework (INFO): Stopping nodes
2023-10-23T15:41:10.970000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_2uz_nii5 on exit
2023-10-23T15:41:10.971000Z TestFramework (INFO): Tests successful

$ doas pkg_delete e2fsprogs
...
$ which chattr && which chflags
which: chattr: Command not found.
$ ./test/functional/feature_reindex_readonly.py
2023-10-23T15:43:04.599000Z TestFramework (INFO): PRNG seed is: 8556685811226418814
2023-10-23T15:43:04.653000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_h4lvamww
2023-10-23T15:43:05.565000Z TestFramework (INFO): Made file immutable with chflags
2023-10-23T15:43:05.622000Z TestFramework (INFO): Attempt to restart and reindex the node with the unwritable block file
2023-10-23T15:43:06.231000Z TestFramework (INFO): Stopping nodes
2023-10-23T15:43:06.295000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_h4lvamww on exit
2023-10-23T15:43:06.362000Z TestFramework (INFO): Tests successful

jonatack · 2023-10-23T18:31:12Z

I agree with @theStack's concerns about catching the generic exception for control flow and would be happy to re-review if this is reverted to platform() queries. That said, I needed to refactor this a fair amount already to extract it in #28116, so I suppose it can be improved there.

ACK 5a0688a tested on macOS only

mzumsande · 2023-10-25T19:42:02Z

test/functional/feature_reindex_readonly.py

+        undo_immutable = lambda: None
+        # Linux
+        try:
+            subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
            try:
                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)


On Ubuntu 22.04.3 LTS, this line throws an exception for me

2023-10-25T19:35:49.783000Z TestFramework (DEBUG): Make the first block file read-only 2023-10-25T19:35:49.785000Z TestFramework (WARNING): Command '['chattr', '+i', PosixPath('/tmp/bitcoin_func_test_i3g4dkjv/node0/regtest/blocks/blk00000.dat')]' returned non-zero exit status 1. 2023-10-25T19:35:49.785000Z TestFramework (WARNING): stderr: b'chattr: Operation not permitted while setting flags on /tmp/bitcoin_func_test_i3g4dkjv/node0/regtest/blocks/blk00000.dat\n'

In spite of this, the file is still changed to readonly, so the test still works as intended (it just doesn't set undo_immutable in the next line because of the exception).

Yes, but this seems unrelated to the changes here, and the warning can be ignored. I guess it should just be a debug or info level?

maflcko · 2023-10-24T11:19:54Z

test/functional/feature_reindex_readonly.py

-        used_chattr = False
-        if platform.system() == "Linux":
+        undo_immutable = lambda: None
+        # Linux


nit: I guess the comment isn't applicable, given that chattr can be installed on FreeBSD

Yes, the comment is already removed in #28116 (which I'll be updating).

maflcko · 2023-10-25T19:44:49Z

test/functional/feature_reindex_readonly.py

+        undo_immutable = lambda: None
+        # Linux
+        try:
+            subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
            try:
                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)


Yes, but this seems unrelated to the changes here, and the warning can be ignored. I guess it should just be a debug or info level?

5a0688a test: enable reindex readonly test on *BSD and macOS as root (Matthew Zipkin) Pull request description: see bitcoin#27850 (comment) OpenBSD and FreeBSD don't have `chattr` but they do have `chflags`, use that method to make the block file immutable for the reindex_readonly test. Written and tested on a VPS running FreeBSD: ``` FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64 ``` ACKs for top commit: maflcko: re-cr-lgtm-ACK 5a0688a jonatack: ACK 5a0688a tested on macOS only theStack: ACK 5a0688a Tree-SHA512: 8c88d282d09c00355d22c4c504b779f60e420327a5e07bcf80fa77b97fefcb04952af9ceaf439d9033a0a2448cb26a02663fe6bddcd4a74792857cfbaf1c5162

DrahtBot added the Tests label Oct 16, 2023

jonatack reviewed Oct 16, 2023

View reviewed changes

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

jonatack reviewed Oct 16, 2023

View reviewed changes

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

DrahtBot mentioned this pull request Oct 16, 2023

test: update tool_wallet coverage for unexpected writes to wallet #28116

Closed

maflcko approved these changes Oct 17, 2023

View reviewed changes

maflcko reviewed Oct 17, 2023

View reviewed changes

pinheadmz force-pushed the readonly-bsd branch 2 times, most recently from 5797225 to e7db19f Compare October 17, 2023 15:05

maflcko approved these changes Oct 17, 2023

View reviewed changes

pinheadmz force-pushed the readonly-bsd branch 2 times, most recently from 693349c to 98f108b Compare October 17, 2023 15:23

pinheadmz force-pushed the readonly-bsd branch from 98f108b to db03177 Compare October 17, 2023 15:26

DrahtBot added the CI failed label Oct 17, 2023

pinheadmz force-pushed the readonly-bsd branch from db03177 to cbd5bf0 Compare October 17, 2023 16:57

DrahtBot removed the CI failed label Oct 17, 2023

DrahtBot requested review from jonatack and theStack October 20, 2023 09:07

maflcko added this to the 26.0 milestone Oct 20, 2023

theStack reviewed Oct 20, 2023

View reviewed changes

test: enable reindex readonly test on *BSD and macOS as root

5a0688a

pinheadmz force-pushed the readonly-bsd branch from cbd5bf0 to 5a0688a Compare October 23, 2023 14:59

DrahtBot requested a review from theStack October 23, 2023 15:07

theStack approved these changes Oct 23, 2023

View reviewed changes

DrahtBot added the CI failed label Oct 23, 2023

DrahtBot removed the request for review from jonatack October 23, 2023 18:38

maflcko assigned fanquake Oct 24, 2023

fanquake merged commit ab61087 into bitcoin:master Oct 24, 2023
15 of 16 checks passed

pinheadmz mentioned this pull request Oct 24, 2023

blockstorage: do not flush block to disk if it is already there #27039

Merged

mzumsande reviewed Oct 25, 2023

View reviewed changes

maflcko reviewed Oct 25, 2023

View reviewed changes

test: enable reindex readonly test on *BSD #28660

test: enable reindex readonly test on *BSD #28660

Conversation

pinheadmz commented Oct 16, 2023

DrahtBot commented Oct 16, 2023 • edited

Code Coverage

Reviews

Conflicts

jonatack left a comment

Choose a reason for hiding this comment

theStack commented Oct 16, 2023

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko Oct 17, 2023 • edited

Choose a reason for hiding this comment

pinheadmz commented Oct 17, 2023

maflcko commented Oct 20, 2023

theStack commented Oct 20, 2023

pinheadmz commented Oct 20, 2023

maflcko commented Oct 20, 2023

maflcko commented Oct 20, 2023

pinheadmz commented Oct 20, 2023

pinheadmz commented Oct 20, 2023

theStack commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theStack Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theStack commented Oct 20, 2023

maflcko commented Oct 23, 2023

maflcko commented Oct 23, 2023

theStack left a comment

Choose a reason for hiding this comment

jonatack commented Oct 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Oct 16, 2023 •

edited

jonatack Oct 17, 2023 •

edited

maflcko Oct 17, 2023 •

edited

theStack Oct 20, 2023 •

edited

jonatack commented Oct 23, 2023 •

edited