tcp_info version handling (#5) plus a batch of decode, cli, and platform fixes#8
Open
svczero wants to merge 19 commits into
Open
tcp_info version handling (#5) plus a batch of decode, cli, and platform fixes#8svczero wants to merge 19 commits into
svczero wants to merge 19 commits into
Conversation
the table had v4.9 smaller than v4.6 (160), which an append-only struct can never be. 148 is the offset of notsent_bytes, a v4.6 field; v4.9 adds tcpi_delivery_rate ending at 168. this size is the optlen passed to getsockopt, so 148 meant the kernel wrote 148 bytes and min_rtt, data_segs_in/out and delivery_rate came back a silent zero with Valid:true.
total_rto, total_rto_recoveries and total_rto_time landed in v6.7 but Unpack marked them valid from _6_2 on, while _6_7 was set and never read. on a 6.2 to 6.6 kernel those three fields sit past the struct the kernel actually wrote, so they came back Valid:true, Value:0. move them under _6_7 and give the flag a reader.
the value is the kernel's tcpi_total_rto_time, which is milliseconds (see the struct comment), but the prometheus help text said nanoseconds, so a dashboard trusting the help would be off by six orders of magnitude.
TxWindowScale, RxWindowScale and the app-limited/fastopen flags are C bitfields, whose member order depends on endianness: the first member sits in the low bits on little-endian and the high bits on big-endian. Unpack assumed little-endian, so on s390x, ppc64 and mips the window scales came back swapped and the flags shifted. read them per byte order.
ToInfo() surfaced RTTMin as the cross-platform Info.RTT, so windows reported the historical minimum where linux and darwin report the most-recent RTT. a link with 50ms current and 5ms minimum showed 5ms. use s.RTT to match the field's meaning and the other platforms.
darwin ToInfo put tcpi_snd_wnd, the send window in bytes, into TxWindowSegs, which is meant to be a congestion window in segments. xnu has no per-segment cwnd, so leave the field 0 rather than reporting a byte count as segments. TxWindowBytes already carries the byte value.
TxOptions is set to an empty slice up front while RxOptions is only appended to, so with no tcp options one marshals to [] and the other to null. give RxOptions the same empty-slice start.
ConnectDone fires once per address the dialer tries, with that address's error. calling log.Fatalf there kills the process on the first failure, before the dialer reaches the remaining addresses, so a dual-stack host on a single-stack network (or one whose first record is filtered) dies with "unable to connect" for a host the stdlib would have reached. only record the connect time and print on success; client.Do() below already reports a genuine total failure.
the final diagnostics read a package-global connConn that setConniver only ever assigned and never cleared, and read it right after CloseIdleConnections without waiting for the async Closed callback. a -L redirect chain printed the previous hop's remote/RTT/byte counts, and a fast single request could print before the callback populated anything at all (DisableKeepAlives keeps the conn out of the pool, so CloseIdleConnections cannot force the close). reset the recorded conn at the start of each visit() and arm a ready channel the callback closes, then wait on that channel before reading, so a hop only ever prints its own connection.
the example closed the body without reading it, so the connection could not be pooled and the CloseIdleConnections call that follows had nothing to close, which is the opposite of what its comment says. copy the body to io.Discard first.
GetVersion() is compatibility-shimmed to cap at 6.2.9200 for any process without a supportedOS manifest, which these binaries do not ship, so GetKernelVersion reported windows 8 on windows 10 and 11 with a nil error. RtlGetVersion is not capped that way and returns the true major, minor and build.
the kvi/major/minor/build comments described a 6.1.7601.17592 to 6/1/7601/17592 mapping the code never produces: kvi is the BuildLabEx string, and the other three come from the GetVersion DWORD (major is the low byte, minor the second byte, build the high word). there is no revision field. describe what it does.
kernel_unix_test.go was tagged !windows && !aix, which includes solaris, illumos, plan9 and js/wasm, where the package defines no VersionInfo, so go vet and go test failed to compile there even though go build was fine. give the test the same positive constraint the code uses.
GetOperatingSystem read utsname.Machine, which is the architecture (arm64, x86_64), so it never returned "Darwin" or "FreeBSD" and disagreed with its own doc comment and the linux and windows siblings. Sysname is the OS name field.
the check keyed on the cgroup v1 layout: a v2 container's /proc/1/cgroup is the single line "0::/", which ends in ":/" and read as host, so IsContainerized returned false on docker 20.10+, ubuntu 22.04+, fedora, rhel 9 and the rest of the v2 world. add the v2 signals (/.dockerenv, /run/.containerenv, a docker / lxc / kubepods / containerd / machine path) and keep the v1 logic.
the scan loop ignored scanner.Err(), so an os-release with a line past bufio's 64KiB token limit, or a mid-read i/o error, stopped the scan early and the function returned an empty value with a nil error, which GetOperatingSystem then reported as the hardcoded "Linux". return the error instead.
Read and Write only called finishIO on the normal return path, so if the underlying conn's Read or Write panicked and was recovered upstream, inFlight never came back down and Close blocked forever in its drain loop. defer finishIO right after beginIO in both, so the counter is always released.
the kernel emits tcpi_last_data_sent, tcpi_last_data_recv and tcpi_last_ack_recv via jiffies_to_msecs(), so they are milliseconds, but Unpack scaled them with the shared microsecond multiplier like rto/ato/rtt. that made LastTxAt, LastRxAt and LastRxAckAt come out 1000x too small on every linux host. give those three time.Millisecond and leave the genuinely-microsecond fields alone. last_ack_sent is never populated by the kernel so it stays 0 either way.
with the shadow gone, adaptToKernelVersion takes the version as an argument, so the tests pin one instead of reading the host, and the shipped bitfield tests move to 6.7.0 so their all-fields-present expectations match a version that now takes effect. three new tests cover what a single host data point never could: the size table must be non-decreasing and end at sizeof(RawTCPInfo) (catches the v4.9=148 shrink), each field is Valid iff its introduction version <= the pin (catches the 6.6 vs 6.7 RTO mix-up), and a pinned version drives the output no matter what host runs the test. each was checked to fail against its bug first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased on
mainafter 662f6a7: since you already fixed the #5 shadow your way, I dropped my version of it and reworked the couple of tests that leaned on it to yourlinuxKernelVersion/ no-arg API. What is left is the batch of other bugs found while digging into #5, none of which the shadow fix touches. One commit per fix so any can be dropped on its own. Builds/vets/tests green on yourmain; I ran the riskier ones against a live kernel (notes at the end).the two bugs the shadow was hiding
Your #5 fix makes version injection actually take effect, which is what makes these testable. Both are still live on
main:notsent_bytes's offset. As thegetsockoptoptlen, 148 means the kernel writes 148 bytes andmin_rtt/data_segs_in/data_segs_out/delivery_ratecome backValid:true, Value:0._6_7, not_6_2:total_rto*came backValid:true, Value:0on 6.2-6.6 kernels (_6_7was set and never read).sizeof(RawTCPInfo), and checks each field isValidiff its introduction version <= the pin. Each new test was confirmed to fail against its bug first. It bumps the test pin from 6.2 to 6.7, which the RTO fix requires.tcpinfo decode
last_data_*is kernel milliseconds (jiffies_to_msecs), not microseconds, soLastRxAt/LastTxAt/LastRxAckAtwere 1000x too smalltotal_rto_timeprom_help said nanoseconds; the value is millisecondsRTTMin(linux and darwin report current)snd_wnd(bytes) was going intoTxWindowSegs(a segment count); leave it 0RxOptionslikeTxOptionsso it marshals[]notnullhttpstat / get
log.Fatalfon one failed address inConnectDone: it aborts before the dialer tries the rest, so a dual-stack host on a single-stack network dies for a host that is actually reachable-Lredirects stop printing the previous hop's numbers and fast single requests stop missing themCloseso the conn can be pooledpkg/kernel, pkg/os
RtlGetVersioninstead of the manifest-shimmedGetVersion(caps at 6.2.9200, so unmanifested binaries report Win8 on Win10/11)VersionInfodoc comments to match the codeGetOperatingSystemreturned the CPU arch (utsname.Machine), not the OS name (Sysname)0::/read as host)scanner.ErringetValueFromOsRelease(an oversized os-release line made it report "Linux" with a nil error)wrap.go
finishIOso a panicking wrapped conn doesn't leakinFlightand deadlockCloseVerified live on Ubuntu 26.04 / kernel 7.0: the full suite plus the live-socket decode, the
ConnectDonedual-stack abort (dies before, connects after), and thelast_data_*fix (LastRxAt2ms -> 2s after a 2s idle).