This is an automated email from the ASF dual-hosted git repository.
serrislew pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new bc0bca7be4 Fix enabling per-server metrics that disables the outbound
keep-alive minimum (#13480)
bc0bca7be4 is described below
commit bc0bca7be4342d0a03c6481d290dab3a143671a1
Author: Serris Santos <[email protected]>
AuthorDate: Tue Aug 4 15:20:45 2026 -0700
Fix enabling per-server metrics that disables the outbound keep-alive
minimum (#13480)
* Fix enabling per-server metrics that disables the outbound keep-alive
minimum
* int to auto from copilot
* Add autest
---
include/iocore/net/ConnectionTracker.h | 10 +-
src/iocore/net/ConnectionTracker.cc | 32 ++----
.../per_server_metric_enabled.replay.yaml | 53 ++++++++++
.../per_server_metric_enabled.test.py | 113 +++++++++++++++++++++
4 files changed, 179 insertions(+), 29 deletions(-)
diff --git a/include/iocore/net/ConnectionTracker.h
b/include/iocore/net/ConnectionTracker.h
index 360fda7b64..ae3691fdbe 100644
--- a/include/iocore/net/ConnectionTracker.h
+++ b/include/iocore/net/ConnectionTracker.h
@@ -449,13 +449,13 @@ inline int
ConnectionTracker::TxnState::reserve()
{
_reserved_p = true;
- // If metric enabled, use metric as count
+ // @a _count is always the authoritative count; the metrics, if enabled,
only mirror it.
+ auto count = ++_g->_count;
if (_g->_count_metric != nullptr) {
ts::Metrics::Gauge::increment(_g->_count_metric);
ts::Metrics::Counter::increment(_g->_count_total_metric);
- return _g->_count_metric->load();
}
- return ++_g->_count;
+ return count;
}
inline void
@@ -463,11 +463,9 @@ ConnectionTracker::TxnState::release()
{
if (_reserved_p) {
_reserved_p = false;
- // If metric enabled, use metric as count
+ --_g->_count;
if (_g->_count_metric != nullptr) {
ts::Metrics::Gauge::decrement(_g->_count_metric);
- } else {
- --_g->_count;
}
}
}
diff --git a/src/iocore/net/ConnectionTracker.cc
b/src/iocore/net/ConnectionTracker.cc
index b7ea956723..45ce7e60f0 100644
--- a/src/iocore/net/ConnectionTracker.cc
+++ b/src/iocore/net/ConnectionTracker.cc
@@ -187,8 +187,8 @@
Groups_To_JSON(std::vector<std::shared_ptr<ConnectionTracker::Group const>> cons
static const std::string_view trailer{" \n]}"};
static const auto printer = [](swoc::BufferWriter &w,
ConnectionTracker::Group const *g) -> swoc::BufferWriter & {
- w.print(item_fmt, g->_match_type, g->_addr, g->_fqdn, g->_count_metric !=
nullptr ? g->_count_metric->load() : g->_count.load(),
- g->_count_max.load(), g->_blocked.load(),
g->get_last_alert_epoch_time());
+ w.print(item_fmt, g->_match_type, g->_addr, g->_fqdn, g->_count.load(),
g->_count_max.load(), g->_blocked.load(),
+ g->get_last_alert_epoch_time());
return w;
};
@@ -522,26 +522,13 @@ ConnectionTracker::Group::should_alert(std::time_t *lat)
void
ConnectionTracker::Group::release()
{
- // If metric enabled, use metric as count
- if (_count_metric != nullptr) {
- if (_count_metric->load() > 0) {
+ // @a _count is always the authoritative count; the metric, if enabled, only
mirrors it.
+ if (_count > 0) {
+ auto count = --_count;
+ if (_count_metric != nullptr) {
ts::Metrics::Gauge::decrement(_count_metric);
- if (_count_metric->load() == 0) {
- TableSingleton &table = _direction ==
DirectionType::INBOUND ? _inbound_table : _outbound_table;
- std::lock_guard<std::mutex> lock(table._mutex); // Table lock
- if (_count_metric->load() > 0) {
- // Someone else grabbed the Group between our last check and taking
the
- // lock.
- return;
- }
- table._table.erase(_key);
- }
- } else {
- // A bit dubious, as there's no guarantee it's still negative, but even
that would be interesting to know.
- Error("Number of tracked connections should be greater than or equal to
zero: %" PRId64, _count_metric->load());
}
- } else if (_count > 0) {
- if (--_count == 0) {
+ if (count == 0) {
TableSingleton &table = _direction == DirectionType::INBOUND
? _inbound_table : _outbound_table;
std::lock_guard<std::mutex> lock(table._mutex); // Table lock
if (_count > 0) {
@@ -553,7 +540,7 @@ ConnectionTracker::Group::release()
}
} else {
// A bit dubious, as there's no guarantee it's still negative, but even
that would be interesting to know.
- Error("Number of tracked connections should be greater than or equal to
zero: %u", _count.load());
+ Error("Number of tracked connections should be greater than or equal to
zero: %d", _count.load());
}
}
@@ -615,8 +602,7 @@ ConnectionTracker::dump_outbound(FILE *f)
for (std::shared_ptr<Group const> g : groups) {
swoc::LocalBufferWriter<128> w;
- w.print("{:7} | {:5} | {:24} | {:33} | {:8} |\n", (g->_count_metric !=
nullptr ? g->_count_metric->load() : g->_count.load()),
- g->_blocked.load(), g->_addr, g->_hash, g->_match_type);
+ w.print("{:7} | {:5} | {:24} | {:33} | {:8} |\n", g->_count.load(),
g->_blocked.load(), g->_addr, g->_hash, g->_match_type);
fwrite(w.data(), w.size(), 1, f);
}
diff --git
a/tests/gold_tests/origin_connection/per_server_metric_enabled.replay.yaml
b/tests/gold_tests/origin_connection/per_server_metric_enabled.replay.yaml
new file mode 100644
index 0000000000..9889af26ed
--- /dev/null
+++ b/tests/gold_tests/origin_connection/per_server_metric_enabled.replay.yaml
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Test Notes:
+# A single transaction which leaves one keep-alive origin connection pooled
+# behind it. The test driving this replay file verifies that the outbound
+# connection tracker keeps an accurate connection count while
+# proxy.config.http.per_server.connection.metric_enabled is set, so that the
+# pooled connection is reaped once the keep alive timeout expires.
+
+meta:
+ version: "1.0"
+
+sessions:
+
+- transactions:
+
+ - client-request:
+ method: GET
+ url: /some/path/first
+ version: '1.1'
+ headers:
+ fields:
+ - [ Host, www.example.com ]
+ - [ Content-Length, 0 ]
+ - [ uuid, first-request ]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [ Content-Length, 16 ]
+ - [ X-Response, first-response ]
+
+ proxy-response:
+ status: 200
+ headers:
+ fields:
+ - [ X-Response, {value: 'first-response', as: equal } ]
diff --git
a/tests/gold_tests/origin_connection/per_server_metric_enabled.test.py
b/tests/gold_tests/origin_connection/per_server_metric_enabled.test.py
new file mode 100644
index 0000000000..510265c3df
--- /dev/null
+++ b/tests/gold_tests/origin_connection/per_server_metric_enabled.test.py
@@ -0,0 +1,113 @@
+'''
+Verify per_server connection tracking stays accurate when
+proxy.config.http.per_server.connection.metric_enabled is set.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+Test.Summary = __doc__
+
+Test.SkipIf(Condition.CurlUsingUnixDomainSocket())
+
+
+class PerServerMetricEnabledTest:
+ """Verify that enabling the per_server metrics does not break the group
connection count.
+
+ Enabling proxy.config.http.per_server.connection.metric_enabled used to
+ make the metric the authoritative connection count, leaving the group's
+ internal counter at zero. That made every pooled origin session look like
+ it was at or below proxy.config.http.per_server.connection.min, so
+ keep-alive origin connections were never reaped on inactivity timeout.
+ """
+
+ _replay_file: str = 'per_server_metric_enabled.replay.yaml'
+ _keep_alive_timeout: int = 2
+
+ def __init__(self) -> None:
+ """Configure the test processes in preparation for the TestRun."""
+ self._configure_server()
+ self._configure_trafficserver()
+
+ def _configure_server(self) -> None:
+ """Configure the origin server to be used in the test."""
+ self._server = Test.MakeVerifierServerProcess('metric_enabled_server',
self._replay_file)
+
+ def _configure_trafficserver(self) -> None:
+ """Configure Traffic Server to be used in the test."""
+ self._ts = Test.MakeATSProcess("ts_metric_enabled")
+ self._ts.Disk.remap_config.AddLine(f'map /
http://127.0.0.1:{self._server.Variables.http_port}')
+ self._ts.Disk.records_config.update(
+ {
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http_ss|conn_track',
+ 'proxy.config.http.per_server.connection.metric_enabled': 1,
+ 'proxy.config.http.per_server.connection.metric_prefix': 'bar',
+ 'proxy.config.http.per_server.connection.match': 'port',
+ # No minimum number of keep alive origin connections: the
pooled
+ # connection should be closed once it times out.
+ 'proxy.config.http.per_server.connection.min': 0,
+ 'proxy.config.http.keep_alive_no_activity_timeout_out':
self._keep_alive_timeout,
+ 'proxy.config.http.server_session_sharing.pool': 'global',
+ })
+ # The connection count should never be decremented below zero.
+ self._ts.Disk.diags_log.Content += Testers.ExcludesExpression(
+ 'Number of tracked connections should be greater than or equal to
zero',
+ 'Verify the group connection count is not double decremented.')
+
+ def _test_connection_is_reaped(self) -> None:
+ """Verify the idle origin connection is closed once it times out."""
+ tr = Test.AddTestRun("Verify the idle keep-alive origin connection is
reaped")
+ tr.Processes.Default.Command = (
+ f'sleep {self._keep_alive_timeout * 3}; '
+ 'traffic_ctl metric get
proxy.process.http.current_server_connections; '
+ 'traffic_ctl metric match per_server')
+ tr.Processes.Default.ReturnCode = 0
+ tr.Processes.Default.Env = self._ts.Env
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+ 'proxy.process.http.current_server_connections 0',
+ 'The idle origin connection should have been closed by the
keep-alive timeout.')
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+
f'per_server.current_connection.bar.127.0.0.1:{self._server.Variables.http_port}
0',
+ 'The per_server connection gauge should have been decremented back
to zero.')
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+
f'per_server.total_connection.bar.127.0.0.1:{self._server.Variables.http_port}
1',
+ 'A single origin connection should have been tracked.')
+
+ def _test_tracker_info(self) -> None:
+ """Verify the JSONRPC connection tracker report agrees with the
metrics."""
+ tr = Test.AddTestRun("Verify the connection tracker report")
+ tr.Processes.Default.Command = "traffic_ctl rpc invoke
get_connection_tracker_info -p 'table: outbound' -f json"
+ tr.Processes.Default.ReturnCode = 0
+ tr.Processes.Default.Env = self._ts.Env
+ # Once the connection is released the group count drops to zero and the
+ # group is removed from the table, so either the table is empty or the
+ # remaining group reports no current connections.
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+ r'"(count|current)":\s*"?0"?', 'The tracker should report no
current outbound connections.')
+
+ def run(self) -> None:
+ """Configure the TestRuns."""
+ tr = Test.AddTestRun('Perform a transaction that leaves a pooled
origin connection')
+ tr.Processes.Default.StartBefore(self._server)
+ tr.Processes.Default.StartBefore(self._ts)
+
+ tr.AddVerifierClientProcess('metric_enabled_client',
self._replay_file, http_ports=[self._ts.Variables.port])
+
+ self._test_connection_is_reaped()
+ self._test_tracker_info()
+
+
+PerServerMetricEnabledTest().run()
