Add support for block log splitting #9184

huangminghuang · 2020-06-04T21:46:07Z

Change Description

This PR support splitting block logs automatically based on block numbers. It also allows user to specify how many split block log files to retains. Once the limit is reached, the older block files can be move to another directory or deleted.

This PR also support recovering from the head block is not completely written to disk automatically which would eliminate most use cases for using eosio-blocklog to manually trim the end of block log file.

Change Type

Select ONE

Documentation

Stability bug fix

Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

Some new options for chain plugin are added:

blocks-log-stride: split the block log file when the head block number is the multiple of the stride. When the stride is reached, the current blog log and index will be renamed 'blocks-<start num>-<end num>.log/index' and a new current block log and index will be created with the most recent block. All files following this format will be used to construct an extended block log.
max-retained-block-files: the maximum number of blocks files to retain so that the blocks in those files can be queried. When the number is reached, the oldest block file would be move to archive dir or deleted if the archive dir is empty. The retained block log files should not be manipulated by users.
blocks-archive-dir: the location of the blocks archive directory (absolute path or relative to blocks dir). If the value is empty, blocks files beyond the retained limit will be deleted. All files in the archive directory are completely under user's control, i.e. they won't be accessed by nodeos anymore.
fix-irreversible-blocks: When the existing block log is inconsistent with the index, allows fixing the log file automatically based on the index - that is, it will take the highest indexed block if it is valid; otherwise it will repair the block log and reconstruct the index.

b1bart

Please note the additional template specializations for fc::datastream in the PR Description.

If there is documentation on the file format changes introduced by this split please link them as well. If not please talk to @nksanthosh about a task to document the blocks.log format in light of these (and any other changes)

unittests/restart_chain_tests.cpp

plugins/chain_plugin/chain_plugin.cpp

b1bart · 2020-06-08T14:34:21Z

libraries/chain/block_log.cpp

+
+            auto [itr, _]      = collection.emplace(log.first_block_num(),
+                                                    mapped_type{ log.last_block_num(), path_without_extension });
+            this->active_index = collection.index_of(itr);


This is only safe if the files are opened in-order, right? Otherwise the index of this entry may change as new entries are added.

Furthermore, it seems like the ordering from for_each_file_in_dir_matches is based on boost::filesystem::directory_iterator which does not guaranteed an order:

from: https://www.boost.org/doc/libs/1_70_0/libs/filesystem/doc/reference.html#Class-directory_iterator

The order of directory entries obtained by dereferencing successive increments of a directory_iterator is unspecified.

Even if it did so lexicographically, the filenames do not contain leading zeros, so you would expect blocks-101-110.log to come before blocks-21-30.log in the test cases below.

If I'm not off base about this we should probably extend the test cases to cover situations where lexicographically ordered filenames are not properly ordered by block number.

test case modified to cover it

The test cases include the new 3 digit names but, they do not close and re-open the block catalog and therefore wouldn't trigger the behavior I'm concerned with. Can we add a test that writes out the segmented blocks log (2 and 3 digit spans), then closes that controller and re-opens a new tester with the same block log directory and ensure that the expected blocks can be read AND have the proper block number after they are read?

test case added in test_split_log_replay

@b1bart We cannot ensure we test this optimization directly, since we don't know the order the file system will decide to give it to us. The iterator will be invalidated by subsequent file additions if they are earlier, but then active_index will be reset and we will drop the invalid iterator without using it.

1. fix bug for command line options type inconsistency 2. rename block-split-factor to block-log-strides 3. modify test to prove that the lexicological order of block log filenames is irrelevant

brianjohnson5972 · 2020-06-10T21:29:40Z

@huangminghuang I think the documentation should indicate that retained block log files should not be manipulated by the user and that all files in the archive directory are completely in the users control

huangminghuang · 2020-06-11T13:38:56Z

Please note the additional template specializations for fc::datastream in the PR Description.

the fc::datastream was copied from the PR 9104 which has been merge to develop.

If there is documentation on the file format changes introduced by this split please link them as well. If not please talk to @nksanthosh about a task to document the blocks.log format in light of these (and any other changes)

There is no block file format change for the PR. Only the additional options to control how to split the log files.

brianjohnson5972 · 2020-06-11T15:56:10Z

