feat(validator): elections powered by etcd leases #31

petermetz · 2019-11-07T05:45:40Z

Fixes #19

Could only run the unit tests, not a proper end to end test on account of the resource leakage issue.
Once the CI environment is up and running the change can be properly verified.

denis-yu-glotov · 2019-11-07T14:48:06Z

How does architecture change here? I see you add 3 raft nodes to 4 quorum-validator nodes. How does the resulting network work in brief? How does validator nodes map to raft nodes?

petermetz · 2019-11-14T22:59:02Z

How does architecture change here? I see you add 3 raft nodes to 4 quorum-validator nodes. How does the resulting network work in brief? How does validator nodes map to raft nodes?

@denis-yu-glotov
Added some documentation that I'm hoping will answer all those questions. If anything is still spotty please let me know, I want to make sure the documentation explains everything properly!

Here's the link to the docs: https://github.com/petermetz/blockchain-integration-framework/blob/feat/etcd-leader-election/docs/architecture/leader-election/etcd-leases.md

denis-yu-glotov

huh, looks my comments were not published, sorry

src/Validator.js

jonathan-m-hamilton · 2019-11-19T16:07:43Z

@hugo-borne to run regression tests and let @petermetz know if any issues to address before merge

hugo-borne

I've got an error running fabric federation

To reproduce:

npm run fabric
npm run fed:fabric
npm run fed:fabric:log

Error (see attached screenshot):

fabric_validator4_1  | [2019-11-19T17:59:51.883] [DEBUG] Validator - Starting as Follower... 
fabric_validator4_1  | [2019-11-19T17:59:51.900] [DEBUG] Validator - My Leader is : 
fabric_validator4_1  |       (undefined,
fabric_validator4_1  |       undefined,
fabric_validator4_1  |       undefined)
fabric_validator4_1  | (node:7) UnhandledPromiseRejectionWarning: TypeError: Address must be a string!
fabric_validator4_1  |     at exports.Socket.Socket.connect (/federation/node_modules/zeromq/lib/index.js:519:13)
fabric_validator4_1  |     at Validator.startAsFollower (/federation/node_modules/@hyperledger/blockchain-integration-framework/src/Validator.js:143:24)
fabric_validator4_1  |     at Validator.start (/federation/node_modules/@hyperledger/blockchain-integration-framework/src/Validator.js:87:12)
fabric_validator4_1  |     at /federation/app.js:46:13
fabric_validator4_1  |     at <anonymous>
fabric_validator4_1  | (node:7) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
fabric_validator4_1  | (node:7) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Warning: the package.json in your fork is pointing to another branch, which means you may be testing your code against another version than the one in feat/etcd-leader-election

petermetz · 2019-11-19T18:11:20Z

Ah, must be one of the stylistic changes I did for review. Will check, thank you!

hugo-borne

Previous error persists on my side:

Non-leader Validators are not able to parse the leader's address
TypeError: Address must be a string!

petermetz · 2019-11-22T17:58:16Z

Previous error persists on my side:

Non-leader Validators are not able to parse the leader's address
TypeError: Address must be a string!

Hmm, is it possible that you have the docker images, node_modules folders hanging around from the previous test?
Just asking because I had the same issue while testing the fix and only after cleaning up my environment was I able to see that the bug was fixed.
The ./tools/ci.sh script is an easy to way to run all the cleanups necessary (if you already used that and the result is the same then something is definitely wrong though).

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

It was copy-pasted from the fabric docker-compose and so the network names were incorrectly referencing the fabric network, not the quorum. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

The getter that was changed: isCurrentNodeLeader Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

TODO(peter.somogyvari): Once leader election has enough test coverage, get rid of these property assignments and just use the this.leaderNodeInfo.networkInfo object directly everywhere. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

