Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

fixing flakiness in voting and forked chain tests #10381

Merged
merged 1 commit into from
May 19, 2021
Merged

Conversation

dimas1185
Copy link
Contributor

Change Description

Fixes:

  1. nodeos_voting_lr_test. It was broken after bugfix in Node.waitForIrreversibleBlock where it was waiting for head instead of lib. After fix this increased execution time and lead to timeout fails.
  2. nodeos_forked_chain_lr_test. It had flakiness due to race condition in head catchup and switching forks that lead to go_away_message being sent with status benign_other due to unknown block requested. Fix adding retry after closing connection.

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

} else {
}
else if ( msg.reason == benign_other ) {
if ( retry ) fc_dlog( logger, "received benign_other reason, retrying to connect");
Copy link
Contributor

Choose a reason for hiding this comment

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

Note retry here only means immediate retry. It would retry either way.

@@ -1221,7 +1221,10 @@ def getNextCleanProductionCycle(self, trans):
# The voted schedule should be promoted now, then need to wait for that to become irreversible
votingTallyWindow=120 #could be up to 120 blocks before the votes were tallied
promotedBlockNum=self.getHeadBlockNum()+votingTallyWindow
self.waitForIrreversibleBlock(promotedBlockNum, timeout=rounds/2)
# There was waitForIrreversibleBlock but due to bug it was waiting for head and not lib.
Copy link
Contributor

Choose a reason for hiding this comment

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

Go ahead and fix waitForIrreversibleBlock which should be as simple as changing the default argument. It is fine to leave this as you have it, but should probably update this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had fixed it, which is why this is taking longer. He could have extended the test, but since it had never failed before, we presumed it was fine to replace this call with what it had been doing before (unintentionally)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'v fine with it using waitForBlock, but we should fix waitForIrreversibleBlock which is still:

    def waitForIrreversibleBlock(self, blockNum, timeout=WaitSpec.default(), blockType=BlockType.head):
        return self.waitForBlock(blockNum, timeout=timeout, blockType=blockType)

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, what I fixed was waitForBlock to actually honor blockType, I missed that waitForIrreversibleBlock looked like this. Which then makes me wonder why Dmytro's change had any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heifner perhaps you are looking at wrong branch.

    def waitForIrreversibleBlock(self, blockNum, timeout=WaitSpec.default()):
        return self.waitForBlock(blockNum, timeout=timeout, blockType=BlockType.lib)

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently so.

@dimas1185 dimas1185 merged commit 207a697 into develop May 19, 2021
@dimas1185 dimas1185 deleted the flaky-tests-fix branch May 19, 2021 17:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants