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

FAB 18365 IT flake evictionsuspector failed to read eviction #2780

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

Param-S
Copy link
Contributor

@Param-S Param-S commented Jul 23, 2021

Description

In failing case, the OSN being removed has committed the config change but failed to receive apply_config msg to halt the chain. This state is defined to be handled as part of the eviction suspicion logic. But it failed when evictionsuspector failed to check from lastconfig block if chain is up-to-date.

Fix: Updated the order of statements which looks at the latest config block and block height validation.

Type of change

  • Bug fix

Signed-off-by: Parameswaran Selvam parselva@in.ibm.com

@Param-S Param-S requested a review from a team as a code owner July 23, 2021 11:02
if lastConfigBlock.Header.Number+1 <= height {
es.logger.Infof("Our height is higher or equal than the height of the orderer we pulled the last block from, aborting.")
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
} else {
es.logger.Infof("Chain has been halted, pulling remaining blocks up to (and including) eviction block.")
}

@@ -139,6 +132,12 @@ func (es *evictionSuspector) confirmSuspicion(cumulativeSuspicion time.Duration)
es.halted = true
es.logger.Infof("Chain has been halted, pulling remaining blocks up to (and including) eviction block.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
es.logger.Infof("Chain has been halted, pulling remaining blocks up to (and including) eviction block.")

@yacovm
Copy link
Contributor

yacovm commented Jul 23, 2021

Please move this:

es.logger.Infof("Chain has been halted, pulling remaining blocks up to (and including) eviction block.")

To an else case of the if clause you moved, otherwise this is confusing - it says it is pulling the remaining blocks and then changes its mind about it later on.

after committing config block in orderer but before applying
it raft.

Signed-off-by: Parameswaran Selvam <parselva@in.ibm.com>
@Param-S
Copy link
Contributor Author

Param-S commented Jul 23, 2021

Yes, I moved it down now.

@Param-S Param-S requested a review from yacovm July 23, 2021 14:13
@yacovm yacovm enabled auto-merge (squash) July 23, 2021 15:39
@@ -194,6 +186,15 @@ func TestEvictionSuspector(t *testing.T) {
height: 9,
halt: t.Fail,
},
{
description: "our height is the highest",
Copy link
Member

Choose a reason for hiding this comment

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

nit - this description probably should be updated to reflect actual semantics

Copy link
Member

Choose a reason for hiding this comment

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

should this be back-ported to 1.4? (which i know is now EOL, but i saw commits are still being added to it...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is important. You can get by without this fix.

@guoger
Copy link
Member

guoger commented Jul 23, 2021

/ci-run

@yacovm yacovm merged commit 463271e into hyperledger:main Jul 23, 2021
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.

3 participants