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.

Sign up for GitHub

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

Jump to bottom

add QUIC support #5350

Merged
merged 5 commits into from
Aug 31, 2018
Merged

add QUIC support #5350

merged 5 commits into from
Aug 31, 2018

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Aug 7, 2018 •
edited
Loading

Fixes #5343.

@marten-seemann marten-seemann force-pushed the feat/quic branch 2 times, most recently from da95d79 to 7c3fe23 Compare August 7, 2018 08:10
Stebalien
Stebalien reviewed Aug 13, 2018
View reviewed changes
docs/experimental-features.md

- [ ] The IETF QUIC specification needs to be finalised.
- [ ] Make sure QUIC connections work reliably
- [ ] Make sure QUIC connection offer equal or better performance than TCP connections on real world networks
Copy link
Member

Choose a reason for hiding this comment

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

Finalize libp2p-TLS handshake spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Aug 15, 2018
@Stebalien
Copy link
Member

This is blocked, right?

@marten-seemann
Copy link
Member Author

Yes, we need to merge a few gx publishes.

@marten-seemann
Copy link
Member Author

@Stebalien, @Kubuxu: This PR is ready for merging now. Please merge ipfs/go-ipfs-config#4 before.

@ghost ghost assigned Stebalien Aug 26, 2018
@ghost ghost added the status/in-progress In progress label Aug 26, 2018
@Stebalien Stebalien added RFM and removed status/blocked Unable to be worked further until needs are met status/in-progress In progress labels Aug 26, 2018
magik6k
magik6k reviewed Aug 26, 2018
View reviewed changes
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Would be nice to get a sharness test or two, other than that looks good

@marten-seemann
Copy link
Member Author

@magik6k: Thanks for the hint. I wasn't aware of the sharness tests before. This is exactly the kind of test we should have to test that we don't accidentally break QUIC support in the future.
I added a test case performing a ping using QUIC. Please have another look.

@Stebalien Stebalien added need/review Needs a review and removed RFM labels Aug 26, 2018
@Stebalien
Copy link
Member

@marten-seemann looks like one of the tests is hanging.

@Stebalien
Copy link
Member

Actually, it would be kind of nice to have a "twonode" test. Just add a variant that uses quic to the twonode sharness test.

magik6k
magik6k reviewed Aug 26, 2018
View reviewed changes
test/sharness/t0190-quic-ping.sh Outdated
'

test_expect_success "test ping self" '
! ipfsi 0 ping -n2 -- "$PEERID_0" &&
Copy link
Member

@magik6k magik6k Aug 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The standard way to check that a command fails in sharness is test_must_fail ipfsi 0 ping -n2 -- "$PEERID_0"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. I just copied it from the ping test: https://github.com/ipfs/go-ipfs/blob/93c4f19169e0570adfb35fb6e9420d414c64a733/test/sharness/t0041-ping.sh#L26-L34

@marten-seemann
Copy link
Member Author

@Stebalien: I just added a copy of the two-node test that uses QUIC. PHAL.

Note that this test is currently failing, which is due to this bug in quic-go: quic-go/quic-go#1499. I'll cherry-pick the fix for this issue once it is reviewed, and bubble up the fix through gx.

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Aug 27, 2018
@marten-seemann
Copy link
Member Author

I rebased everything to include the current version of quic-go.
Tests are failing because now there's a gx version mismatch of go-libp2p-transport. Looks like we need to update that one first here...

@Stebalien
Copy link
Member

I'll do that once merge some stuff (keep needing to rebase PRs).

@marten-seemann
update go-ipfs-config to version 0.2.5
1c57b1d 
License: MIT
Signed-off-by: Marten Seemann <martenseemann@gmail.com>
@marten-seemann
add QUIC support
481d19e 
License: MIT
Signed-off-by: Marten Seemann <martenseemann@gmail.com>
@marten-seemann
Copy link
Member Author

@Kubuxu: Can you review this please? This should be ready for merge now.

@bigs
Copy link
Contributor

bigs commented Aug 29, 2018

gx renders aside, this change looks good to me.

Stebalien
Stebalien reviewed Aug 30, 2018
View reviewed changes
test/sharness/t0126-twonode-quic.sh Outdated
@@ -0,0 +1,109 @@
#!/usr/bin/env bash

test_description="Test two ipfs nodes transferring a file via QUIC"
Copy link
Member

Choose a reason for hiding this comment

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

Let's just add a case to the existing twonode test. It's designed for multiple variants (just run run_advanced_test again after configuring the nodes to use QUIC).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was worried about the test output. By putting it in one file, it's hard to see which test exactly is failing. I added some more logging, so that shouldn't be a problem anymore.

@ghost ghost added the status/in-progress In progress label Aug 30, 2018
@Stebalien
Copy link
Member

gx update bonanza done.

@Stebalien
Copy link
Member

I need to pull through a multistream update to get this to work.

@marten-seemann
add a QUIC sharness test
8193a76 
License: MIT
Signed-off-by: Marten Seemann <martenseemann@gmail.com>
@marten-seemann
add a QUIC twonode sharness test
41806cc 
License: MIT
Signed-off-by: Marten Seemann <martenseemann@gmail.com>
@Stebalien
gx update deps
46a1d9d 
License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien Stebalien removed status/blocked Unable to be worked further until needs are met need/review Needs a review labels Aug 31, 2018
@magik6k
Copy link
Member

magik6k commented Aug 31, 2018

ci seems unhappy about deps

@Stebalien
Copy link
Member

ci seems unhappy about deps

It keeps complaining about not being able to fetch deps it really should be able to fetch.

@Stebalien Stebalien merged commit 1b4972b into ipfs:master Aug 31, 2018
@ghost ghost removed the status/in-progress In progress label Aug 31, 2018
@Stebalien
Copy link
Member

We have liftoff!

🎉 🚀 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magik6k magik6k magik6k approved these changes

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@Stebalien Stebalien

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marten-seemann @Stebalien @bigs @magik6k