Closed w-miller closed 1 year ago
Patch coverage: 75.00%
and project coverage change: +0.11%
:tada:
Comparison is base (
f3f7422
) 40.11% compared to head (1d6d41c
) 40.22%. Report is 8 commits behind head on master.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hello @w-miller thank you for the PR.
But I would like to understand: what problem are you trying to solve with this PR? Is there any performance or stability issue with the current flow? Why do you think reusing the same tc
container is a better option, than creating a new instance for every command? Did you measure the performance impact of this change?
@alexei-led thanks for your response. Our pumba workflow involves using a separate container to run the tc commands (with --tc-image=gaiadocker/iproute2
), and setting specific target IP addresses with the --target
filter. We then typically use a long-ish duration for the pumba netem invocation, and terminate the container when we want to undo the rules. The motivation for this pull request was to reduce the time between invoking pumba, and the netem tc rules being successfully applied to the target container.
Explanation of the specific change in this patch: before the patch, pumba was attempting to do the following for every tc
command it executes:
--tc-image
. Assuming that you already have the image present, this adds the overheads of making a network call to your container registry, and validating that the local image is still up to date.tc
command, execute the command, then stop and remove the container again.It's probably worth mentioning that if you don't specify any IP or port filters, only a single tc
command will be run, meaning this patch will behave identically to master. However, as soon as you add a single IP filter (for example), 5 tc
commands are run, and this number further increases if you have additional IP or port filters.
I've run a benchmark locally with and without this patch to calculate the time between starting pumba and the netem tc
rules being successfully applied. I compared these results between the code with and without the patch in this PR. N.B. in order to measure this timespan, I also needed to cherry-pick the patch in https://github.com/alexei-led/pumba/pull/250 to both of the pumba builds I tested, since the time at which the log message netem command started
appears is the time at which the tc
rules have all been applied, and there was no existing message logged at this point. I created a dummy container to be used as the target for the tc
rules - a trivial container from the ubuntu
image with the entrypoint sleep infinity
. This container had name funny_napier
. I used the following command, replacing pumba_image
with a docker image built from each of the two versions of the code. I ran each variant 10 times
docker run -t -v /var/run/docker.sock:/var/run/docker.sock -it <pumba_image> --json --log-level=debug netem --duration=60s --tc-image=gaiadocker/iproute2 --target=192.168.0.22 loss --percent 10 funny_napier | grep --line-buffered 'netem command started' | head -n1 | ts -s '%.s'
The full results are below, but the outcome was that the average time between pumba invocation and netem command started
being logged was:
This result mirrors the speedup of around 3x that we saw in our testing workflows that use pumba after applying this patch.
Optimise the flow when using a separate container to run
tc
commands, by only pulling the container image once, creating a single container for the commands, andexec
ing each command in that container.