INDY-1759: Restoration of lastPrePrepareSeqNo #971

spivachuk · 2018-11-08T20:01:44Z

Implemented restoration of lastPrePrepareSeqNo on a backup primary after restart.
Made some renaming in the code related to view change to make it clearer.
Corrected sdk_send_batches_of_random and sdk_send_batches_of_random_and_check test helper functions.
Wrote tests for lastPrePrepareSeqNo restoration feature.
Corrected existing tests that restarted node instances which had been stopped previously. Now a new node instance is created for restarting a node.
Made advancing last_ordered_3pc on a backup replica up to master's last_ordered_3pc on completion of a regular view change (not propagate primary) conditional: now it is made only if the backup replica is not primary.
Updated tests in test_no_propagate_request_on_different_last_ordered_before_vc module according to the change in the logic of advancing last_ordered_3pc on backup replicas on a regular view change.
Fixed intermittent failures in tests in test_no_propagate_request_on_different_last_ordered_before_vc module.

- Implemented restoration of lastPrePrepareSeqNo on a backup primary after restart. - Made some renaming in the code related to view change to make it clearer. - Corrected sdk_send_batches_of_random and sdk_send_batches_of_random_and_check test helper functions. - Wrote a test for a positive case of lastPrePrepareSeqNo restoration on a backup primary after restart. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

…nto backup-primary-restoration Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com> # Conflicts: # plenum/common/messages/node_messages.py # plenum/server/view_change/view_changer.py

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

…nto indy_1759_test

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

- Corrected tests that restarted node instances which had been stopped previously. Now a new node instance is created for restarting a node. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

- Added more tests for lastPrePrepareSeqNo restoration feature. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

ashcherbakov · 2018-11-13T07:38:12Z

plenum/test/helper.py

@@ -962,14 +962,17 @@ def sdk_send_batches_of_random_and_check(looper, txnPoolNodeSet, sdk_pool, sdk_w
    if num_batches == 1:
        return sdk_send_random_and_check(looper, txnPoolNodeSet, sdk_pool, sdk_wallet, num_reqs, **kwargs)

+    reqs_in_batch = num_reqs // num_batches
+    reqs_in_last_batch = reqs_in_batch + num_reqs % num_batches


This looks incorrect, it should be num_reqs % num_batches?

ashcherbakov · 2018-11-13T07:40:00Z

plenum/server/node.py

@@ -674,6 +683,69 @@ def on_view_change_complete(self):
                replica.clear_requests_and_fix_last_ordered()
        self.monitor.reset()

+    def store_last_sent_pre_prepare_seq_no(self, inst_id, pp_seq_no):


Please move all logic related to last_sent_pre_prepare_seq_no to a separate helper class

Cover it by unit tests

ashcherbakov · 2018-11-13T07:41:45Z

plenum/server/replica.py

@@ -745,6 +745,7 @@ def send3PCBatch(self):
                if ppReq is None:
                    continue
                self.sendPrePrepare(ppReq)
+                self.node.store_last_sent_pre_prepare_seq_no(self.instId, ppReq.ppSeqNo)


Please do not store it for master instance.

ashcherbakov · 2018-11-13T08:03:15Z

plenum/server/node.py

+        logger.info("{} restoring lastPrePrepareSeqNo "
+                    "from stored lastSentPrePrepare value {}"
+                    .format(self, serialized_value))
+        primary_replica.lastPrePrepareSeqNo = pp_seq_no


What about watermarks?

ashcherbakov · 2018-11-13T08:08:32Z

plenum/server/node.py

+            if self.view_changer.previous_view_no == 0:
+                backup_primary_pp_seq_no_restored = \
+                    self._try_restore_last_sent_pre_prepare_seq_no()
+        if not backup_primary_pp_seq_no_restored:


Why do we need this flag? Why can't we always erase it?

ashcherbakov · 2018-11-13T08:13:50Z

plenum/test/restart/test_backup_primary_does_not_restore_pp_seq_no_if_view_is_not_same.py

+        looper, txnPoolNodeSet, sdk_pool_handle, sdk_wallet_client,
+        tconf, tdir, allPluginsPath):
+
+    for _ in range(6):


