Hi Willy, On 30/05/2024 15:48, Willy Tarreau wrote: > Hi Dorian, > > I'm now done with the release and having more time to read your > work. First, thanks for this update. I understand that you're almost > running out of time on this topic which must be completed before > June so I'm not going to make you waste your time. Some comments > below.
Thank you for your reply! I hope you don't mind if I reply instead of Dorian, as he is no longer available to work on that. > On Thu, May 16, 2024 at 03:53:40PM +0200, Dorian Craps wrote: >> From: Dorian Craps <dorian.cr...@student.vinci.be> >> >> Multipath TCP (MPTCP), standardized in RFC8684 [1], is a TCP extension >> that enables a TCP connection to use different paths. >> >> Multipath TCP has been used for several use cases. On smartphones, MPTCP >> enables seamless handovers between cellular and Wi-Fi networks while >> preserving established connections. This use-case is what pushed Apple >> to use MPTCP since 2013 in multiple applications [2]. On dual-stack >> hosts, Multipath TCP enables the TCP connection to automatically use the >> best performing path, either IPv4 or IPv6. If one path fails, MPTCP >> automatically uses the other path. >> >> To benefit from MPTCP, both the client and the server have to support >> it. Multipath TCP is a backward-compatible TCP extension that is enabled >> by default on recent Linux distributions (Debian, Ubuntu, Redhat, ...). >> Multipath TCP is included in the Linux kernel since version 5.6 [3]. To >> use it on Linux, an application must explicitly enable it when creating >> the socket. No need to change anything else in the application. >> >> This attached patch adds the "mptcp" global option in the config, which >> allows the creation of an MPTCP socket instead of TCP on Linux. If >> Multipath TCP is not supported on the system, an error will be reported, >> and the application will stop. >> >> A test has been added, it is a copy of "default_rules.vtc" in tcp-rules >> with the addition of "mptcp" in the config. I'm not sure what else needs >> to be tested for the moment, with this global MPTCP option. >> >> Note: another patch is coming to support enabling MPTCP per address, but >> I prefer to already send this patch, just in case, as I will soon have >> less time to dedicate to this. > > Thanks for this. As I previously mentioned, I'm not going to merge a > patch with a global option that does all or nothing, however having a > working patch like this can definitely serve as a proof-of-concept and > a reference when testing a more granular approach, and in this regard > your patch is much welcome! Thank you, that's clear. I started to look for a solution where MPTCP can be enabled per address [1]. I will try to find more time to drop the global option. [1] https://github.com/matttbe/haproxy/commit/f48f36191 >> Due to the limited impact within a data center environment, >> we have decided not to implement MPTCP between the proxy and the servers. >> The high-speed, low-latency nature of data center networks reduces >> the benefits of MPTCP, making the complexity of its implementation >> unnecessary in this context. > > I definitely understand. However let's keep in mind that whenever we > envision a use case, someone will come with a different one and suggest > that we should support it. Here I can imagine that some CDN operators > might be interested in using MPTCP to the origin server as well by > connecting via multiple paths. This opens a big can of worms with > questions about source address binding, interface binding etc, which > cannot be addressed easily as they'll all have impacts on the way > servers are configured. So I'm totally fine with having only a frontend > support in a first time. I just mentioned this to give you more context > and so that you know it's not only the DC that's on the backend, even > if it's by far the most common, and what some of the complex aspects > can be. Thank you for the explanation! The priority has been put on the frontend side mainly because it looked easier -- thanks to 'sock_prot' from the 'struct protocol' -- and it was addressing the main use-case. That was a good starting point, then the rest would have seen later if there was an interest (and time). > I'll be in vacation for two weeks at the end of this week, will you need > some review of a possible next patch, or did you give up due to some > difficulties you faced while exploring the per-listener configuration ? Dorian faced some difficulties, but he also had to stop to prepare his future exams. On my side, I quickly looked for a solution (see [1] above), but I was waiting for the v3.0.0 release before pushing anything. I can wait for you to be back before sending a version to the mailing list. > I'll give you a few comments on the patch below: Thank you, much appreciated! > >> diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c >> index b173511c9..0feccd4b2 100644 >> --- a/src/cfgparse-global.c >> +++ b/src/cfgparse-global.c >> @@ -23,6 +23,8 @@ >> #include <haproxy/log.h> >> #include <haproxy/peers.h> >> #include <haproxy/protocol.h> >> +#include <haproxy/proto_rhttp.h> >> +#include <haproxy/proto_tcp.h> >> #include <haproxy/tools.h> >> >> int cluster_secret_isset; >> @@ -52,7 +54,7 @@ static const char *common_kw_list[] = { >> "presetenv", "unsetenv", "resetenv", "strict-limits", "localpeer", >> "numa-cpu-mapping", "defaults", "listen", "frontend", "backend", >> "peers", "resolvers", "cluster-secret", "no-quic", "limited-quic", >> - NULL /* must be last */ >> + "mptcp", NULL /* must be last */ >> }; > > Style only: better leave the NULL alone on its line so that it remains > distinct from the rest. It makes sense. In the new version, it should not be an issue because I will drop this global option. > >> /* >> @@ -1334,6 +1336,23 @@ int cfg_parse_global(const char *file, int linenum, >> char **args, int kwm) >> HA_ATOMIC_STORE(&global.anon_key, tmp); >> } >> } >> + else if (strcmp(args[0], "mptcp") == 0) { >> + if (alertif_too_many_args(0, file, linenum, args, &err_code)) >> + goto out; >> +#ifdef __linux__ >> +#ifndef IPPROTO_MPTCP >> +#define IPPROTO_MPTCP 262 >> +#endif > > This would be better placed in include/haproxy/compat.h (there are > already other ones there). Good idea! >> + proto_tcpv4.sock_prot = IPPROTO_MPTCP; >> + proto_tcpv6.sock_prot = IPPROTO_MPTCP; >> + proto_rhttp.sock_prot = IPPROTO_MPTCP; > > In my opinion it should not be needed to assign anything to the rhttp > stuff. I know it's complicated (and I get my mind mixed about it very > often due to the reversed nature), but the protocol here is in fact > used for outgoing connections that later get reversed and assigned to > the listener in an established state. Thank you for the explanation, I didn't realise 'rhttp' was not what I was expecting. > >> +#else >> + ha_alert("parsing [%s:%d]: '%s' is only supported on Linux.\n", >> + file, linenum, args[0]); >> + err_code |= ERR_ALERT | ERR_FATAL; >> + goto out; >> +#endif >> + } >> else { >> struct cfg_kw_list *kwl; >> const char *best; > > Thanks! > Willy Cheers, Matt -- Sponsored by the NGI0 Core fund.
