Add iperf output types and use it instead of go-iperf #2089
Conversation
AbdelrahmanElawady
requested review from
muhamadazmy,
delandtj,
MarioBassem,
ashraffouda,
rawdaGastan and
xmonader
as code owners
| log.Error().Err(err).Msgf("failed to start iperf client with ip '%s'", clientIP) | ||
| output, err := exec.CommandContext(ctx, "iperf", opts...).CombinedOutput() | ||
| exitErr := &exec.ExitError{} | ||
| if err != nil && !errors.As(err, &exitErr) { |
There was a problem hiding this comment.
Actually I'm not sure I follow, we ignore errors other than exitErr? while that actually means we failed to execute the binary?
There was a problem hiding this comment.
we ignore exit errors because iperf exits with a non-zero code in case it failed to run and prints the error reason in error field in JSON output. For example:
{
"start": {
"connected": [],
"version": "iperf 3.14",
"system_info": "Linux zero-os 5.10.55-Zero-OS #2 SMP PREEMPT Wed Aug 9 13:31:25 UTC 2023 x86_64"
},
"intervals": [],
"end": {},
"error": "unable to connect to server - server may have stopped running or use a different port, firewall issue, etc.: Network unreachable"
}
pkg/perf/iperf_types.go
Outdated
| @@ -0,0 +1,134 @@ | |||
| package perf | |||
There was a problem hiding this comment.
What is the point of having these here while they exist in go-iperf code?
There was a problem hiding this comment.
because go-iperf creates a temp directory in init so by importing go-iperf it would cause the same problem we are avoiding each time the task runs
There was a problem hiding this comment.
In general this looks good, but my only comment is that the perf package is getting very cluttered. Can u move the iperf task into a sub package
perf
/iperf/
since the registration of the tasks happens in noded/main this should not cause any cyclic dependency
NOTE: please also move other tasks to their own separate sub packages to perf.
so we will also havecpuand eventually public-ips
* Add iperf output types and use it instead of go-iperf * Organize perf tasks * Remove go-iperf tmp directories
Description
go-iperfdidn't have proper cleanup for temp directories created. So, it was easier to just exec the command and parse it directly instead of using it.Changes