Skip to content

test: enable reindex readonly test on *BSD #28660

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.

Sign up for GitHub

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

Merged
fanquake merged 1 commit into from
Oct 24, 2023
Merged

test: enable reindex readonly test on *BSD #28660

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions test/functional/feature_reindex_readonly.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"""

import os
import platform
import stat
import subprocess
from test_framework.test_framework import BitcoinTestFramework
Expand All @@ -34,11 +33,13 @@ def reindex_readonly(self):
filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
filename.chmod(stat.S_IREAD)

used_chattr = False
if platform.system() == "Linux":
undo_immutable = lambda: None
# Linux
Copy link
Member

@maflcko maflcko Oct 24, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@jonatack jonatack Oct 25, 2023

Choose a reason for hiding this comment

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

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

try:
subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
try:
subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
Copy link
Contributor

@mzumsande mzumsande Oct 25, 2023

Choose a reason for hiding this comment

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

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).

Copy link
Member

@maflcko maflcko Oct 25, 2023

Choose a reason for hiding this comment

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

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?

used_chattr = True
undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename])
self.log.info("Made file immutable with chattr")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
Expand All @@ -50,15 +51,33 @@ def reindex_readonly(self):
self.log.warning("Return early on Linux under root, because chattr failed.")
self.log.warning("This should only happen due to missing capabilities in a container.")
self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
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=[]):
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
Copy link
Contributor

@theStack theStack Oct 20, 2023

Choose a reason for hiding this comment

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

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)

Copy link
Member

@maflcko maflcko Oct 20, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@theStack theStack Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@maflcko maflcko Oct 20, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@theStack theStack Oct 20, 2023

Choose a reason for hiding this comment

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

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.

except Exception:
# macOS, and *BSD
try:
subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
try:
subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename])
self.log.info("Made file immutable with chflags")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
if e.stdout:
self.log.warning(f"stdout: {e.stdout}")
if e.stderr:
self.log.warning(f"stderr: {e.stderr}")
if os.getuid() == 0:
self.log.warning("Return early on BSD under root, because chflags failed.")
undo_immutable = False
except Exception:
pass

if used_chattr:
subprocess.check_call(['chattr', '-i', filename])
if undo_immutable:
self.log.info("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=[]):
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()

filename.chmod(0o777)

Expand Down