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

[prototype] automatic DB migation on nodeos upgrades #394

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spoonincode
Copy link
Member

Background

This is a prototype for automatic DB migration when the DB is not compatible with the current version of nodeos (such as when chainbase's environment check fails, or chainbase's internal structures have changed). Currently under such conditions users are required to go through a manual upgrade process that includes manually removing files from nodeos' data directories and loading a snapshot (which some users may prefer to only source themselves for security reasons).

The basic premise of this approach is that when a DB is created or loaded successfully (in other words, at the end of nodeos startup), a shared library is dropped alongside the DB1. This shared library is built with the same compiler and chainbase library as the currently running nodeos. The shared library exposes one public function: create a portable snapshot. It's nice to lean on using portable snapshots to upgrade since that's already a very well defined structure where new nodeos remains backward compatible with even OG snapshots.

When nodeos is started and it finds it needs to migrate the DB (because it's in an older format, or the environment check has failed), it loads the shared library alongside the existing DB and executes the function to create a snapshot. This function is quite simple (see new libsnapshotter), but an important aspect is that before creating the snapshot it undoes to LIB state. Rolling back to LIB is required because portable snapshots don't convey any undo state; portable snapshots are generally expected to represent an irreversible block -- something like the create_snapshot RPC endpoint will wait for it to become irreversible before making it available for use.

Once the snapshot has been created, the library is unloaded and the new nodeos' controller is started as-if it was commanded to start from a snapshot. That includes automatic removal of the previous state files (similar to how it would be done manually).

Testing

The easiest way to play with it is to grab a build with gcc and a build with clang (such as the nonpinned build and pinned build) and switch between them. Today that would not work due to the environment check, but here it seamlessly works (with the delay for creating & restoring the snapshot)

Notable Changes

(besides the path changes discussed elsewhere) the other big change stem from me not wanting to use controller. Creating and destroying a controller just has a lot of side effects and potential failures modes I'd rather avoid. So some member functions in controller (and resource_limits_manager & authorization_manager) are refactored out as static. This actually ended up not too bad. There would still need to be some cleanup done to locations, naming, and some public/private/const aspects of course, but the changes don't seem to grossly violate anything.

Problems

Whoops it isn't building in CI due to a dependency failure in ninja; will look in to a good solution.

The current shared library approach has an outstanding problem that nodeos will SEGV at the end of its execution (after main) when performing an upgrade. Something in LLVM is unhappy about either being placed in a shared library or how the library is loaded in a linker namespace. Since libsnapshotter doesn't use controller, ideailly it wouldn't even be linking with LLVM etc. But that's too much refactoring to do with libchain I think. Ultimately, for now, this will probably need to be a changed to be a separate process so that there is no library funny business.

Footnotes

  1. When I first proposed this approach I had in my mind that only the chainbase DB was required to generate a snapshot. Thus, I proposed that the shared library be embedded in to the chainbase DB. But of course, we need a wee bit of forkdb to generate a snapshot. So right now this current implementation elects to just leave the shared library in the state directory.

@spoonincode spoonincode linked an issue Jul 23, 2024 that may be closed by this pull request
using namespace eosio::chain;

//extern "C" void doit(int db_fd, int forkdb_fd) {
extern "C" int makesnap(char* db_path, char* forkdb_path, char* protocol_features_path, char* snapshot_output_path) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Originally instead of paths I wanted to pass in file descriptors because the intent was to sandbox the execution and passing in file descriptors means that the sandbox can disallow opening files. Allowing chainbase & forkdb to accept already open file handles will require some further refactoring of those. But the real problem is the protocol_feature_set which needs to be loaded from a directory and is required for one aspect of snapshot creation. I think it might be worthwhile to reflect on whether what we're doing with these protocol feature files on disk is even needed/appropriate.

(it'd be possible for me to potentially work around it by passing a serialized protocol_feature_set to the function)

Since paths are being passed here, can undo some of the other changes made for chainbase & forkdb which pass a path instead of a directory.

@@ -122,6 +122,7 @@ if(ENABLE_WEXTRA)
set( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wextra" )
endif()

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually not as much of a change as it might look -- while gcc defaults to non-PIC, most (nearly all?) distros tweak their gcc to produce PIC by default (since it's needed for ASLR) including Ubuntu. Our pinned builds are all PIC.

So it most cases this is really a no-op change.

At one point clang didn't have the PIC tweak in Ubuntu but it appears they do it to clang now too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automatic DB migration ("self snapshotting chainbase")
1 participant