The examples were hardcoding GitHub branch references which is a moving target and makes it error prone in certain situations to have the intended code running for local tests/CI. Solved it with a pre-install npm script that runs npm pack in the root directory of the project creating an installable tarball of the latest source code that's checked out at the current moment. This strongly ties the examples of any given revision to the exact source code of BIF from that revision as well, guaranteeing that whoever/wherever does an npm install inside the examples will get the current source code of that revision packaged up as an npm dependency. that they are committed with Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Reverting an attempted fix that was breaking the CI. The attempted fix was to introduce resource constraints on the quorum containers (CPU, RAM usage). This worked in the sense that on a smaller dev laptop launching the quorum network didn't cause OOM, but on the other hand containers got stuck in 'unhealthy' status on CI so overall the fix didn't pan out. Also: Performance optimization is added by reducing logging verbosity within the quorum containers and the geth syncmode is being changed from 'full' to 'fast' (the default) which does not verify each block one by one upon initialization. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

This was forgotten from the original commit which only updated fabrid and quorum Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

denis-yu-glotov

I tested on my VM by running tools/ci.sh and it went fine. Is this enough or you have some other manual tests to try?

src/Validator.js

examples/simple-asset-transfer/package.json

tools/create-local-npm-package.js

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

petermetz · 2019-12-02T19:59:32Z

I tested on my VM by running tools/ci.sh and it went fine. Is this enough or you have some other manual tests to try?

I think we are good, trying to get this in as soon as possible now, been open too long and the follow-up PR fixing test:bc is already in the queue as well.

denis-yu-glotov

LGTM, provided that the validator code is work in progress: having separate etcd3 nodes is very questionable to me.

jonathan-m-hamilton

LGTM!

* feat(validator): elections powered by etcd leases Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * docs: etcd leases, state and deployment diagram Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * fix(quorum): docker-compose network name typo It was copy-pasted from the fabric docker-compose and so the network names were incorrectly referencing the fabric network, not the quorum. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * refactor(validator): change getter to method The getter that was changed: isCurrentNodeLeader Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * refactor(validator): Object.assign -> line by line TODO(peter.somogyvari): Once leader election has enough test coverage, get rid of these property assignments and just use the this.leaderNodeInfo.networkInfo object directly everywhere. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * fix(test/validator): isCurrentNodeLeader syntax Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * fix(examples,tools): use local npm package The examples were hardcoding GitHub branch references which is a moving target and makes it error prone in certain situations to have the intended code running for local tests/CI. Solved it with a pre-install npm script that runs npm pack in the root directory of the project creating an installable tarball of the latest source code that's checked out at the current moment. This strongly ties the examples of any given revision to the exact source code of BIF from that revision as well, guaranteeing that whoever/wherever does an npm install inside the examples will get the current source code of that revision packaged up as an npm dependency. that they are committed with Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * fix(example/quorum): container resource usage Reverting an attempted fix that was breaking the CI. The attempted fix was to introduce resource constraints on the quorum containers (CPU, RAM usage). This worked in the sense that on a smaller dev laptop launching the quorum network didn't cause OOM, but on the other hand containers got stuck in 'unhealthy' status on CI so overall the fix didn't pan out. Also: Performance optimization is added by reducing logging verbosity within the quorum containers and the geth syncmode is being changed from 'full' to 'fast' (the default) which does not verify each block one by one upon initialization. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * fix(examples/corda): adds etcd to docker-compose This was forgotten from the original commit which only updated fabrid and quorum Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> * docs(tools): add docs for create local npm packge Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Driver Update

petermetz added the enhancement New feature or request label Nov 7, 2019

petermetz requested review from hugo-borne, jonathan-m-hamilton and denis-yu-glotov November 7, 2019 05:45

petermetz requested a review from tkuhrt as a code owner November 7, 2019 05:45

petermetz force-pushed the feat/etcd-leader-election branch 6 times, most recently from fc52e6b to c99da7d Compare November 14, 2019 18:46

petermetz force-pushed the feat/etcd-leader-election branch from 02f6344 to 00ee082 Compare November 14, 2019 22:59

jonathan-m-hamilton added this to the v0.2.0 milestone Nov 15, 2019

petermetz force-pushed the feat/etcd-leader-election branch 3 times, most recently from 64b1fa4 to 76a97af Compare November 19, 2019 02:31

denis-yu-glotov reviewed Nov 19, 2019

View reviewed changes

