see #27850 (review)
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:
0FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
51@@ -51,6 +52,20 @@ def reindex_readonly(self):
52 self.log.warning("This should only happen due to missing capabilities in a container.")
53 self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
54 return
55+ if platform.system() == "FreeBSD" or platform.system() == "OpenBSD":
import platform; print(platform.system()), but how did you check for both platforms?
73@@ -59,6 +74,8 @@ def reindex_readonly(self):
74
75 if used_chattr:
76 subprocess.check_call(['chattr', '-i', filename])
77+ if used_chflags:
78+ subprocess.check_call(['chflags', 'nouchg', filename])
uchg and nouchg appear to be the correct values for “set the user immutable flag (owner or superuser only)” and “clear the immutable bit”, respectively.
34@@ -35,6 +35,7 @@ def reindex_readonly(self):
35 filename.chmod(stat.S_IREAD)
36
37 used_chattr = False
38+ 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:
0diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
1index 26531f472b..77716b371e 100755
2--- a/test/functional/feature_reindex_readonly.py
3+++ b/test/functional/feature_reindex_readonly.py
4@@ -53,12 +53,11 @@ class BlockstoreReindexTest(BitcoinTestFramework):
5 return
6
7 self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
8- with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
9+ if undo_immutable:
10+ with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
11 self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
12 expected_msg="Error: A fatal internal error occurred, see debug.log for details")
13-
14- if used_chattr:
15- subprocess.check_call(['chattr', '-i', filename])
16+ undo_immutable()
17
18 filename.chmod(0o777)
19
79+ with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
80+ self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
81+ expected_msg="Error: A fatal internal error occurred, see debug.log for details")
82+ undo_immutable()
83+ else:
84+ return
heh thanks. Having an issue on macos now…
The last push (57972250feb3ad61b91c8fbcb01eac1b4b57095f) seems to run fine on arm64 macOS for me.
lambda: None, no?
sudo test/functional/feature_reindex_readonly.py stalls.
yes, macos as root on master doesnt work. However, this will:
if platform.system() == "FreeBSD" or platform.system() == "OpenBSD" or platform.system() == "Darwin":
whats the best style for a triple - or in python?
if platform.system() in ["FreeBSD", "OpenBSD", "Darwin"]: there we go ;-)
55- self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
56- with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
57- self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
58- expected_msg="Error: A fatal internal error occurred, see debug.log for details")
59+ undo_immutable = False
60+ if platform.system() in ["FreeBSD", "OpenBSD", "Darwin"]:
if chflags --version or similar here?
lgtm ACK
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
platform queries with try/catch tests for the commands themselves.
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.
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”
55-
56- self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
57- with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
58- self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
59- expected_msg="Error: A fatal internal error occurred, see debug.log for details")
60+ undo_immutable = False
nit, might be good not to mix data types (probably a leftover from an earlier version of the PR?) :
0 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.
lambda: None was falsy and thought the if condition below wouldn’t be executed, my bad.
lambda: pass, which should be equal to lambda: None?
It may be possible to type
lambda: pass, which should be equal tolambda: None?
Unfortunately not:
0$ python3
1Python 3.10.11 (main, Apr 13 2023, 01:52:20) [Clang 13.0.0 ] on openbsd7
2Type "help", "copyright", "credits" or "license" for more information.
3>>> x = lambda: pass
4 File "<stdin>", line 1
5 x = lambda: pass
6 ^^^^
7SyntaxError: 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.
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?
79+ pass
80
81- if used_chattr:
82- subprocess.check_call(['chattr', '-i', filename])
83+ if undo_immutable:
84+ self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
nit: I think it’s fine to always show this line, so one can quickly see if the actual test is executed at all:
0 self.log.info("Attempt to restart and reindex the node with the unwritable block file")
I’m running OpenBSD 7.4 with the package e2fsprogs installed, which has a binary
/usr/local/bin/chattrinstalled, 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?
62+ # macOS, and *BSD
63+ try:
64+ subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
65+ try:
66+ subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
67+ undo_immutable = subprocess.check_call(['chflags', 'nouchg', filename])
0 undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename])
(currently, the test is not executed on OpenBSD, as undo_immutable is assigned an integer of value zero, if the chflags call succeeds)
@theStack the issue was hard-coding all the OSs. Every flavor of *BSD plus macOS has
chattr, Linux haschflagsand Windows, well…chmodseems 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 IMHONot 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).
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:
0diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
1index b276be9c03..9f3b642338 100755
2--- a/test/functional/feature_reindex_readonly.py
3+++ b/test/functional/feature_reindex_readonly.py
4@@ -36,7 +36,7 @@ class BlockstoreReindexTest(BitcoinTestFramework):
5 undo_immutable = lambda: None
6 # Linux
7 try:
8- subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
9+ subprocess.run_thismethoddoesnotevenexist(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
10 try:
11 subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
12 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 :)
ACK 5a0688a20d88a9641c02436abbd7b49e227f1a37
Tested in OpenBSD 7.4, once with and once without the e2fsprogs package installed, to trigger both code paths:
0$ which chattr && which chflags
1/usr/local/bin/chattr
2/usr/bin/chflags
3$ ./test/functional/feature_reindex_readonly.py
42023-10-23T15:41:09.438000Z TestFramework (INFO): PRNG seed is: 435157676393106655
52023-10-23T15:41:09.493000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_2uz_nii5
62023-10-23T15:41:10.354000Z TestFramework (INFO): Made file immutable with chattr
72023-10-23T15:41:10.363000Z TestFramework (INFO): Attempt to restart and reindex the node with the unwritable block file
82023-10-23T15:41:10.966000Z TestFramework (INFO): Stopping nodes
92023-10-23T15:41:10.970000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_2uz_nii5 on exit
102023-10-23T15:41:10.971000Z TestFramework (INFO): Tests successful
0$ doas pkg_delete e2fsprogs
1...
2$ which chattr && which chflags
3which: chattr: Command not found.
4$ ./test/functional/feature_reindex_readonly.py
52023-10-23T15:43:04.599000Z TestFramework (INFO): PRNG seed is: 8556685811226418814
62023-10-23T15:43:04.653000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_h4lvamww
72023-10-23T15:43:05.565000Z TestFramework (INFO): Made file immutable with chflags
82023-10-23T15:43:05.622000Z TestFramework (INFO): Attempt to restart and reindex the node with the unwritable block file
92023-10-23T15:43:06.231000Z TestFramework (INFO): Stopping nodes
102023-10-23T15:43:06.295000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_h4lvamww on exit
112023-10-23T15:43:06.362000Z TestFramework (INFO): Tests successful
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 5a0688a20d88a9641c02436abbd7b49e227f1a37 tested on macOS only
32@@ -34,11 +33,13 @@ def reindex_readonly(self):
33 filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
34 filename.chmod(stat.S_IREAD)
35
36- used_chattr = False
37- if platform.system() == "Linux":
38+ undo_immutable = lambda: None
39+ # Linux
38+ undo_immutable = lambda: None
39+ # Linux
40+ try:
41+ subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
42 try:
43 subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
On Ubuntu 22.04.3 LTS, this line throws an exception for me
02023-10-25T19:35:49.783000Z TestFramework (DEBUG): Make the first block file read-only
12023-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.
22023-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).
debug or info level?