Please note the additional template specializations for fc::datastream in the PR Description.

the fc::datastream was copied from the PR 9104 which has been merge to develop.

If there is documentation on the file format changes introduced by this split please link them as well. If not please talk to @nksanthosh about a task to document the blocks.log format in light of these (and any other changes)

There is no block file format change for the PR. Only the additional options to control how to split the log files.

I would say that this is an extension to the block log format, in that now instead of it just being a single file representing a block log, we have daisy chained files, with an understanding of the ordering based on the file name. This does need to be added to the documentation, which will include things like: if the file name must match the range of blocks in the file, that a more recent range of blocks obfuscates the same blocks from a lower range. These are assumptions about functionality, I have not looked at code yet.

b1bart · 2020-06-15T19:57:15Z

libraries/chain/block_log.cpp

+
+            auto [itr, _]      = collection.emplace(log.first_block_num(),
+                                                    mapped_type{ log.last_block_num(), path_without_extension });
+            this->active_index = collection.index_of(itr);


The test cases include the new 3 digit names but, they do not close and re-open the block catalog and therefore wouldn't trigger the behavior I'm concerned with. Can we add a test that writes out the segmented blocks log (2 and 3 digit spans), then closes that controller and re-opens a new tester with the same block log directory and ensure that the expected blocks can be read AND have the proper block number after they are read?

brianjohnson5972

Will this work with starting with a block log version == 1 (starting at block 1) and then will append to it? I think it will, but in looking through the code I see that we only care about the version if we are calling reset. I am wondering if we need a check for starting at block num 1 if version ==1 and stride is set. Would also be nice to have a test for older version, but obviously we are not setup for doing that.

brianjohnson5972 · 2020-06-15T16:50:48Z

libraries/chain/block_log.cpp

      if (!fc::is_directory(data_dir))
         fc::create_directories(data_dir);
+      else
+         catalog.open(data_dir);


Are we retro-actively adding multi-file support for all post-version 1 block log formats? (haven't thought if that is possible, just know that it definitely couldn't work if format requires first block to be block num 1) Even if we are, wouldn't we need a check here for version > 1, or will that happen later on in the logic?

Possibly unpopular thought: discontinue v1 block log format support in nodeos. Instead provide a utility to convert v1 format to v2 format which can be run manually prior to upgrading to this version of nodeos.

Otherwise need continue to maintain backwards support (forever)?

test case added to split log from v1 to the latest version; i.e. if the system starts with v1 block log, it will continue using the v1 log until it reaches the block number to split. After splitting, the newer block log would use the latest block log version regardless of the original block log version.

libraries/chain/block_log.cpp

# Conflicts: # libraries/chain/block_log.cpp

brianjohnson5972 · 2020-06-16T20:53:24Z

unittests/restart_chain_tests.cpp

+}
+
+BOOST_AUTO_TEST_CASE(test_trim_blocklog_front_v1) {
+   block_log::set_version(1); 


Don't need this anymore.

brianjohnson5972 · 2020-06-16T21:55:59Z

#9184 (comment)
I agree, but since @huangminghuang has done such a phenomenal job refactoring the block_log and adding unit tests, it is much easier to keep the support, than to go through the documentation to remove it. Also, this feature should allow any users maintaining a complete block log to use this feature to not have to keep it as one big file. We also need to know the logic works to be able to add the feature into block log to convert the file. Right now the best it would allow you to do would be to trim block 1, but would not be complete. Hopefully we will also add a feature to eosio-blocklog to split a block log on a given stride as well, so that users can maintain a complete block_log backup, without having to have it all residing on their active server.

brianjohnson5972

I also mentioned 2 tests I would like to add in a text.

brianjohnson5972 · 2020-06-16T22:42:53Z

libraries/chain/block_log.cpp

+            this->active_index = this->active_index == npos ? npos : this->active_index - items_to_erase;
+         }
+         this->collection.emplace(start_block_num, mapped_type{end_block_num, filename_base});
+      }


Documentation indicates that inserting here could invalidate the active_index (unless it is a stable_vector, not sure how we know). Also, do we know that the flat_map storage is fixed size per iterator, since the filename_base is not going to be guaranteed to be the same size? I think the safest thing here is to just set active_index = npos after an add. (another corner case on top of this is that we could have an archived file that has a higher start_block_num than this start_block_num, and if we have not had any call that has changed active_index, it could be pointing at that iterator and then it would be invalid at this point)