Why do we need 6 view changes here? This will be very long test.
I think the check for viewNo can be done on unit test level.

ashcherbakov · 2018-11-13T08:31:36Z

plenum/test/restart/test_backup_primary_does_not_restore_pp_seq_no_if_view_is_not_same.py

+backup_inst_id = 1
+
+
+def test_backup_primary_does_not_restore_pp_seq_no_if_view_is_not_same(


The test looks too complex to just check that backup primary does not restore pp_seq_no if viewNo is not the same.
Why do we need to restart all nodes here?

ashcherbakov · 2018-11-13T08:32:42Z

plenum/test/restart/test_backup_primary_restores_pp_seq_if_view_is_same.py

+backup_inst_id = 1
+
+
+def test_backup_primary_restores_pp_seq_no_if_view_is_same(


This can also be done as a unit test.

ashcherbakov · 2018-11-13T08:32:54Z

plenum/test/restart/test_backup_replica_does_not_restore_pp_seq_no_if_not_primary_anymore.py

+nodeCount = 7
+
+
+def test_backup_replica_does_not_restore_pp_seq_no_if_not_primary_anymore(


This can be done as a unit test.

ashcherbakov · 2018-11-13T08:45:05Z

I think we need to have the following tests:

Unit tests for LastPrePrepareSeqNoRestoreHelper:
-- cover can_restore method:
--- check that can not restore if not primary
--- check that can not retore if viewNo is not equal
--- other checks (cover every if-else with a test)
-- cover restore method:
--- check that replica and node values are restored properly
Integration tests:

Restart Backup Primary [parametrize for viewNo=0 and 1]:

send reqs
restart backup primary
make sure that it restored prepare_seq_no, ordered_seq_no and watermarks
make sure backup can continue ordering

Restart all nodes [parametrize for viewNo=0 and 1]:

send reqs
restart all nodes
make sure that nothing is restored
make sure backup can continue ordering

Restart backup primary with high watermark [parametrize for viewNo=0 and 1]:

send a lot of reqs (more than stable checkpoint, so that watermarks are changed)
restart backup primary
make sure that it restored prepare_seq_no, ordered_seq_no and watermarks
make sure backup can continue ordering

Negative case [parametrize for viewNo=0 and 1]:

send a lot of reqs (more than stable checkpoint, so that watermarks are changed)
stop backup primary on Inst2
do view change
start the node
make sure that nothing is restored
make sure backup can continue ordering

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

Indy 1759 test

- Extracted last_sent_pp_seq_no restoration feature to a separate class. - Added shifting watermarks correspondingly to restored last_sent_pp_seq_no. - Removed storing last_sent_pp_seq_no for master replica. - Added unit tests for last_sent_pp_seq_no restoration class. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

ashcherbakov · 2018-11-15T13:17:07Z

plenum/test/input_validation/node_status_db_validation/test_restore_last_sent_pp_seq_no.py

+def check_last_warning(expected_msg):
+    global warning_msg_count
+    warning_msg_count += 1
+    assert warning_msg_count == len(container)


I would rather prefer error codes (instead of just true/false) instead of analyzing logs

- Added integration tests for last_sent_pp_seq_no restoration feature. - Added a unit test for LastSentPpStoreHelper._restore_last_sent_pp_seq_no method. - Rolled back changes in FakeNode class that had caused regression in existing tests. - Removed OutputWarningHandler test helper class. - Removed tests of last_sent_pp_seq_no restoration feature which had been replaced with new ones. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

- Removed tests of last_sent_pp_seq_no restoration feature which had been replaced with new ones. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

ashcherbakov · 2018-11-19T06:49:45Z

plenum/test/pp_seq_no_restoration/test_node_erases_stored_last_sent_pp_key_on_pool_restart.py

+backup_inst_id = 1
+
+
+def test_node_erases_stored_last_sent_pp_key_on_pool_restart(


This tests stops a tested node, then restarts other nodes in the pool, then start the tested node.
It would be great to write another test, where all nodes (including the tested one) are restarted at the same time

- Added an integration test for erasing last_sent_pp_seq_no in case of simultaneous restart of all the nodes in the pool. - Made advancing last_ordered_3pc on a backup replica up to master's last_ordered_3pc on completion of a regular view change (not propagate primary) conditional: now it is made only if the backup replica is not primary. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

…nto backup-primary-restoration Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com> # Conflicts: # plenum/server/node.py

- Updated tests in test_no_propagate_request_on_different_last_ordered_before_vc module according to the change in the logic of advancing last_ordered_3pc on backup replicas on a regular view change. - Fixed intermittent failures in tests in test_no_propagate_request_on_different_last_ordered_before_vc module. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

ashcherbakov · 2018-11-20T06:36:49Z

(ci) test this please

spivachuk added 2 commits November 8, 2018 20:00

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

9f8290d

…nto backup-primary-restoration Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com> # Conflicts: # plenum/common/messages/node_messages.py # plenum/server/view_change/view_changer.py

spivachuk changed the title ~~INDY-1759: Restoration of lastPrePrepareSeqNo~~ [WiP] INDY-1759: Restoration of lastPrePrepareSeqNo Nov 8, 2018

ArtObr and others added 8 commits November 9, 2018 10:33

INDY-1759: Init tests

20dc2c5

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

INDY-1759: Improve tests

6754fdc

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

Merge remote-tracking branch 'spivachuk/backup-primary-restoration' i…

aaeb517

…nto indy_1759_test

INDY-1759: Test final version

b778130

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

INDY-1759: Corrected existing tests

631ade8

- Corrected tests that restarted node instances which had been stopped previously. Now a new node instance is created for restarting a node. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

INDY-1759: Review fixes

4912504

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

INDY-1759: Tests for restoring lastPrePrepareSeqNo

5d29b3b

- Added more tests for lastPrePrepareSeqNo restoration feature. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

INDY-1759: Add unit tests and review fixes

635c280

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

ashcherbakov reviewed Nov 13, 2018

View reviewed changes

ArtObr and others added 3 commits November 13, 2018 11:55

IDNU-1759 Review fixes

c210839

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>

Merge pull request #119 from ArtObr/indy_1759_test

ed18190

Indy 1759 test

ashcherbakov reviewed Nov 15, 2018

View reviewed changes

spivachuk changed the title ~~[WiP] INDY-1759: Restoration of lastPrePrepareSeqNo~~ INDY-1759: Restoration of lastPrePrepareSeqNo Nov 18, 2018

INDY-1759: Integration tests last_sent_pp_seq_no

f24882b

- Removed tests of last_sent_pp_seq_no restoration feature which had been replaced with new ones. Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com>

ashcherbakov reviewed Nov 19, 2018

View reviewed changes

spivachuk added 3 commits November 19, 2018 13:21

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

d476023

…nto backup-primary-restoration Signed-off-by: Nikita Spivachuk <nikita.spivachuk@dsr-company.com> # Conflicts: # plenum/server/node.py

ashcherbakov approved these changes Nov 20, 2018

View reviewed changes

ashcherbakov merged commit 9dbf18e into hyperledger:master Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-1759: Restoration of lastPrePrepareSeqNo #971

INDY-1759: Restoration of lastPrePrepareSeqNo #971

spivachuk commented Nov 8, 2018 •

edited

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov Nov 13, 2018

ashcherbakov commented Nov 13, 2018 •

edited

ashcherbakov Nov 15, 2018

ashcherbakov Nov 19, 2018

ashcherbakov commented Nov 20, 2018

		backup_inst_id = 1


		def test_backup_primary_does_not_restore_pp_seq_no_if_view_is_not_same(

		backup_inst_id = 1


		def test_backup_primary_restores_pp_seq_no_if_view_is_same(

		nodeCount = 7


		def test_backup_replica_does_not_restore_pp_seq_no_if_not_primary_anymore(

		backup_inst_id = 1


		def test_node_erases_stored_last_sent_pp_key_on_pool_restart(

INDY-1759: Restoration of lastPrePrepareSeqNo #971

INDY-1759: Restoration of lastPrePrepareSeqNo #971

Conversation

spivachuk commented Nov 8, 2018 • edited

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

ashcherbakov commented Nov 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashcherbakov commented Nov 20, 2018

spivachuk commented Nov 8, 2018 •

edited

ashcherbakov commented Nov 13, 2018 •

edited