chore: BlockAsLocalFile Persistence Test Plan [Phase 2] #856
Conversation
ata-nas
added
the
pull request
ata-nas
force-pushed
the
844-local-persistence-test-plan-phase-2
branch
from
53ef22b to
2ee40ca
Compare
block-node/test-specifications/local-persistence/local-persistence-tp.md
Outdated
Show resolved
Hide resolved
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md
Outdated
Show resolved
Hide resolved
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md
Outdated
Show resolved
Hide resolved
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md
Outdated
Show resolved
Hide resolved
ata-nas
force-pushed
the
844-local-persistence-test-plan-phase-2
branch
from
bfb3c5d to
9a24c4e
Compare
Signed-off-by: Atanas Atanasov <[email protected]>
ata-nas
force-pushed
the
844-local-persistence-test-plan-phase-2
branch
from
9a24c4e to
8c3347a
Compare
| > all the digits of a `long` (max. 19 digits). We have a single digit per node | ||
| > or directory up to a max. depth of 18, and then we have the full `BlockNumber` | ||
| > as the file name of any persisted block with all leading zeros for the `live` | ||
| > root and for the `archive` root we have a max. depth of |
There was a problem hiding this comment.
| > root and for the `archive` root we have a max. depth of | |
| > root. For the `archive` root we have a max. depth of |
| readers etc. which when combined, provide a robust and efficient way to store | ||
| and retrieve`Blocks`. This E2E test plan aims to highlight the key scenarios | ||
| that need to be tested for end results in order to ensure the correctness and | ||
| proper working of the `BlockAsLocalFile Persistence`. |
There was a problem hiding this comment.
you can link the design doc here for further clarity
There was a problem hiding this comment.
Good point. That one also needs an update with latest changes (mainly migrating to an async implementation and the fact that we dropped support for block-as-dir). I will make an issue for the update of the design doc to be addressed separately. Also, there is a simple chore to update all the md file internal links as most of them got broken due to module/renaming/hiero migration changes (just FYI).
There was a problem hiding this comment.
We also need to move the docs folder back up to the repository root so documentation is associated with the entire product, not just one module. I tried to keep the links working by moving the documents with the "server" module, but apparently that was not effective.
There was a problem hiding this comment.
@jsync-swirlds @Nana-EC where do you believe it would be best to move the test-specification directory? Under repo root maybe?
There was a problem hiding this comment.
Once this PR merges, these will belong in a reasonable structure under /docs/test-plans (at least that is the way I read that PR).
| | Test Case ID | Test Name | Scenario Description | Requirement | Input | Output | Implemented (Y/N) | | ||
| |----------------:|:-----------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-----------------:| | ||
| | E2ETC_BLFP_0001 | `Verify Location of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the `Block` is indeed persisted in the proper location. For `unverified` root, `Block`s are stored directly under the root, no `trie` structure is utilized. | Unverified `Block`s are persisted as files directly under the `unverified` root, without the use of the `trie` structure. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Regular file: `0000000000000012345.blk` with location: `/unverified/0000000000000012345.blk`. | N | | ||
| | E2ETC_BLFP_0002 | `Verify Content of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the persisted `Block`'s content is indeed what we expect it to be (same as the content received through the `BlockStream` as input). | The persisted `Block`'s content is the same as the content that has been received as input. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Content of persisted `Block` is the same as the one received via input. | N | |
There was a problem hiding this comment.
Q: Is there a consideration for the order of block items when confirming contents? Or does it make sense to check elsewhere?
There was a problem hiding this comment.
The writer is agnostic to any decision making, it is being supplied data. If it gets a block header, as long as it has received a block proof just prior to the header, it will initiate a new writer task and will keep supplying it with items until it sees the next proof, repeating the process afterwards. The persistence is void of any logic to check the contents thereof (while writing), this is why initially we even have the file in unverified root. Once verification passes, then it gets published. So here I would assume that your input for the test would be a legit, ordered stream of items, starting with header/s and ending with proof/s. Depending on the test case, we will want verification to pass or not, as it will be important in order to assert the test correctly. I guess what you are referring to is block content proof, which may be a bit different than what is tried to be asserted here.
There was a problem hiding this comment.
I see.
Not necessarily block contents proof.
More so if the input to the server is a batch of list of block items in order, then it would make sense to confirm that when grouped and written as a file that the order was maintained and the file is correct.
| |----------------:|:-----------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-----------------:| | ||
| | E2ETC_BLFP_0001 | `Verify Location of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the `Block` is indeed persisted in the proper location. For `unverified` root, `Block`s are stored directly under the root, no `trie` structure is utilized. | Unverified `Block`s are persisted as files directly under the `unverified` root, without the use of the `trie` structure. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Regular file: `0000000000000012345.blk` with location: `/unverified/0000000000000012345.blk`. | N | | ||
| | E2ETC_BLFP_0002 | `Verify Content of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the persisted `Block`'s content is indeed what we expect it to be (same as the content received through the `BlockStream` as input). | The persisted `Block`'s content is the same as the content that has been received as input. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Content of persisted `Block` is the same as the one received via input. | N | | ||
| | E2ETC_BLFP_0003 | `Verify Overwritable Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. Unverified `Block`s can be overwritten no matter the cause of the overwrite (a failed persistence, system restart or verification failure could potentially trigger a rewrite). | Given that `Block` with `BlockNumber` 12345L is persisted as a file under the `unverified` root, verify that it can be overwritten, file content must be verified changed after an overwrite. | Ensure that `Block` with `BlockNumber`=12345L is persisted as file at `/unverified/0000000000000012345.blk` and is not published to `live` nor archived in `archive`, then send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | `Block` persisted as file at `/unverified/0000000000000012345.blk` is overwritten with contents of the newly sent `Block` to the persistence service. | N | |
There was a problem hiding this comment.
file content must be verified changed after an overwrite
To confirm the test will check that the override contents are different than before?
If they are the same is an error expected or is the file not overridden?
There was a problem hiding this comment.
Even if contents are the same, the file will still be truncated and overwritten. Always. As it is not possible to assert the state of the currently existing file (the one that will be now overwritten)
There was a problem hiding this comment.
It might be possible to calculate a file hash before the overwrite, then again after and assert that the two are not the same... Any MessageDigest algorithm would be sufficient (perhaps MD5, to minimize testing load?)
| | E2ETC_BLFP_0001 | `Verify Location of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the `Block` is indeed persisted in the proper location. For `unverified` root, `Block`s are stored directly under the root, no `trie` structure is utilized. | Unverified `Block`s are persisted as files directly under the `unverified` root, without the use of the `trie` structure. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Regular file: `0000000000000012345.blk` with location: `/unverified/0000000000000012345.blk`. | N | | ||
| | E2ETC_BLFP_0002 | `Verify Content of Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. We must make sure that the persisted `Block`'s content is indeed what we expect it to be (same as the content received through the `BlockStream` as input). | The persisted `Block`'s content is the same as the content that has been received as input. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | Content of persisted `Block` is the same as the one received via input. | N | | ||
| | E2ETC_BLFP_0003 | `Verify Overwritable Block Persisted Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. Unverified `Block`s can be overwritten no matter the cause of the overwrite (a failed persistence, system restart or verification failure could potentially trigger a rewrite). | Given that `Block` with `BlockNumber` 12345L is persisted as a file under the `unverified` root, verify that it can be overwritten, file content must be verified changed after an overwrite. | Ensure that `Block` with `BlockNumber`=12345L is persisted as file at `/unverified/0000000000000012345.blk` and is not published to `live` nor archived in `archive`, then send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | `Block` persisted as file at `/unverified/0000000000000012345.blk` is overwritten with contents of the newly sent `Block` to the persistence service. | N | | ||
| | E2ETC_BLFP_0004 | `Verify Cleanup for Failed Block Persistence Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. While the `Block` is still under the `unverified` root, if persistence or verification fails, a cleanup of the written file is expected. | `Block` file and any data that may have resulted during a failed writing task must be cleaned up if persistence or verification fails. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | No file/data present under the `unverified` root that are result of a failed writing task. | N | |
There was a problem hiding this comment.
Do we log in such a case?
Should log checks be part of the coverage here?
There was a problem hiding this comment.
Which test case are you referring to? If you mean E2ETC_BLFP_0004, there are debug logs on all persistence results, but doing assertions based on logs is not a good idea IMO.
There was a problem hiding this comment.
Consensus has some pretty big headaches because certain logs are, effectively, part of the operational API and any change to those log entries breaks node management and performance testing tooling.
We should learn from that and make sure that logging is purely informational.
If something is truly important and must be reported, the "container-native" recommendation for doing that would be an /statusz endpoint managed by the Server Health facility (along with /healthz and /readyz) which provides a well-defined JSON response with detailed system status and point-in-time metrics. We could also offer an /alertz endpoint which publishes anything that should be alertable (ideally with much more detail than a single-line log string can offer).
That would be a completely separate and not-inconsiderable effort to implement, however. It would probably make operations staff (possibly including ourselves) very grateful, though.
There was a problem hiding this comment.
Yeah in hindsight I'm not a fan of logs in API either.
It's manageable when every case has a test case to capture behaviour but it quickly falls apart.
We are indeed missing the API to expose some details so maybe the full form of this and a few other tests should include an API call
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md
Show resolved
Hide resolved
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md
Show resolved
Hide resolved
| | E2ETC_BLFP_0004 | `Verify Cleanup for Failed Block Persistence Under Unverified Root` | `BlockAsLocalFile Persistence` will initially persist a `Block` under `unverified` root. While the `Block` is still under the `unverified` root, if persistence or verification fails, a cleanup of the written file is expected. | `Block` file and any data that may have resulted during a failed writing task must be cleaned up if persistence or verification fails. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service. | No file/data present under the `unverified` root that are result of a failed writing task. | N | | ||
| | E2ETC_BLFP_0005 | `Verify Discoverability of Block Persisted Under Unverified Root` | A `Block` that is persisted under the `unverified` root must not be discoverable (i.e. readable) as if it were available as data while the `Block` is still under the `unverified` root (essentially not yet published as available). | Any `Block` that is persisted under the `unverified` root must not be discoverable (essentially visible as available) while it still resides under the `unverified` root. | Ensure that `Block` with `BlockNumber`=12345L is persisted as file at `/unverified/0000000000000012345.blk` and is not published to `live` nor archived in `archive`, then send `long` blockNumber=12345L as parameter to the block reader. | Block reader should return a not found status for the queried `Block`. | N | | ||
| | E2ETC_BLFP_0006 | `Verify Location of Block Persisted Under Unverified Root After Move to Live Root` | `BlockAsLocalFile Persistence` will move a `Block` from `unverified` to `live` root after verification passes. Given that we have a persisted `Block` as a file under the `unverified` root, after verification for the said `Block` passes, the `BlockAsLocalFile Persistence` must move the persisted `Block` file from `unverified` root to the `live` root, utilizing the `trie` structure of the `live` root, essentially publishing the `Block` and making it available. | Any `Block` that is persisted under the `unverified` root must be moved to `live` once verification passes (essentially publishing it as available). | Ensure that `Block` with `BlockNumber`=12345L is persisted as file at `/unverified/0000000000000012345.blk` and is not published to `live` nor archived in `archive`, then verification must publish a successful verification result for `Block` with `BlockNumber`=12345L. | Regular file: `0000000000000012345.blk` with location: `/unverified/0000000000000012345.blk` is no longer present, but is now moved as a regular file at `/live/0/0/0/0/0/0/0/0/0/0/0/0/0/0/1/2/3/4/0000000000000012345.blk`. | N | | ||
| | E2ETC_BLFP_0007 | `Verify Contnet of Block Persisted Under Unverified Root After Move to Live Root` | `BlockAsLocalFile Persistence` will move a `Block` from `unverified` to `live` root after verification passes. After the move happens, the content of the now live file must be the same compared to pre-move. | Any `Block` that is persisted under the `unverified` root must be moved to `live` once verification passes (essentially publishing it as available) and the content of the moved `Block` must be the same as pre-move. | Ensure that `Block` with `BlockNumber`=12345L is persisted as file at `/unverified/0000000000000012345.blk` and is not published to `live` nor archived in `archive`, then verification must publish a successful verification result for `Block` with `BlockNumber`=12345L. | Regular file: `0000000000000012345.blk` with location: `/unverified/0000000000000012345.blk` is no longer present, but is now moved as a regular file at `/live/0/0/0/0/0/0/0/0/0/0/0/0/0/0/1/2/3/4/0000000000000012345.blk` and content of the moved file are the same as pre-move. | N | |
There was a problem hiding this comment.
Should we add a test to confirm that 10 files written in succession are all available under the /live root dir e.g. 12340 to 12349
There was a problem hiding this comment.
Which test case are you referring to here?
There was a problem hiding this comment.
I'm suggesting a new test case
There was a problem hiding this comment.
✅ Done, test case added.
| | E2ETC_BLFP_0013 | `Verify Persistence Failure in Case of an IO Issue` | A publisher of the `BlockStream` must expect a `PERSISTENCE_FAILURE` response if an IO issue occurs while the actual persistence of the current `Block` is happening. | A publisher of the `BlockStream` will receive a `PERSISTENCE_FAILURE` response if there is an IO issue during the actual persistence of the current `Block`. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service, then an IO issue occurs during writing of the file. | Publisher must expect a `PERSISTENCE_FAILURE` response. | N | | ||
| | E2ETC_BLFP_0014 | `Verify Successful Block Persistence` | A publisher of the `BlockStream` must expect a successful acknowledgement if the persistence and verification of the current streamed `Block` succeed. | A publisher of the `BlockStream` will receive a successful acknowledgement if the persistence and verification of the current streamed `Block` succeed. | Send `BlockItem`s for `Block` with `BlockNumber`=12345L to the persistence service, then persist in `unverified`, then verification publishes a successful verification result for the input `Block`, then persistence moves/publishes the `Block` in `live`. | Publisher must expect a successful acknowledgement. | N | | ||
| | E2ETC_BLFP_0015 | `Verify Successful Archival of Blocks` | `BlockAsLocalFile Persistence` will periodically (based on passed persistence threshold) archive `Block`s in groups of configurable size. Once the archive threshold passes, the `Block Node` has to archive the next group in line. | Given the `archiveGroupSize` is set to 10 and that we have all `Block`s persisted as files and published under the `live` root for `Block`s with `BlockNumber`s between 0L and 19L both included, when we now receive as input the `BlockItems` for `Block` with `BlockNumber` 20L, then persist, verify and publish it to `live`, we expect that all the `Block`s with `BlockNumber`s between 0L and 10L will be zipped to `/archive/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0.zip` and a link `/live/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0.zip` will be available. | Ensure that `archiveGroupSize` is 10, `Block`s with `BlockNumber`s between 0L and 19L both included are persisted, verified and published in `live`, then send `BlockItem`s for `Block` with `BlockNumber` = 20L and persist, verify and publish it to `live`. | Link: `/live/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0.zip` exists in the place of where the files for `Block`s with `BlockNumber`s between 0L and 9L, both included, must be residing, regular file: `/archive/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0.zip` exists that contains all the files for `Block`s with `BlockNumber`s between 0L and 9L, both included. | N | | ||
| | E2ETC_BLFP_0016 | `Verify Discoverability of Block Persisted Under Archive Root` | A `Block` file that has been archived must be readable by using the system's APIs. Archived `Block`s are available, final and discoverable. | Any `Block` that is archived is discoverable in the sense that it is deemed available and can be read/queried via the `Block Node`'s APIs. | Ensure that `Block` with `BlockNumber`=12345L is archived and is not available in `live` nor `unverified`, then send `long` blockNumber=12345L as parameter to the block reader. | Block reader should return the queried `Block`. | N | |
There was a problem hiding this comment.
Add a test case to confirm the archived zip files contents for each block matches what's it was in unverified and then /live
There was a problem hiding this comment.
Good point, will do. We already have one that asserts unverified to live content, now I believe we only need live to archive, as it is the other edge case. It is better to test separately each aspect of a feature on it's own as it is far easier to assert just one thing that is the aim of a test and also to discern what is wrong, having 5 tests with a simple assert to test different aspects of a feature is far more granular than having 1 or two that assert multiple things in chain.
| > must be found for the zip. `archiveGroupSize` is the configurable amount of | ||
| > files that will be archived as a group (must be a pow of 10)._ | ||
|
|
||
| | Test Case ID | Test Name | Scenario Description | Requirement | Input | Output | Implemented (Y/N) | |
There was a problem hiding this comment.
I would suggest a column for noting the grouping factor of functionality. Example I see ingestion, writing/publication, reading, archiving as potential values, there's probably better names
Let's think and agree on one before adding it. Doesn't have to be in this PR if we're unsure but under the umbrella of persistence there are a few features that it would be nice to know which combination of features each row is tackling
There was a problem hiding this comment.
That is a good point. @georgi-l95 @AlfredoG87 @jsync-swirlds opinions?
There was a problem hiding this comment.
I agree in concept, but I also think it's best done separately from this PR.
This affects all of these test plans, and probably should be reflected in a template document which incorporates all of the things brought up for this document.
There was a problem hiding this comment.
@jsync-swirlds @Nana-EC I have created #889 where we will create a template for the test cases and we can address all these additions.
There was a problem hiding this comment.
I like the suggestion 👍
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an end-to-end test plan for BlockAsLocalFile Persistence, outlining various test scenarios to validate block storage, movement, archival, and error handling.
- Added a comprehensive list of test cases covering persistence paths, content verification, cleanup behavior, and archival processes.
- Specifies expected inputs and outputs for each test scenario to ensure system correctness.
Comments suppressed due to low confidence (1)
block-node/test-specifications/local-persistence/block-as-local-file-persistence-tp.md:9
- Missing space between 'retrieve' and
Blocks. Consider updating to 'retrieveBlocks' for improved readability.
and retrieve`Blocks` from a local storage. It has several moving parts, like path resolution logic, writer tasks,
| - **Trie Data Structure for Path Resolution**: `BlockAsLocalFile Persistence` | ||
| utilizes a trie data structure to resolve paths for `Block`s. This is done to | ||
| ensure that the path resolution is efficient and fast. | ||
| - **`unverfied` root path**: Initially all blocks are stored under the |
There was a problem hiding this comment.
Typo detected: 'unverfied' should be 'unverified'.
| - **`unverfied` root path**: Initially all blocks are stored under the | |
| - **`unverified` root path**: Initially all blocks are stored under the |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #856 +/- ##
============================================
- Coverage 89.20% 88.70% -0.50%
+ Complexity 713 710 -3
============================================
Files 131 131
Lines 3047 3046 -1
Branches 219 219
============================================
- Hits 2718 2702 -16
- Misses 264 279 +15
Partials 65 65 see 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Reviewer Notes
BlockAsLocalFile Persistence (BLFP)Related Issue(s)
Closes #844