brianjohnson5972 · 2020-06-18T16:37:09Z

libraries/chain/block_log.cpp

+            if (!index_matches_data(index_path, log))
+               block_log::construct_index(log_path, index_path);
+
+            auto [itr, _]      = collection.emplace(log.first_block_num(),


Need to check if iterator is not added and if it was not, then need to log a warning and set active_index to npos, since the cached information isn't really in the collection.

brianjohnson5972 · 2020-06-18T16:38:04Z

libraries/chain/block_log.cpp

+
+   block_log::~block_log() {}
+
+   bool detail::block_log_impl::recover_from_incomplete_block_head(block_log_data& log_data, block_log_index& index) {


@b1bart we need to discus this, it is a change to how nodeos functions with an invalid block log.

brianjohnson5972 · 2020-06-19T18:29:59Z

plugins/chain_plugin/chain_plugin.cpp

+         "split the block log file when the head block number is the multiple of the split factor")
+         ("max-retained-block-files", bpo::value<uint16_t>()->default_value(config::default_max_retained_block_files),
+          "the maximum number of blocks files to retain so that the blocks in those files can be queried.\n" 
+          "When the number is reached, the oldest block file would be move to archive dir or deleted if the archive dir is empty." )


Make this match the documentation in the PR.

brianjohnson5972 · 2020-06-19T18:30:23Z

plugins/chain_plugin/chain_plugin.cpp

+          "When the number is reached, the oldest block file would be move to archive dir or deleted if the archive dir is empty." )
+         ("blocks-archive-dir", bpo::value<bfs::path>()->default_value(config::default_blocks_archive_dir_name),
+          "the location of the blocks archive directory (absolute path or relative to blocks dir).\n"
+          "If the value is empty, blocks files beyond the retained limit will be deleted.")


Make this match the documentation in the PR.

brianjohnson5972 · 2020-06-19T19:49:16Z

plugins/chain_plugin/chain_plugin.cpp

