Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Allow enabling MPTCP on a listening socket through the records.config #4203

Merged
merged 1 commit into from Sep 12, 2018

Conversation

@cpaasch
Copy link

cpaasch commented Sep 5, 2018

This change allows to enable MPTCP on a listener socket. We check whether the running host has the necessary sysctl that indicates that the kernel has been built with the right support.

When the socket-option fails, we just log a message and gracefully continue.

P.S.: Multipath TCP is currently in a forked Linux Kernel at https://multipath-tcp.org.

@shukitchan

This comment has been minimized.

Copy link
Contributor

shukitchan commented Sep 6, 2018

[approve ci]

@zwoop zwoop requested review from bryancall and zwoop Sep 6, 2018
@zwoop zwoop added the Network label Sep 6, 2018
@zwoop zwoop added this to the 9.0.0 milestone Sep 6, 2018
@zwoop

This comment has been minimized.

Copy link
Contributor

zwoop commented Sep 6, 2018

Talking to Bryan, I think we should do a couple of changes, I'll talk to Christoph off-line.

@cpaasch cpaasch force-pushed the cpaasch:enable_mptcp branch from a7a37f3 to dd956b4 Sep 6, 2018
@cpaasch

This comment has been minimized.

Copy link
Author

cpaasch commented Sep 6, 2018

I updated the PR such that one can now configure MPTCP through the "mptcp" port-keyword. That way, one can enable it on a case-by-case basis. There is no default opt-in anymore.

@@ -1025,6 +1049,19 @@ LocalManager::bindProxyPort(HttpProxyPort &port)
}
}

if (port.m_mptcp && mptcp_supported()) {

This comment has been minimized.

Copy link
@SolidWallOfCode

SolidWallOfCode Sep 9, 2018

Member

I think it it would be better to not allow m_mptcp to be set if it is not supported. Please look at the transparency support code, which dealt with a similar issue back when transparency support required a special kernel mod. In essence, if the port descriptor has "mptcp" and it's not supported, issue a warning at that point and don't enable m_mptcp. The state of support isn't going to change while TS is running.

This comment has been minimized.

Copy link
@cpaasch

cpaasch Sep 10, 2018

Author

Thanks for the feedback. I did it the way you described, checking for mptcp_supported in processOptions and based on that enable m_mptcp or not.

I just pushed the update to my branch here.

@SolidWallOfCode SolidWallOfCode dismissed their stale review Sep 10, 2018

Bryan approved the changes.

@cpaasch cpaasch force-pushed the cpaasch:enable_mptcp branch from dd956b4 to 73d3c02 Sep 10, 2018
@@ -1025,6 +1033,21 @@ LocalManager::bindProxyPort(HttpProxyPort &port)
}
}

if (port.m_mptcp) {
#if defined(MPTCP_ENABLED)

This comment has been minimized.

Copy link
@zwoop

zwoop Sep 11, 2018

Contributor

This is always defined isn't it?

This comment has been minimized.

Copy link
@cpaasch

cpaasch Sep 11, 2018

Author

Yes - I am changing the if to: #if MPTCP_ENABLED

@cpaasch cpaasch dismissed stale reviews from zwoop and SolidWallOfCode via c3279d5 Sep 11, 2018
@cpaasch cpaasch force-pushed the cpaasch:enable_mptcp branch from 73d3c02 to c3279d5 Sep 11, 2018
@cpaasch cpaasch force-pushed the cpaasch:enable_mptcp branch from c3279d5 to 1832780 Sep 11, 2018
@apache apache deleted a comment from shinrich Sep 11, 2018
@apache apache deleted a comment from randall Sep 11, 2018
Copy link
Contributor

zwoop left a comment

This looks good to me, but would like for @SolidWallOfCode to review before we merge.

@zwoop zwoop merged commit 2755a80 into apache:master Sep 12, 2018
9 checks passed
9 checks passed
Jenkins CentOS Build finished.
Details
Jenkins Clang-Analyzer Build finished.
Details
Jenkins Debian Build finished.
Details
Jenkins FreeBSD Build finished.
Details
Jenkins ICC Build finished.
Details
Jenkins RAT Build finished.
Details
Jenkins Ubuntu Build finished.
Details
Jenkins autest Build finished.
Details
Jenkins clang-format Build finished.
Details
@bryancall bryancall added this to 8.0.1 backports in 8.x releases Sep 27, 2018
@bryancall bryancall moved this from 8.0.1 backports to 8.1.x backports (new LTS) in 8.x releases Oct 11, 2018
@zwoop zwoop moved this from 8.1.x backports (new LTS) to 8.1.0 backports in 8.x releases Dec 4, 2018
@bryancall bryancall modified the milestones: 9.0.0, 8.1.0 Mar 25, 2019
@bryancall bryancall removed this from 8.1.0 backports in 8.x releases Mar 25, 2019
@bryancall

This comment has been minimized.

Copy link
Contributor

bryancall commented Mar 25, 2019

Cherry picked to 8.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.