src/Validator.js Show resolved Hide resolved

src/Validator.js Outdated Show resolved Hide resolved

src/Validator.js Outdated Show resolved Hide resolved

petermetz force-pushed the feat/etcd-leader-election branch 2 times, most recently from 4bda8ca to b7bcb1f Compare November 19, 2019 17:24

hugo-borne reviewed Nov 19, 2019

View reviewed changes

petermetz force-pushed the feat/etcd-leader-election branch from 8857fa0 to da42d1c Compare November 20, 2019 05:51

petermetz requested review from hugo-borne and denis-yu-glotov November 20, 2019 05:52

hugo-borne reviewed Nov 21, 2019

View reviewed changes

petermetz mentioned this pull request Nov 25, 2019

Fix/peter.somogyvari/ci e2e tests #62

Closed

petermetz force-pushed the feat/etcd-leader-election branch from d3da0ab to a325049 Compare November 26, 2019 20:52

hugo-borne self-assigned this Nov 28, 2019

petermetz added 9 commits November 28, 2019 11:32

feat(validator): elections powered by etcd leases

c2b4012

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

docs: etcd leases, state and deployment diagram

6b386c3

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

fix(quorum): docker-compose network name typo

1af3236

It was copy-pasted from the fabric docker-compose and so the network names were incorrectly referencing the fabric network, not the quorum. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

refactor(validator): change getter to method

7454ed0

The getter that was changed: isCurrentNodeLeader Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

fix(test/validator): isCurrentNodeLeader syntax

f595881

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

fix(examples/corda): adds etcd to docker-compose

0f9b886

This was forgotten from the original commit which only updated fabrid and quorum Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

petermetz force-pushed the feat/etcd-leader-election branch from a325049 to 0f9b886 Compare November 28, 2019 19:33

denis-yu-glotov reviewed Nov 29, 2019

View reviewed changes

docs(tools): add docs for create local npm packge

390018e

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>

petermetz requested a review from denis-yu-glotov December 2, 2019 20:01

petermetz mentioned this pull request Dec 2, 2019

Feat/peter.somogyvari/ci e2e tests #70

Merged

denis-yu-glotov approved these changes Dec 2, 2019

View reviewed changes

jonathan-m-hamilton approved these changes Dec 2, 2019

View reviewed changes

petermetz merged commit 61aab4a into hyperledger:master Dec 2, 2019

petermetz deleted the feat/etcd-leader-election branch December 2, 2019 23:20

ryjones unassigned hugo-borne May 7, 2020

ryjones pushed a commit that referenced this pull request Feb 1, 2023

Merge pull request #31 from sanvenDev/driver-fixes

3006865

Driver Update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(validator): elections powered by etcd leases #31

feat(validator): elections powered by etcd leases #31

petermetz commented Nov 7, 2019

denis-yu-glotov commented Nov 7, 2019

petermetz commented Nov 14, 2019

denis-yu-glotov left a comment

jonathan-m-hamilton commented Nov 19, 2019

hugo-borne left a comment

petermetz commented Nov 19, 2019

hugo-borne left a comment

petermetz commented Nov 22, 2019

denis-yu-glotov left a comment

petermetz commented Dec 2, 2019

denis-yu-glotov left a comment

jonathan-m-hamilton left a comment

feat(validator): elections powered by etcd leases #31

feat(validator): elections powered by etcd leases #31

Conversation

petermetz commented Nov 7, 2019

denis-yu-glotov commented Nov 7, 2019

petermetz commented Nov 14, 2019

denis-yu-glotov left a comment

Choose a reason for hiding this comment

jonathan-m-hamilton commented Nov 19, 2019

hugo-borne left a comment

Choose a reason for hiding this comment

petermetz commented Nov 19, 2019

hugo-borne left a comment

Choose a reason for hiding this comment

petermetz commented Nov 22, 2019

denis-yu-glotov left a comment

Choose a reason for hiding this comment

petermetz commented Dec 2, 2019

denis-yu-glotov left a comment

Choose a reason for hiding this comment

jonathan-m-hamilton left a comment

Choose a reason for hiding this comment