@@ -228,6 +227,14 @@ void chain_plugin::set_program_options(options_description& cli, options_descrip
   cfg.add_options()
         ("blocks-dir", bpo::value<bfs::path>()->default_value("blocks"),
          "the location of the blocks directory (absolute path or relative to application data dir)")
+         ("blocks-log-stride", bpo::value<uint32_t>()->default_value(config::default_blocks_log_stride),


I think we should add here (and also in PR API documentation) something like "When the stride is breached, the current blog log and index will be renamed 'blocks--.log/index' and a new current block log and index will be created with the most recent block. All files following this format will be used to construct an extended block log."

1. add allow-block-log-auto-fix option 2. reimplement recover_from_incomplete_block_head() to work with v3 log 3. improve some chain plugin argument descriptions

brianjohnson5972 · 2020-06-21T16:14:29Z

libraries/chain/block_log.cpp

@@ -185,6 +185,12 @@ namespace eosio { namespace chain {

      using log_entry = std::variant<log_entry_v4, signed_block_v0>;

+      const block_header& get_block_header(const log_entry& entry) {
+         return std::visit(overloaded{ [](const signed_block_v0& v) -> const block_header& { return v; },
+                                [](const log_entry_v4& v) -> const block_header& { return v.block; } },


Indentation here is a little wierd.

brianjohnson5972 · 2020-06-21T16:20:51Z

libraries/chain/include/eosio/chain/block_log.hpp

@@ -8,7 +8,7 @@ namespace eosio { namespace chain {
   namespace detail { class block_log_impl; }

   /* The block log is an external append only log of the blocks with a header. Blocks should only
-    * be written to the log after they irreverisble as the log is append only. The log is a doubly
+    * be written to the log after they irreversible as the log is append only. The log is a doubly


"after they are irreversible"

brianjohnson5972 · 2020-06-21T16:22:20Z

plugins/chain_plugin/chain_plugin.cpp

         ("max-retained-block-files", bpo::value<uint16_t>()->default_value(config::default_max_retained_block_files),
          "the maximum number of blocks files to retain so that the blocks in those files can be queried.\n" 
-          "When the number is reached, the oldest block file would be move to archive dir or deleted if the archive dir is empty." )
+          "When the number is reached, the oldest block file would be move to archive dir or deleted if the archive dir is empty.\n"


"would be moved to archive dir"

brianjohnson5972 · 2020-06-21T16:24:40Z

unittests/restart_chain_tests.cpp

@@ -402,6 +404,7 @@ BOOST_FIXTURE_TEST_CASE(restart_from_block_log_with_incomplete_head,restart_from
   logfile.open("ab");
   const char random_data[] = "12345678901231876983271649837";
   logfile.write(random_data, sizeof(random_data));
+   allow_block_log_auto_fix = true;


I'm confused here, why is this being set at the very end of the test?

I would think that we should have a test for all paths: truncate index don't set flag and verify block log and index are at the block that block log pointed to, same setup but with flag and verify block log points to block that index pointed to, set flag and have block index point to invalid position (possibly both to far and wrong place in file) and verify that it then matches the block log's block (and index is fixed).

brianjohnson5972 · 2020-06-21T16:27:14Z

plugins/chain_plugin/chain_plugin.cpp

@@ -228,13 +228,20 @@ void chain_plugin::set_program_options(options_description& cli, options_descrip
         ("blocks-dir", bpo::value<bfs::path>()->default_value("blocks"),
          "the location of the blocks directory (absolute path or relative to application data dir)")
         ("blocks-log-stride", bpo::value<uint32_t>()->default_value(config::default_blocks_log_stride),
-         "split the block log file when the head block number is the multiple of the split factor")
+         "split the block log file when the head block number is the multiple of the split factor\n"
+         "When the stride is reached, the current block log and index will be renamed 'blocks-num_begin-num_end.log/index'\n"


Can we use 'block--.log/index' to make it clearer? (here and in the PR documentation)

sorry, markup removed my text
block-<start num>-<end num>.log/index

brianjohnson5972 · 2020-06-22T14:39:08Z

plugins/chain_plugin/chain_plugin.cpp

+          "If the value is empty, blocks files beyond the retained limit will be deleted.\n"
+          "All files in the archive directory are completely under user's control, i.e. they won't be accessed by nodeos anymore.")
+         ("allow-block-log-auto-fix", bpo::value<bool>()->default_value("false"),
+          "When the existing block log is inconsistent with the index, allows fixing the block log and index files")


I think this should add that it will take the highest indexed block, if it is valid, otherwise it will repair the block log and reconstruct the index.

also, add the same to the PR description.

Add support for block log splitting

e9cfda6

huangminghuang requested review from heifner and brianjohnson5972 June 4, 2020 21:46

b1bart suggested changes Jun 8, 2020

View reviewed changes

huangminghuang added 2 commits June 10, 2020 14:15

Merge branch 'develop' into block-log-split

3ac568f

bug fixes and PR comments

efec3c7

1. fix bug for command line options type inconsistency 2. rename block-split-factor to block-log-strides 3. modify test to prove that the lexicological order of block log filenames is irrelevant

huangminghuang requested a review from b1bart June 10, 2020 21:17

rename blocks-split-factor to blocks-log-stride

34c81d3

allows log file version upgrade after spliting

f9937b6

huangminghuang added 2 commits June 11, 2020 11:19

more stride rename

dce3cc3

add support to recover from incomplete block head automatically

0745026

b1bart suggested changes Jun 15, 2020

View reviewed changes

brianjohnson5972 suggested changes Jun 15, 2020

View reviewed changes

huangminghuang added 3 commits June 15, 2020 16:45

fix bug and modify restart chain test with block num with 3 digits.

dd10241

Merge branch 'develop' into block-log-split

68ec5ae

# Conflicts: # libraries/chain/block_log.cpp

add test to split from v1 log

ebf48b0

brianjohnson5972 approved these changes Jun 16, 2020

View reviewed changes

use RAII to set blocklog version in unittests

00eb6c9

brianjohnson5972 suggested changes Jun 16, 2020

View reviewed changes

huangminghuang added 3 commits June 17, 2020 09:02

remove unneeded code

d15e3b5

add comment to address PR concern

d2f8e5b

added tests for different max_retained_block_files configuration

0ebb3fd

brianjohnson5972 suggested changes Jun 18, 2020

View reviewed changes

huangminghuang added 2 commits June 18, 2020 12:13

handle retained block log with overlapping ranges

5fb2af4

add assertion to protect against bad memory access

b069f54

brianjohnson5972 reviewed Jun 19, 2020

View reviewed changes

Address more PR comments

3063759

1. add allow-block-log-auto-fix option 2. reimplement recover_from_incomplete_block_head() to work with v3 log 3. improve some chain plugin argument descriptions

huangminghuang requested a review from brianjohnson5972 June 19, 2020 20:36

brianjohnson5972 suggested changes Jun 21, 2020

View reviewed changes

huangminghuang added 2 commits June 22, 2020 09:06

extend block-log-auto-fix to recovery from corrupted index file

e2dd458

change command line option description

a674825

kimjh2005 mentioned this pull request Jun 22, 2020

[develop] added eosio-blocklog options to take last good blocks #9206

Closed

7 tasks

brianjohnson5972 suggested changes Jun 22, 2020

View reviewed changes

more command line description change

ee3a733

huangminghuang requested a review from brianjohnson5972 June 22, 2020 22:03

Some more PR comments fix

c158c7b

brianjohnson5972 approved these changes Jun 23, 2020

View reviewed changes

rename allow_block_log_auto_fix to fix_irreversible_blocks

3207d7d

huangminghuang merged commit 2b07ec5 into develop Jun 23, 2020

huangminghuang mentioned this pull request Jun 23, 2020

[docs] Documentation changes for PR 9184 #9238

Open

This was referenced Jun 23, 2020

Split the state history file in chunks #9240

Closed

eosio-blocklog should be able to split up an existing monolithic blocks.log #9243

Open

huangminghuang deleted the block-log-split branch June 25, 2020 21:09

aclark-b1 mentioned this pull request Jan 20, 2021

rotate log file #9920

Closed

cc32d9 mentioned this pull request Apr 13, 2022

backport: logs splitting eosnetworkfoundation/mandel#100

Closed

huangminghuang mentioned this pull request Dec 5, 2022

Add block log partition functionality AntelopeIO/leap#532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for block log splitting #9184

Add support for block log splitting #9184

huangminghuang commented Jun 4, 2020 •

edited

Loading

b1bart left a comment

b1bart Jun 8, 2020

b1bart Jun 8, 2020

huangminghuang Jun 11, 2020

b1bart Jun 15, 2020

huangminghuang Jun 16, 2020

brianjohnson5972 Jun 16, 2020

brianjohnson5972 commented Jun 10, 2020

huangminghuang commented Jun 11, 2020

brianjohnson5972 commented Jun 11, 2020

b1bart Jun 15, 2020

brianjohnson5972 left a comment

brianjohnson5972 Jun 15, 2020

matthewdarwin Jun 16, 2020

huangminghuang Jun 16, 2020

brianjohnson5972 Jun 16, 2020

brianjohnson5972 commented Jun 16, 2020

brianjohnson5972 left a comment

brianjohnson5972 Jun 16, 2020

brianjohnson5972 Jun 18, 2020

brianjohnson5972 Jun 18, 2020

brianjohnson5972 Jun 19, 2020

brianjohnson5972 Jun 19, 2020

brianjohnson5972 Jun 19, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 21, 2020

brianjohnson5972 Jun 22, 2020

brianjohnson5972 Jun 22, 2020

brianjohnson5972 Jun 22, 2020

huangminghuang Jun 22, 2020


		block_log::~block_log() {}

		bool detail::block_log_impl::recover_from_incomplete_block_head(block_log_data& log_data, block_log_index& index) {

Add support for block log splitting #9184

Add support for block log splitting #9184

Conversation

huangminghuang commented Jun 4, 2020 • edited Loading

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

b1bart 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

brianjohnson5972 commented Jun 10, 2020

huangminghuang commented Jun 11, 2020

brianjohnson5972 commented Jun 11, 2020

Choose a reason for hiding this comment

brianjohnson5972 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

brianjohnson5972 commented Jun 16, 2020

brianjohnson5972 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

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

huangminghuang commented Jun 4, 2020 •

edited

Loading