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

feat: basic ipfs network diagnostics #7068

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

Stebalien
Copy link
Member

This implements a basic command and subsystem for diagnosing various networking issues.

@Stebalien
Copy link
Member Author

(needs tests)

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Apr 1, 2020
To (your current IP): {{.LocalIP}}
{{- end}}
{{if .Gateway -}}
Your router's administrative console can likely be found at:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is less likely to be actionable in corporate / educational situations. Maybe clearer specification of "if you are on a personal home network" or similar?

doctor/doctor.go Show resolved Hide resolved
@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label May 26, 2020
@autonome
Copy link

@jacobheun Is there an epic/meta issue for debugging/tools/troubleshooting?

@jacobheun
Copy link
Contributor

Is there an epic/meta issue for debugging/tools/troubleshooting?

@autonome not at the moment, but this would be helpful. I think there is some overlap here with metrics/telemetry but they're discrete concerns. I'll open up a meta issue for tracking/discussion.

@aschmahmann
Copy link
Contributor

Just a note, that while I've updated this PR and it looks like it's passing tests we don't really have any tests to make sure this works (testing this is hard bc it's network level/configuration things). I've done a bit of local testing and found the results a little flaky (it wouldn't always realize if I was unreachable).

We'll need to put a bit of time into review + testing before we ship this. Would love to get this in though, it should be super useful.

@aschmahmann
Copy link
Contributor

cc @aarshkshah1992 you may want to modify this PR or use it as inspiration given your work on libp2p/go-libp2p#1042

@aarshkshah1992 aarshkshah1992 self-assigned this Jan 28, 2021
}

n.printf("Firewall Status: %s", status)
if !n.hasNagged {

Choose a reason for hiding this comment

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

@Stebalien Would you remember why you put in this flag ? What's the use case ?

Copy link
Contributor

Choose a reason for hiding this comment

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

it appears to be so that it only says it once, and then remembers that it has said it so it doesn't say it again, right?

n.printf("Firewall Status: %s", status)
if !n.hasNagged {
n.hasNagged = true
n.printf("NOTE: Your node appears to be behind a firewall. Please run `ipfs diag net` to diagnose.")
Copy link

@aarshkshah1992 aarshkshah1992 Jan 28, 2021

Choose a reason for hiding this comment

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

@Stebalien

How can we be sure here that we are behind a firewall ?

@aarshkshah1992
Copy link

@willscott @aschmahmann I've made some small changes and have updated the deps and rebased. This PR looks in pretty good shape. Please can. I get a review ?

Will augment it with the NAT Type event on a separate PR as that work will time time to get merged.

@willscott
Copy link
Contributor

@aarshkshah1992 - looking at this conversation, the main thing this PR was waiting for was some sort of testing to gain confidence that we tell people the right things given their circumstances, and that future changes don't break this. Thoughts on any clear paths to gain confidence in this code?

@willscott
Copy link
Contributor

concretely: lets put in a sanity test: run a daemon. run this command. make sure daemon and command exit cleanly.

@aschmahmann aschmahmann marked this pull request as draft September 15, 2021 15:04
@aschmahmann
Copy link
Contributor

This is needs a rebase and may be dependent on some updates in go-libp2p to support detecting the ability to do NAT traversal.

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.

6 participants