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

Suppress bootstrap error #5769

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Suppress bootstrap error #5769

merged 1 commit into from
Nov 13, 2018

Conversation

zonque
Copy link
Contributor

@zonque zonque commented Nov 12, 2018

When used in private network mode with Bootstrap set to [], the daemon currently spits out the following message ever 30 seconds:

ERROR       core: no bootstrap nodes configured: go-ipfs may have difficulty connecting to the network bootstrap.go:144

While it is good to mention, I see two issues with this:

  • In regular environments, running the daemon with no bootstrap nodes will in fact render it unusable, so there should be an error at daemon start time instead of a repetitive message
  • For private network setups where nodes discover themselves via MDNS, this message is unnecessary.

This PR changes both the above, with one patch for each of them.

@zonque zonque requested a review from Kubuxu as a code owner November 12, 2018 17:12
@zonque
Copy link
Contributor Author

zonque commented Nov 12, 2018

Hmm, so some test now fail because they instantiate daemons without bootstrap nodes. Can anyone give me a hint on how to quickly fix that?

@Stebalien
Copy link
Member

For private network setups where nodes discover themselves via MDNS, this message is unnecessary.

I'm not sure I agree with this. Really, MDNS and private networks are unrelated (unless you mean private networks via private VPNs). Most private networks (that use swarm keys) will want private bootstrap nodes.

However, I do agree that this is way too noisy. It may make sense to just print a warning once and never again.

@Stebalien
Copy link
Member

The tests are probably failing because bootstrappers can be added and removed at runtime.

Rather than checking for the presence of bootstrap nodes on each iteration
of the deferred function, do the check at initialization time and report an
error immediately.

License: MIT
Signed-off-by: Daniel Mack <daniel@zonque.org>
@zonque
Copy link
Contributor Author

zonque commented Nov 12, 2018

I'm not sure I agree with this. Really, MDNS and private networks are unrelated (unless you mean private networks via private VPNs). Most private networks (that use swarm keys) will want private bootstrap nodes.

However, I do agree that this is way too noisy. It may make sense to just print a warning once and never again.

Hmm, okay. Agreed.

I've force-pushed a new version that does just that.

Thanks for your input!

@Stebalien
Copy link
Member

LGTM but I'd like someone else to take a quick look for a sanity check.

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

LGTM, I would change the error for a warning (but it's just a personal preference so feel free to ignore it).

@schomatis
Copy link
Contributor

The tests are probably failing because bootstrappers can be added and removed at runtime.

Can this be merged with the test errors?

@Stebalien
Copy link
Member

LGTM, I would change the error for a warning (but it's just a personal preference so feel free to ignore it).

We should, but I'd like to enable warnings by default first. Otherwise, users will never see it.

@Stebalien
Copy link
Member

Can this be merged with the test errors?

That has been fixed. The first iteration aborted the construction of the periodic bootstrapper when no bootstrappers were configured. This one just warns and continues.

@Stebalien Stebalien merged commit e3c6776 into ipfs:master Nov 13, 2018
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