-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[windows][network path] Add windows support for traceroute #30201
Conversation
ff99cc5
to
a8ef587
Compare
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=50035851 --os-family=ubuntu Note: This applies to commit 0276eca |
if hop.IsDest { | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section seems to be same/very similar between windows and linux. Can we try to factor it as utils to avoid logic duplication?
@@ -10,6 +10,7 @@ package traceroute | |||
import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that overall unit test coverage could be better, many code paths seems to be not tested. It might require some code refactoring to make some functions testable though.
DO NOT MERGE Initial support for traceroute on windows. As presently implemented may (probably does) break linux build. Implements the traceroute functionality on windows by making direct syscalls to the single raw socket. Uses existing code for packet formatting. Uses some existing code for packet processing; although it cuts out lots of layers.
but is sufficiently large we were just returning an error rather than moving on.
555f982
to
b82972d
Compare
if n < 20 { // min size of ipv4 header | ||
continue | ||
} | ||
header, err := ipv4.ParseHeader(buf[:n]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting ebpfless is using gopacket's layer parsing to do something similar. It's not quite the same though, since we parse the TCP layer no matter what's in the IP layer (no early exit). If it's suitable, it could save some headache. It supports ICMP layers
// the receive timeout is set to 100ms in the constructor, to match the | ||
// linux side. This is a workaround for the lack of a deadline for sockets. | ||
//err := conn.SetReadDeadline(now.Add(time.Millisecond * 100)) | ||
n, _, err := windows.Recvfrom(w.s, buf, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make it easier to test if the socket recvfrom loop could be in a separate function from the packet parsing, since each is rather branch heavy
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 4ee1010 Optimization Goals: ✅ No significant changes detected
|
perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
---|---|---|---|---|---|---|
➖ | basic_py_check | % cpu utilization | +4.04 | [+0.11, +7.98] | 1 | Logs |
➖ | pycheck_lots_of_tags | % cpu utilization | +1.16 | [-2.32, +4.63] | 1 | Logs |
➖ | otel_to_otel_logs | ingress throughput | +0.39 | [-0.27, +1.05] | 1 | Logs |
➖ | tcp_syslog_to_blackhole | ingress throughput | +0.25 | [+0.19, +0.31] | 1 | Logs |
➖ | file_to_blackhole_1000ms_latency | egress throughput | +0.16 | [-0.62, +0.94] | 1 | Logs |
➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +0.05 | [-0.68, +0.78] | 1 | Logs |
➖ | file_to_blackhole_0ms_latency | egress throughput | +0.04 | [-0.86, +0.94] | 1 | Logs |
➖ | file_to_blackhole_300ms_latency | egress throughput | +0.03 | [-0.60, +0.66] | 1 | Logs |
➖ | file_to_blackhole_100ms_latency | egress throughput | +0.02 | [-0.70, +0.74] | 1 | Logs |
➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.10, +0.10] | 1 | Logs |
➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.01, +0.01] | 1 | Logs |
➖ | file_to_blackhole_500ms_latency | egress throughput | -0.09 | [-0.85, +0.68] | 1 | Logs |
➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | -0.11 | [-0.56, +0.35] | 1 | Logs |
➖ | quality_gate_idle | memory utilization | -0.28 | [-0.35, -0.20] | 1 | Logs bounds checks dashboard |
➖ | quality_gate_idle_all_features | memory utilization | -1.07 | [-1.18, -0.95] | 1 | Logs bounds checks dashboard |
➖ | file_tree | memory utilization | -2.93 | [-3.06, -2.80] | 1 | Logs |
Bounds Checks: ❌ Failed
perf | experiment | bounds_check_name | replicates_passed | links |
---|---|---|---|---|
❌ | file_to_blackhole_0ms_latency | lost_bytes | 9/10 | |
❌ | file_to_blackhole_300ms_latency | lost_bytes | 9/10 | |
❌ | quality_gate_idle_all_features | memory_usage | 9/10 | bounds checks dashboard |
✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
❌ Failed. Some Quality Gates were violated.
- quality_gate_idle_all_features, bounds check memory_usage: 9/10 replicas passed. Failed 1 which is > 0. Gate FAILED.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
|
/merge |
Devflow running:
|
What does this PR do?
Initial support for traceroute on windows.
Implements the traceroute functionality on windows by making direct syscalls to the single raw socket. Uses existing code for packet formatting. Uses some existing code for packet processing; although it cuts out lots of layers.
Motivation
Network Path customers have expressed interest in using this product on Windows. Unfortunately, the
net
library methods we use for socket operations on unix systems are not implemented for Windows in Go.Describe how to test/QA your changes
Possible Drawbacks / Trade-offs
Additional Notes