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

swarm/network: WIP consider all nodes for healthy iteration #19155

Merged
merged 10 commits into from
Feb 28, 2019

Conversation

nolash
Copy link
Contributor

@nolash nolash commented Feb 22, 2019

addresses ethersphere/swarm#1263

This PR corrects two problems with the existing WaitTillHealthy implementation.

  1. The healthy check only called ConnectNN, should instead call Healthy()
  2. The kademlias being checked were only from the nodes that were up at the time. It should instead include all nodes that are in the simulation.

swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Show resolved Hide resolved
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

LGTM except for a few code-style comments.
The strange thing about this is that I had a feeling that the AddNodes and ConnectNodesX would actually block. Judging by the results of the snapshot it means that both these methods return and potentially before the nodes were even up? This is quite strange. Moreover considering that the Node.Up is set to be true when creating the node - they should all be up after create.go#L82. We should add a panic in Net.ConnectX when the Up node count does not equal the parameter value.

@@ -85,6 +86,7 @@ func (s *Simulation) WaitTillHealthy(ctx context.Context) (ill map[enode.ID]*net
// in simulation bucket.
func (s *Simulation) kademlias() (ks map[enode.ID]*network.Kademlia) {
items := s.UpNodesItems(BucketKeyKademlia)
log.Debug("kademlia len items", "len", len(items))
Copy link
Member

Choose a reason for hiding this comment

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

can be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
@zelig
Copy link
Contributor

zelig commented Feb 25, 2019

@holisticode there is a consistent CI failure on snapshot creation
https://travis-ci.org/ethereum/go-ethereum/jobs/497949535#L816

swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
@skylenet skylenet added this to the 1.9.0 milestone Feb 25, 2019
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Show resolved Hide resolved
swarm/network/simulation/kademlia_test.go Outdated Show resolved Hide resolved
@@ -76,17 +79,17 @@ func createSnapshot(filename string, nodes int, services []string) (err error) {
})
defer sim.Close()

_, err = sim.AddNodes(nodes)
ids, err := sim.AddNodes(nodes)
Copy link
Contributor

Choose a reason for hiding this comment

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

should just use AddNodesAndConnectRing here

kiku-jw pushed a commit to kiku-jw/go-ethereum that referenced this pull request Mar 29, 2019
…#19155)

* swarm/network: WIP consider all nodes for healthy iteration

* swarm/network/simulation: extend TestWaitTillHealthy to really check kads are healthy

* cmd/swarm/swarm-snapshot: fixed bugs in snapshot creation binary

* swarm/network/simulation: addressed PR comments

* swarm/network/simulation: defer sim.Clsoe()

* swarm/network/simulation: fixed wrong sim.Close()

* swarm/network/simulation: addressed PR comments

* cmd/swarm/swarm-snapshot: reducing default to 8 nodes, more to 4

* cmd/swarm/swarm-snapshot: extended timeout to 3 mins, or 256 nodes snapshot times out

* swarm/network/simulation: More PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants