Harden timing-sensitive AuTests - #13508
Conversation
7c09a1b to
291d52e
Compare
|
[approve ci] |
cmcfarlen
left a comment
There was a problem hiding this comment.
Three unrelated flake fixes; all sound. One thing I would want answered before merge, plus polish.
1. ports.py — count bound UDP ports
Correct, and it fixes a genuine latent crash beyond the flakiness: the old code called conn.laddr.port on every CONN_LISTEN connection from net_connections(kind='all'), which includes AF_UNIX sockets whose laddr is a path string — presumably the "setup exception" in your description. The new family filter closes that, and treating any UDP socket with a local address as unavailable is the right call given UDP has no listen state. socket is already imported at ports.py:22, so no missing import.
- The predicate is duplicated verbatim in the main path and the macOS
AccessDeniedfallback. Extractdef _is_bound(conn) -> bool:— one definition, and both call sites stay readable inside 132 columns. - Naming drift. The function is now
_get_bound_ports(), but callers still assign tolistening_ports, andPortOpen()'s parameter, docstring ("A set of ports that are currently listening") and debug message ("because it is in the listening sockets set") all still say listening. The semantics genuinely changed — the set now contains ports that nothing is listening on — so the stale name is actively misleading. Worth renaming tobound_portswhile you are in here. - Worth a sentence in the description: this monotonically shrinks the usable port pool, since a port that happened to be bound by an unrelated process during
_setup_port_queue()is excluded for the whole run. That is the safe direction and the pool is large, but it is a real behavior change on busy CI hosts, not purely a bug fix. - Confirm
-t formatwas run — the multi-line boolean continuation style is the kind of thing yapf likes to rewrite.
2. grpc_server.py — count RPCs at completion
Moving from "increment at handler entry" to context.add_done_callback() is right: the counter drives done_event, which drives server.stop(5), so counting at entry lets shutdown begin while the response is still in flight — the 502s. The grpc.aio done-callback signature does receive the context, so _record_message(self, _context) matches.
Two notes:
- Done callbacks fire on cancellation and error, not just success. Previously only handler entry counted; now a cancelled or failed RPC also increments
global_message_counterand can satisfynum_expected_messagesearly. Probably harmless for these tests, but if you want the tightest semantics, checkcontext.code()in the callback and only countOK. _record_messagecallsasyncio.get_running_loop(). Inside a coroutine that was guaranteed; invoked from gRPC's callback machinery the guarantee is weaker.grpc.aiodoes dispatch these on the loop, so this is fine as written — flagging only so that if it ever raisesRuntimeErrorin CI,call_soon_threadsafeon a captured loop is the fix.
3. serial_tests.txt — one concrete gap
Serialization does what you claim: autest-parallel.py.in:1053 runs serial tests only after the parallel phase completes, so the "after tls_conn_timeout" ordering is genuinely guaranteed. The "14 ATS instances" in the comment is accurate (num_upstream = 6 + num_peer = 8).
But zzz_strategies_peer — peer1 — is not in the list, and it is the same test. Same 6 + 8 = 14 ATS instances, and the identical ordering comment verbatim:
tests/gold_tests/next_hop/zzz_strategies_peer/zzz_strategies_peer.test.py:23:
# The tls_conn_timeout test will fail if it runs before this test in CI. Therefore, this test has a zzz
tests/gold_tests/next_hop/zzz_strategies_peer2/zzz_strategies_peer2.test.py:23:
# The tls_conn_timeout test will fail if it runs before this test in CI. Therefore, this test has a zzz
If the zzz prefix no longer buys ordering under the parallel runner — the premise of this change — then peer1 is just as exposed as peer2 and should be serialized too. If it is deliberately excluded, the PR should say why.
Separately: the comment in both test files reads backwards relative to the zzz prefix's intent. "tls_conn_timeout will fail if it runs before this test" implies this test must run first, while zzz forces it last. Your new serial_tests.txt comment ("Must run after tls_conn_timeout") states the actual constraint correctly. Since you are touching this area, fixing the two stale in-test comments would retire a genuinely confusing pair — and with serialization in place, the zzz prefix and its comment are arguably obsolete for peer2 entirely.
Several AuTests fail nondeterministically in parallel CI. The gRPC server can stop before its final response reaches the client, and the port allocator both ignores bound UDP ports and assumes every datagram address has a numeric port. The heavyweight strategy tests also rely on filename ordering that the parallel runner does not preserve. These failures appear as 502s, bind errors, setup exceptions, or port collisions. This patch addresses the races by counting completed RPCs, reserving bound IPv4 and IPv6 UDP ports while ignoring Unix sockets, and running both ordering-sensitive strategy tests after the parallel workers. Ports bound when the queue is initialized stay excluded for the full run, safely reducing the pool available on busy hosts.
291d52e to
949dfb6
Compare
|
Thanks—addressed the duplicated predicate and naming drift, serialized both peer strategy tests, corrected their ordering comments, and documented the bound-port snapshot behavior. I kept counting all terminal RPC callbacks because client errors already fail the test and counting them avoids an additional 60-second server timeout. The callback runs on the gRPC event loop, so |
Several AuTests fail nondeterministically in parallel CI. The gRPC server can stop before its final response reaches the client, and the port allocator both ignores bound UDP ports and assumes every datagram address has a numeric port. The heavyweight strategy tests also rely on filename ordering that the parallel runner does not preserve. These failures appear as 502s, bind errors, setup exceptions, or port collisions. This patch addresses the races by counting completed RPCs, reserving bound IPv4 and IPv6 UDP ports while ignoring Unix sockets, and running both ordering-sensitive strategy tests after the parallel workers. Ports bound when the queue is initialized stay excluded for the full run, safely reducing the pool available on busy hosts. (cherry picked from commit 816420e)
|
Cherry-picked to the 10.2.x branch as de92d60 for the 10.2.0 release. |
Several AuTests fail nondeterministically in parallel CI. The gRPC
server can stop before its final response reaches the client, and the
port allocator both ignores bound UDP ports and assumes every datagram
address has a numeric port. The heavyweight strategy tests also rely
on filename ordering that the parallel runner does not preserve. These
failures appear as 502s, bind errors, setup exceptions, or port
collisions.
This patch addresses the races by counting completed RPCs, reserving
bound IPv4 and IPv6 UDP ports while ignoring Unix sockets, and running
both ordering-sensitive strategy tests after the parallel workers.
Ports bound when the queue is initialized stay excluded for the full
run, safely reducing the pool available on busy hosts.