Khalil/5844 access node fallbacks #1395

kc1116 · 2021-09-30T16:38:34Z

This PR removes 2 flags from LN/SN nodes --access-address & --secure-access-node-id . It replaces them with a single flag --access-node-ids because protocol state will be the source of truth for connection info to staked access node APIs.
--access-node-ids is a priority ordered list that will be used to create multiple flow client connections for QCVoter and DKG broker. This allows us to fallback to another access node if a node goes down.

update LN/SN flags
add common util funcs to setup multiple client connection options
update QCVoter and DKG Broker to support multiple contract client instances and fallback to the next instance in use every 2 failed retry attempts
update localnet container configs
update integration tests container configs (from make secure grpc access api default for LN/SN client connections #1323)

- update CLI for LN/SN nodes to remove access-address, and secure-access-node-api flags - add --access-node-ids flag - update unit tests - add fallback logic to retry logic of Broadcast and Vote

- update local net flags for LN/SN nodes - update default access node count to 2 - put flow client config prep in common function

- update integration tests flags and container configs (from #1323)

…opts

…ccess-node-fallbacks

codecov-commenter · 2021-09-30T17:48:17Z

Codecov Report

Merging #1395 (78a0825) into master (9d0c789) will decrease coverage by 0.02%.
The diff coverage is 44.73%.

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
- Coverage   55.41%   55.39%   -0.03%     
==========================================
  Files         512      512              
  Lines       31942    31972      +30     
==========================================
+ Hits        17700    17710      +10     
- Misses      11862    11878      +16     
- Partials     2380     2384       +4

Flag	Coverage Δ
unittests	`55.39% <44.73%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/dkg/controller_factory.go	`0.00% <0.00%> (ø)`
module/dkg/broker.go	`58.55% <46.80%> (-7.31%)`	⬇️
module/epochs/qc_voter.go	`59.67% <52.17%> (+1.34%)`	⬆️
engine/collection/synchronization/engine.go	`62.90% <0.00%> (-1.08%)`	⬇️
admin/command_runner.go	`80.00% <0.00%> (+1.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d0c789...78a0825. Read the comment docs.

jordanschalm · 2021-09-30T17:00:44Z

cmd/collection/main.go

-				return fmt.Errorf("missing required flag --access-address")
+		Module("sdk client connection options", func(builder cmd.NodeBuilder, node *cmd.NodeConfig) error {
+			if len(accessNodeIDS) < common.DefaultAccessNodeIDSMinimum {
+				return fmt.Errorf("invalid flag --access-node-ids atleast %x IDs must be provided", common.DefaultAccessNodeIDSMinimum)


Suggested change

return fmt.Errorf("invalid flag --access-node-ids atleast %x IDs must be provided", common.DefaultAccessNodeIDSMinimum)

return fmt.Errorf("invalid flag --access-node-ids atleast %d IDs must be provided", common.DefaultAccessNodeIDSMinimum)

jordanschalm · 2021-09-30T17:05:46Z

cmd/consensus/main.go

-				return fmt.Errorf("missing required flag --access-address")
+		Module("sdk client connection options", func(builder cmd.NodeBuilder, node *cmd.NodeConfig) error {
+			if len(accessNodeIDS) < common.DefaultAccessNodeIDSMinimum {
+				return fmt.Errorf("invalid flag --access-node-ids atleast %x IDs must be provided", common.DefaultAccessNodeIDSMinimum)


Suggested change

return fmt.Errorf("invalid flag --access-node-ids atleast %x IDs must be provided", common.DefaultAccessNodeIDSMinimum)

return fmt.Errorf("invalid flag --access-node-ids atleast %d IDs must be provided", common.DefaultAccessNodeIDSMinimum)

jordanschalm · 2021-09-30T17:08:52Z

cmd/util/cmd/common/flow_client.go

+	for _, anID := range accessNodeIDS {
+		id, err := flow.HexStringToIdentifier(anID)
+		if err != nil {
+			return nil, fmt.Errorf("could not get flow identifer from secured access node id: %s", id)


Suggested change

return nil, fmt.Errorf("could not get flow identifer from secured access node id: %s", id)

return nil, fmt.Errorf("could not get flow identifer from secured access node id (%s): %w", id, err)

jordanschalm · 2021-09-30T17:09:46Z

cmd/util/cmd/common/flow_client.go

+
+	identities, err := snapshot.Identities(filter.HasNodeID(anIDS...))
+	if err != nil {
+		return nil, fmt.Errorf("failed get identities access node IDs from snapshot: %v", accessNodeIDS)


Suggested change

return nil, fmt.Errorf("failed get identities access node IDs from snapshot: %v", accessNodeIDS)

return nil, fmt.Errorf("failed get identities access node identities (ids=%v) from snapshot: %w", accessNodeIDS, err)

jordanschalm · 2021-09-30T17:10:30Z

cmd/util/cmd/common/flow_client.go

+
+	// make sure we have identities for all the access node IDs provided
+	if len(identities) != len(accessNodeIDS) {
+		return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %s", accessNodeIDS, identities)


Suggested change

return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %s", accessNodeIDS, identities)

return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %v", accessNodeIDS, identities.NodeIDs())

so the expected and actual values are in the same format

jordanschalm · 2021-09-30T17:21:59Z

integration/testnet/network.go

+
+		// string slice argument for LN/SN node --access-node-ids
+		// remove extra comma at the end of string
+		anIDS := accessNodeIDS.String()[:accessNodeIDS.Len()-1]


Can use strings.Join to make this a bit cleaner

jordanschalm · 2021-09-30T17:24:25Z

module/dkg/broker.go

 			if err != nil {
 				b.log.Error().Err(err).Msgf("error broadcasting, retrying (%x)", attempts)
+
+				// retry with next fallback client every 2 attempts


Suggested change

// retry with next fallback client every 2 attempts

// retry with next fallback client after 2 failed attempts

jordanschalm · 2021-09-30T17:44:08Z

module/dkg/broker.go

+		committee:          committee,
+		me:                 me,
+		myIndex:            myIndex,
+		dkgContractClients: dkgContractClients,


Writing down some more detail of my suggestion from our call yesterday for sharing the fallback logic between the QCVoter and DKGBroker and keeping the details of the logic abstracted from QCVoter/DKGBroker.

type FallbackStrategy interface { // return index of client to use ClientIndex() int // indicate successful use of client at index Success(int) // indicate failure to use client at index Failure(int) } // broker.go type Broker struct { fallbackStrategy FallbackStrategy clients []DKGContractClient } // ... when using a client clientIndex := fallbackStrategy.ClientIndex() err := clients[clientIndex].CallSomeFunction() if err != nil { fallbackStrategy.Failure(clientIndex) } else { fallbackStrategy.Sucess(clientIndex) }

This is probably an (excellent) idea that deserves its own issue.

jordanschalm · 2021-09-30T17:46:30Z

module/epochs/qc_voter.go

+		me:                me,
+		signer:            signer,
+		state:             state,
+		qcContractClients: contractClients,


can we initialize activeQCContractClient to 0 here, just so it is explicit and the reader doesn't have to know about Go's initialization rules to reason about the behaviour. (same for Broker)

jordanschalm · 2021-09-30T17:48:40Z

module/epochs/qc_voter.go

+
+		// retry with next fallback client every 2 attempts
+		if attempts%2 == 0 {
+			log.Info().Msgf("retrying on attempt (%x) with fallback access node", attempts)


Suggested change

log.Info().Msgf("retrying on attempt (%x) with fallback access node", attempts)

log.Info().Msgf("retrying on attempt (%d) with fallback access node", attempts)

%x is hex

we always used the public key from the first AN, making any fallback attempts useless (wrong key)

vishalchangrani · 2021-10-01T17:54:34Z

cmd/util/cmd/common/flow_client.go

 )

-// SecureFlowClient creates a flow client with secured GRPC connection
-func SecureFlowClient(accessAddress, accessApiNodePubKey string) (*client.Client, error) {
+type FlowClientOpt struct {


I would call this FlowClientConfig since they are configs instead of options

vishalchangrani · 2021-10-01T17:55:24Z

cmd/util/cmd/common/flow_client.go

@@ -47,3 +79,61 @@ func InsecureFlowClient(accessAddress string) (*client.Client, error) {

 	return flowClient, nil
 }
+
+// PrepareFlowClientOpts will assemble connection options for the flow client for each access node id
+func PrepareFlowClientOpts(accessNodeIDS []string, insecureAccessAPI bool, snapshot protocol.Snapshot) ([]*FlowClientOpt, error) {


Suggested change

func PrepareFlowClientOpts(accessNodeIDS []string, insecureAccessAPI bool, snapshot protocol.Snapshot) ([]*FlowClientOpt, error) {

func FlowClientConfigs(accessNodeIDS []string, insecureAccessAPI bool, snapshot protocol.Snapshot) ([]*FlowClientConfig, error) {

vishalchangrani · 2021-10-01T17:58:00Z

cmd/util/cmd/common/flow_client.go

+
+// ConvertAccessAddrFromState takes raw network address from the protocol state in the for of [DNS/IP]:PORT, removes the port and applies the appropriate
+// port number depending on the insecureAccessAPI arg.
+func ConvertAccessAddrFromState(address string, insecureAccessAPI bool) string {


Suggested change

func ConvertAccessAddrFromState(address string, insecureAccessAPI bool) string {

func convertAccessAddrFromState(address string, insecureAccessAPI bool) string {

vishalchangrani

left a couple of nits but otherwise lgtm

…ccess-node-fallbacks

huitseeker

Thanks a bunch! This is probably close to done, I left a bunch of mostly-nits and will do a second pass soon.

huitseeker · 2021-10-01T17:51:06Z

cmd/collection/main.go

 		flags.BoolVar(&insecureAccessAPI, "insecure-access-api", true, "required if insecure GRPC connection should be used")
+		flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, "array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. minimum length 2")


Could this refer to DefaultAccessNodeIDSMinimum, if there's not dependency cycle?

huitseeker · 2021-10-01T17:57:24Z

cmd/util/cmd/common/flow_client.go

+
+	// make sure we have identities for all the access node IDs provided
+	if len(identities) != len(accessNodeIDS) {
+		return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %v", accessNodeIDS, identities.NodeIDs())


Suggested change

return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %v", accessNodeIDS, identities.NodeIDs())

return nil, fmt.Errorf("failed to get %v distinct identities for all the access node IDs provided: %v, found %v", len(accessNodeIDs), accessNodeIDS, identities.NodeIDs())

Nit: the user may repeat a single access node ID parameter in the arguments to the client, in the case where they only want to contact one (their) access node, thereby bypassing the fallback.

One way to solve this is to allow just one access node as a parameter, and if the overall issue was any less urgent, I'd actually suggest it.

But in so far as we won't come back to that, let's at least help with an error message that takes the repetition into account here.

huitseeker · 2021-10-01T17:59:28Z

integration/localnet/bootstrap.go

@@ -251,7 +254,7 @@ type Build struct {
 	Target     string
 }

-func prepareServices(containers []testnet.ContainerConfig, secureAccessNodeID string) Services {
+func prepareServices(containers []testnet.ContainerConfig, secureAccessNodeIDS string) Services {


Nit: if secure ANs have become an irrelevant concept, let's remove the adjective from those parameter names as well.

huitseeker · 2021-10-01T18:01:46Z

module/dkg/broker.go

+		committee:          committee,
+		me:                 me,
+		myIndex:            myIndex,
+		dkgContractClients: dkgContractClients,


This is probably an (excellent) idea that deserves its own issue.

huitseeker · 2021-10-01T18:09:53Z

integration/testnet/network.go

+			accessNodeIDS = append(accessNodeIDS, n.NodeID.String())
+		}
+	}
+	require.True(t,  len(accessNodeIDS) > 1, "at-least 2 access node that is not a ghost must be configured for test suite")


nit

Suggested change

require.True(t, len(accessNodeIDS) > 1, "at-least 2 access node that is not a ghost must be configured for test suite")

require.True(t, len(accessNodeIDS) > 1, "at-least 2 access node that is not a ghost must be configured for test suite")

huitseeker · 2021-10-01T18:19:17Z

module/dkg/broker.go

+				// retry with next fallback client after 2 failed attempts
+				if attempts%2 == 0 {
+					b.log.Warn().Msgf("broadcast: retrying on attempt (%x) with fallback access node", attempts)
+					b.updateActiveDKGContractClient()


Here I would actually call the update first, and then print the activeQCContractClient in the message, so that the engineer trying to debug this from logs will know which node is attempted when.

huitseeker · 2021-10-01T18:19:34Z

module/dkg/broker.go

+		// retry with next fallback client after 2 failed attempts
+		if attempts%2 == 0 {
+			b.log.Warn().Msgf("submit result: retrying on attempt (%d) with fallback access node", attempts)
+			b.updateActiveDKGContractClient()


Same as above: here I would actually call the update first, and then print the activeQCContractClient in the message, so that the engineer trying to debug this from logs will know which node is attempted when.

Nit: missed the change from d8c0995 here

huitseeker · 2021-10-01T18:19:51Z

module/epochs/qc_voter.go

+		// retry with next fallback client after 2 failed attempts
+		if attempts%2 == 0 {
+			log.Warn().Msgf("retrying on attempt (%d) with fallback access node", attempts)
+			voter.updateActiveQcContractClient()


Same as elsewhere: I'd print the activeQCContractClient.

huitseeker · 2021-10-01T18:21:11Z

cmd/consensus/main.go

 		flags.BoolVar(&insecureAccessAPI, "insecure-access-api", true, "required if insecure GRPC connection should be used")
+		flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, "array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. minimum length 2")


Same comment: it would be great if this could use the constant (provided it introduces no dependency cycles)

huitseeker · 2021-10-01T18:28:58Z

cmd/util/cmd/common/flow_client.go

+	for i, identity := range identities {
+		accessAddress := ConvertAccessAddrFromState(identity.Address, insecureAccessAPI)
+
+		// remove the 0x prefix from network public keys


Consider using TrimPrefix instead, as you'll be removing key information and getting it reported as "invalid" or "wrong length" later down the line if the initial characters aren't what you expect.

- use trim prefix

huitseeker

LGTM!

huitseeker · 2021-10-01T20:44:47Z

module/dkg/broker.go

+		// retry with next fallback client after 2 failed attempts
+		if attempts%2 == 0 {
+			b.log.Warn().Msgf("submit result: retrying on attempt (%d) with fallback access node", attempts)
+			b.updateActiveDKGContractClient()


Nit: missed the change from d8c0995 here

…/flow-go into khalil/5844-access-node-fallbacks

jordanschalm · 2021-10-01T23:01:03Z

bors merge

bors · 2021-10-01T23:20:41Z

Build succeeded:

kc1116 added 6 commits September 28, 2021 15:20

update QCVoter and DKG Broker to handle access node fallbacks

51846f8

- update CLI for LN/SN nodes to remove access-address, and secure-access-node-api flags - add --access-node-ids flag - update unit tests - add fallback logic to retry logic of Broadcast and Vote

lint fixes

e325f99

update localnet

b90ca6b

- update local net flags for LN/SN nodes - update default access node count to 2 - put flow client config prep in common function

update integration tests

43da870

- update integration tests flags and container configs (from #1323)

add missing error return when node fails to prepare flow client conn …

7e6ed6c

…opts

Update flow_client.go

40c9850

kc1116 requested review from AlexHentschel, jordanschalm, ramtinms, yhassanzadeh13 and zhangchiqing as code owners September 30, 2021 16:38

kc1116 added 2 commits September 30, 2021 12:46

Merge branch 'master' of github.com:onflow/flow-go into khalil/5844-a…

de10bd5

…ccess-node-fallbacks

update dkg tests controller factory client arg

8be2355

jordanschalm reviewed Sep 30, 2021

View reviewed changes

kc1116 added 2 commits September 30, 2021 22:03

update comments and log statement formatting

6303825

Update flow_client.go

492c35c

jordanschalm force-pushed the khalil/5844-access-node-fallbacks branch from a24564d to 492c35c Compare October 1, 2021 15:59

jordanschalm added 3 commits October 1, 2021 09:03

add fallback to poll/submitResult

91b2c54

add log

14b2282

fix bug creating flow client opts

db9970d

we always used the public key from the first AN, making any fallback attempts useless (wrong key)

vishalchangrani reviewed Oct 1, 2021

View reviewed changes

vishalchangrani approved these changes Oct 1, 2021

View reviewed changes

jordanschalm and others added 3 commits October 1, 2021 11:23

maintain ordering of ANs

2d8b4c2

update var names

e925198

Merge branch 'master' of github.com:onflow/flow-go into khalil/5844-a…

db73f45

…ccess-node-fallbacks

huitseeker reviewed Oct 1, 2021

View reviewed changes

kc1116 requested a review from huitseeker October 1, 2021 18:34

update flag help string, improve logging,

d8c0995

- use trim prefix

jordanschalm approved these changes Oct 1, 2021

View reviewed changes

jordanschalm and others added 2 commits October 1, 2021 12:39

Merge branch 'master' into khalil/5844-access-node-fallbacks

d88b2ca

Merge branch 'master' into khalil/5844-access-node-fallbacks

bf0de79

huitseeker approved these changes Oct 1, 2021

View reviewed changes

kc1116 added 2 commits October 1, 2021 17:59

Update broker.go

f8f992a

Merge branch 'khalil/5844-access-node-fallbacks' of github.com:onflow…

78a0825

…/flow-go into khalil/5844-access-node-fallbacks

bors bot merged commit 5b3a98a into master Oct 1, 2021

bors bot deleted the khalil/5844-access-node-fallbacks branch October 1, 2021 23:20

jordanschalm mentioned this pull request Oct 2, 2021

[v0.22] Access Node Fallbacks #1415

Merged

	return fmt.Errorf("invalid flag --access-node-ids atleast %x IDs must be provided", common.DefaultAccessNodeIDSMinimum)
	return fmt.Errorf("invalid flag --access-node-ids atleast %d IDs must be provided", common.DefaultAccessNodeIDSMinimum)

	return nil, fmt.Errorf("could not get flow identifer from secured access node id: %s", id)
	return nil, fmt.Errorf("could not get flow identifer from secured access node id (%s): %w", id, err)

	return nil, fmt.Errorf("failed get identities access node IDs from snapshot: %v", accessNodeIDS)
	return nil, fmt.Errorf("failed get identities access node identities (ids=%v) from snapshot: %w", accessNodeIDS, err)

	return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %s", accessNodeIDS, identities)
	return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %v", accessNodeIDS, identities.NodeIDs())

	// retry with next fallback client every 2 attempts
	// retry with next fallback client after 2 failed attempts

	log.Info().Msgf("retrying on attempt (%x) with fallback access node", attempts)
	log.Info().Msgf("retrying on attempt (%d) with fallback access node", attempts)

	func PrepareFlowClientOpts(accessNodeIDS []string, insecureAccessAPI bool, snapshot protocol.Snapshot) ([]*FlowClientOpt, error) {
	func FlowClientConfigs(accessNodeIDS []string, insecureAccessAPI bool, snapshot protocol.Snapshot) ([]*FlowClientConfig, error) {

	func ConvertAccessAddrFromState(address string, insecureAccessAPI bool) string {
	func convertAccessAddrFromState(address string, insecureAccessAPI bool) string {

		flags.BoolVar(&insecureAccessAPI, "insecure-access-api", true, "required if insecure GRPC connection should be used")
		flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, "array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. minimum length 2")

	return nil, fmt.Errorf("failed to get identity for all the access node IDs provided: %v, got %v", accessNodeIDS, identities.NodeIDs())
	return nil, fmt.Errorf("failed to get %v distinct identities for all the access node IDs provided: %v, found %v", len(accessNodeIDs), accessNodeIDS, identities.NodeIDs())

	require.True(t, len(accessNodeIDS) > 1, "at-least 2 access node that is not a ghost must be configured for test suite")
	require.True(t, len(accessNodeIDS) > 1, "at-least 2 access node that is not a ghost must be configured for test suite")

Khalil/5844 access node fallbacks #1395

Khalil/5844 access node fallbacks #1395

Conversation

kc1116 commented Sep 30, 2021

codecov-commenter commented Sep 30, 2021 • edited

Codecov Report

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

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

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

vishalchangrani left a comment

Choose a reason for hiding this comment

huitseeker left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Oct 1, 2021

bors bot commented Oct 1, 2021

codecov-commenter commented Sep 30, 2021 •

edited