Hi Willy.
On 25.02.22 14:54, Willy Tarreau wrote:
Hi Alex,
On Thu, Feb 24, 2022 at 03:03:59AM +0100, Aleksandar Lazic wrote:
Hi.
Here the first patch for feature request "New Balancing algorithm (Peak) EWMA
#1570"
Note, I don't think it is needed for this algo as long as we instead
use measured response time and/or health check time. But regardless
it's something useful to have. A few comments below:
Thanks you for your much valuable feedback.
I think also that the rtt information as fetch sample could be useful.
From e95bf6a4bf107fdc59696c4b4a4ef7b03133b813 Mon Sep 17 00:00:00 2001
From: Aleksandar Lazic <al-hapr...@none.at>
Date: Thu, 24 Feb 2022 02:56:21 +0100
Subject: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample
This sample fetch get the server round trip time
You should mention "TCP round trip time" since it's measured at the TCP
level.
+srv_rtt : integer
+ Returns the Round Trip Time (RTT) measured by the kernel for the server
+ connection. <unit> is facultative, by default the unit is milliseconds.
<unit>
+ can be set to "ms" for milliseconds or "us" for microseconds. If the server
+ connection is not established, if the connection is not TCP or if the
+ operating system does not support TCP_INFO, for example Linux kernels before
+ 2.4, the sample fetch fails.
I would rather call it "bc_rtt" since it's not the server but the backend
connection. Technically speaking it indeed requires a connection to be
established and will only report the value for *this* connection and not
anything stateless related to the server. Thta's more in line with what we
have for the frontend connection already with fc_rtt.
You mentioned "unit" but it does not appear in the keyword syntax.
Also I think it would be useful to have all other fc_* from the same
section turned to bc_* (fc_rttvar, fc_retrans, etc), as it can sometimes
explain long response times in logs.
diff --git a/reg-tests/sample_fetches/srv_rtt.vtc
b/reg-tests/sample_fetches/srv_rtt.vtc
new file mode 100644
index 000000000..c0ad0cbae
--- /dev/null
+++ b/reg-tests/sample_fetches/srv_rtt.vtc
@@ -0,0 +1,34 @@
+varnishtest "srv_rtt sample fetch Test"
+
+#REQUIRE_VERSION=2.6
Note, we *might* need to add a new macro to detect support for TCP_INFO.
We still don't have the config predicates to detect support for certain
keywords or sample fetch functions so that's not easy, but it's possible
that this test will break on some OS like cygwin. If so we could work
around this temporarily using "EXCLUDE_TARGETS" and in the worst case we
could mark it broken for the time it takes to completely solve this.
Agree here the suggestion is to add something like USE_GETSOCKOPT to be able to
make '#REQUIRE_OPTIONS=GETSOCKOPT',
something like similar to USE_GETADDRINFO, right?
(...)
diff --git a/src/tcp_sample.c b/src/tcp_sample.c
index 19edcd243..7b8b616cb 100644
--- a/src/tcp_sample.c
+++ b/src/tcp_sample.c
@@ -446,6 +446,20 @@ smp_fetch_fc_reordering(const struct arg *args, struct
sample *smp, const char *
return 0;
return 1;
}
+
+/* get the mean rtt of a client connection */
+static int
+smp_fetch_srv_rtt(const struct arg *args, struct sample *smp, const char *kw,
void *private)
+{
+ if (!get_tcp_info(args, smp, 1, 0))
+ return 0;
+
+ /* By default or if explicitly specified, convert rtt to ms */
+ if (!args || args[0].type == ARGT_STOP || args[0].data.sint ==
TIME_UNIT_MS)
+ smp->data.u.sint = (smp->data.u.sint + 500) / 1000;
+
+ return 1;
+}
That's another reason for extending the existing keywords, avoiding code
duplication. You can have all your new keywords map to the fc_* equivalent
and just change this:
- if (!get_tcp_info(args, smp, 0, 0))
+ if (!get_tcp_info(args, smp, *kw == 'b', 0))
Please update the comments on top of the functions to mention that they're
called for both "fc_*" and "bc_*" depending on the side, and that's OK.
Let me go back to the "drawing board" and will send a update as soon as there
is a update :-)
thanks,
Willy
Regards
Alex
