Copilot commented on code in PR #13385:
URL: https://github.com/apache/trafficserver/pull/13385#discussion_r3596814338
##########
tests/gold_tests/pluginTest/packet_mark/packet_mark.test.py:
##########
@@ -44,60 +46,131 @@ def _can_set_so_mark() -> bool:
Test.SkipUnless(
Condition.IsPlatform("linux"),
- Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with
CAP_NET_ADMIN or CAP_NET_RAW", True),
+ # pass_value defaults to True: run only when the probe reports SO_MARK is
settable.
+ Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with
CAP_NET_ADMIN or CAP_NET_RAW"),
)
+# SOCK_OPT_PACKET_MARK (0x10) | SOCK_OPT_NO_DELAY (0x1). The mark is only
pushed
+# to the socket when the PACKET_MARK bit is set in the sock option flag.
+SOCK_OPT_FLAG_PACKET_MARK = 0x11
-class ClientPacketMarkTest:
- """Drive TSHttpTxnClientPacketMarkSet through a test plugin and assert on
the
- firewall mark read back off the client socket.
- The starting mark is seeded per process via
- proxy.config.net.sock_packet_mark_in, applied at accept time.
+class PacketMarkTest:
+ """Drive a TSHttpTxn*PacketMarkSet API through a test plugin and assert on
the
+ firewall mark read back off the relevant socket.
+
+ This base holds the shared skeleton -- process setup, the common records,
the
+ curl-and-assert case runner. Each subclass supplies its plugin and echo
+ header and extends _configure() (via super()) with the side-specific mark
and
+ flag records.
"""
# Value the plugin sets; the mark is expected to become exactly this.
SET_MARK = 0x0000000A
+ # Seeded starting mark, distinct from SET_MARK so a no-op would be visible.
+ SEED_MARK = 0x0000FF00
+
+ # Bumped per instance so each side gets uniquely-numbered processes.
+ _counter = 0
def __init__(self):
+ self._num = PacketMarkTest._counter
+ PacketMarkTest._counter += 1
self._server = self._make_server()
- self._ts = self._make_ats("ts", seed_mark=0x0000FF00)
+ self._ts = self._make_ats()
+ self._configure(self._ts)
+ self._started = False
- def _make_server(self) -> 'Process':
- server = Test.MakeOriginServer("server")
+ def _make_server(self):
+ server = Test.MakeOriginServer(f"server{self._num}")
request_header = {"headers": "GET / HTTP/1.1\r\nHost:
example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection:
close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
- server.addResponse("sessionlog.json", request_header, response_header)
+ for _ in range(2):
+ server.addResponse("sessionlog.json", request_header,
response_header)
return server
- def _make_ats(self, name: str, seed_mark: int) -> 'Process':
- ts = Test.MakeATSProcess(name, enable_cache=False)
+ def _make_ats(self):
+ return Test.MakeATSProcess(f"ts{self._num}", enable_cache=False)
+
+ def _configure(self, ts):
+ # Records and remap shared by both sides. Subclasses override to add
the
+ # side-specific mark/flag records and load their plugin, calling
super()
+ # for these.
ts.Disk.records_config.update(
{
- 'proxy.config.net.sock_packet_mark_in': seed_mark,
- 'proxy.config.net.sock_option_flag_in': 0x11,
'proxy.config.diags.debug.enabled': 1,
- 'proxy.config.diags.debug.tags': 'http|client_packet_mark',
'proxy.config.url_remap.remap_required': 0,
# Keep ATS running as the invoking user inside sudo (no
privilege drop).
'proxy.config.admin.user_id': '#-1',
})
ts.Disk.remap_config.AddLine(f"map /
http://127.0.0.1:{self._server.Variables.Port}";)
- Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir,
'client_packet_mark.so'), ts)
- return ts
- def run(self):
+ def _add_case(self, echo_header: str, description: str, set_header: str):
# The mark is set to the supplied value, regardless of the seeded
- # starting mark.
- tr = Test.AddTestRun("TSHttpTxnClientPacketMarkSet sets the mark")
- tr.Processes.Default.StartBefore(self._server)
- tr.Processes.Default.StartBefore(self._ts)
+ # starting mark. The set is driven by whichever request header the
plugin
+ # keys on; the observed mark is echoed into echo_header. The origin
server
+ # and ATS are started before the first case, independent of the order
in
+ # which cases are added.
+ tr = Test.AddTestRun(description)
+ if not self._started:
+ tr.Processes.Default.StartBefore(self._server)
+ tr.Processes.Default.StartBefore(self._ts)
+ self._started = True
Review Comment:
Only the first TestRun created by _add_case() declares dependencies on the
origin server / ATS via StartBefore(). Subsequent cases (e.g., the second
ServerPacketMarkTest case) may run without ATS/server being started or without
correct ordering, making the test flaky or failing depending on AuTest
scheduling/lifecycle. Add StartBefore() for every case; if the goal is to avoid
restarting processes between cases, keep them running after the first case with
StillRunningAfter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
