Hi Sven,
First, thanks for this contribution and the detailed description.
On Tue, Sep 03, 2019 at 12:23:04PM +0000, PR Bot wrote:
> Description:
> Add configuration parameters to control TCP keep-alives:
> * tcp-
> keepalive-time: Idle time before keep-alive probes are sent
> * tcp-
> keepalive-interval: Interval between keep-alive probes
> * tcp-
> keepalive-count: Number of keep-alive probes to send before giving up
> Tested with TCP and HTTP, and with different settings in the default,
> listen, frontend and backend sections.
Note, these are typically the info to place into the commit message, along
with the rationale for this (think that it helps decide whether or not it
should be backported, then later whether or not it should be reverted or
adjusted if it's found to cause a side effect on anything).
> Potential issues:
> * Only tested on Linux.
> * Darwin `#ifdef TCP_KEEPALIVE`
> implemented but untested.
Isn't it the same as OSX ? If so we do have one in the CI test so at
least we can see if it properly builds.
I'm generally fine with the principle, but I'm having a concern about the
unique keyword for frontends and backends. We've had this quite a few
times in the past already (e.g "option nolinger") and it turned out to
be a bad idea :
- setting it in a "defaults" section will automatically apply to
both sides
- it's impossible to use different settings on the two sides in a
"listen" section.
Just like we've had distinct (and ugly) "clitcpka" and "srvtcpka", we'd
need to have distinct settings depending on the side. I suggest you prefix
them with "cli" and "srv" or "fc" and "bc" (since we have such prefixes in
sample fetches for "front connection" and "back connection") or "fe-" and
"be-" that we also have, that makes it clear what side it acts on. You'll
just have to do the same in the proxy structure, to have the two added
there. Just try to be imaginative to find good names ;-)
> diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c
> index 656031b0a..4b32fe0c3 100644
> --- a/src/cfgparse-listen.c
> +++ b/src/cfgparse-listen.c
> @@ -3257,6 +3257,36 @@ int cfg_parse_listen(const char *file, int linenum,
> char **args, int kwm)
> if (alertif_too_many_args(1, file, linenum, args, &err_code))
> goto out;
> }
> + else if (!strcmp(args[0], "tcp-keepalive-time")) { /* TCP keep-alive
> idle time */
> + if (*(args[1]) == 0) {
> + ha_alert("parsing [%s:%d] : '%s' expects an integer
> argument.\n", file, linenum, args[0]);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> + curproxy->tcp_keepalive_time = atol(args[1]);
> + if (alertif_too_many_args(1, file, linenum, args, &err_code))
> + goto out;
> + }
> + else if (!strcmp(args[0], "tcp-keepalive-interval")) { /* TCP
> keep-alive probe retry interval */
> + if (*(args[1]) == 0) {
> + ha_alert("parsing [%s:%d] : '%s' expects an integer
> argument.\n", file, linenum, args[0]);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> + curproxy->tcp_keepalive_interval = atol(args[1]);
> + if (alertif_too_many_args(1, file, linenum, args, &err_code))
> + goto out;
> + }
> + else if (!strcmp(args[0], "tcp-keepalive-count")) { /* TCP keep-alive
> probe retry count */
> + if (*(args[1]) == 0) {
> + ha_alert("parsing [%s:%d] : '%s' expects an integer
> argument.\n", file, linenum, args[0]);
> + err_code |= ERR_ALERT | ERR_FATAL;
> + goto out;
> + }
> + curproxy->tcp_keepalive_count = atol(args[1]);
> + if (alertif_too_many_args(1, file, linenum, args, &err_code))
> + goto out;
> + }
> else if (!strcmp(args[0], "dispatch")) { /* dispatch address */
> struct sockaddr_storage *sk;
> int port1, port2;
It would be better to register keywords and their respective parsers.
We're trying to put cfgparse on a diet. Have a look at "cfg_kws" in
src/proxy.c to get an example. You may even place it there by the
way.
> diff --git a/src/proto_tcp.c b/src/proto_tcp.c
> index ff252e891..819cb7868 100644
> --- a/src/proto_tcp.c
> +++ b/src/proto_tcp.c
> @@ -389,6 +389,52 @@ int tcp_connect_server(struct connection *conn, int
> flags)
> if (be->options & PR_O_TCP_SRV_KA)
> setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &one, sizeof(one));
>
> +#ifdef TCP_KEEPALIVE
> + /* Darwin */
> + if (be->tcp_keepalive_time &&
> + (setsockopt(fd, IPPROTO_TCP, TCP_KEEPALIVE
> + &be->tcp_keepalive_time,
> + sizeof(be->tcp_keepalive_time)) == -1)) {
> + ha_alert("Failed to set TCP keepalive time: %s. Aborting.\n",
> + strerror(errno));
> + return SF_ERR_INTERNAL;
> + }
> +#endif
> +
> +#ifdef TCP_KEEPIDLE
> + /* Linux, NetBSD */
> + if (be->tcp_keepalive_time &&
> + (setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE,
> + &be->tcp_keepalive_time,
> + sizeof(be->tcp_keepalive_time)) == -1)) {
> + ha_alert("Failed to set TCP keepalive time: %s. Aborting.\n",
> + strerror(errno));
> + return SF_ERR_INTERNAL;
> + }
> +#endif
Here I'd suggest limiting the ifdefs and code duplication, we have
common/compat.h to deal with such variants. You can for example define
HA_TCP_KEEPALIVE_TIME when the macros above are set, then you just have
to have one instance of the call above using this macro and surrounded
by a test on its definition. Same for the 2 other ones.
Thanks!
Willy
