Skip to content
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

misc: Align tc_iterate behavior #312

Closed
wants to merge 2 commits into from

Conversation

jkoeppeler
Copy link

@jkoeppeler jkoeppeler commented Oct 28, 2024

The execution of tc_iterate.sh can cause different behaviors depending on whether the tc_iterate binary is installed. The tc_iterate prog collects tc statistics "count"-times where as the bash loop runs for "length" seconds. This leads to different measuring durations.

This patch replaces the "count" parameter with the "length" parameter for tc_iterate.c CLI. tc_iterate.c then calculates the "count" based on the length to determine the duration.

The execution of tc_iterate.sh can cause different behaviors depending
on whether the tc_iterate binary is installed. The tc_iterate prog runs
collects tc statistics "count"-times where as the bash loop runs for
"length" seconds. This leads to different measuring durations.

Signed-off-by: Jonas Köppeler <[email protected]>
@tohojo
Copy link
Owner

tohojo commented Oct 29, 2024

Hmm, so the problem with this is that with this change, flent will no longer work with an old version of tc_iterate, which can be a problem especially when running it on remote hosts. I think this may actually be why tc_iterate itself wasn't updated back when the script was.

Rather than dealing with doing this in a backwards-compatible way, I wonder if it isn't better to just deprecate the use of the tc_iterate binary entirely. I'm not sure the accuracy is actually that much worse for the shell script version. Did you observe a case where this was significant?

@jkoeppeler
Copy link
Author

I compared the performance between the script and the binary by running both for 10 seconds with an interval of 0.01. This should capture 1000 data points. The bash script only captured 470 and the binary could record all 1000. Looking at the timestamps the binary is quite precisely able to trigger every 0.01 seconds where as the bash script only gets an accuracy of around 0.02.
The tc_iterate binary is also able to capture everything up to an 0.001s interval.

@@ -23,7 +21,7 @@ buffer=""


command_string=$(cat <<EOF
which tc_iterate >/dev/null && exec tc_iterate $buffer -i $interface -c $count -I $interval -C $command;
which tc_iterate >/dev/null && exec tc_iterate $buffer -i $interface -l $length -I $interval -C $command;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so one thing that comes to mind as a way of keeping backwards compatibility, is that instead of unconditionally exec'ing when the binary exists, we do a regular call to the binary, and exit the script on success. That way, if it's an old binary without the -l option, we will fall back to the script. The only potential problem with this is that if the binary runs, but still exits non-zero, we'll get duplicate results. This could happen if it explicitly killed; however, it seems that that results in a different exit code, so maybe if we only react to the return value in usage() (255), that would work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option could be to check the output of the binary when called with --help (or similar) and basically grep if -l or -c is in the output. This way we could distinguish between the binaries.
Or we just keep -c and calculate the count in the script instead of in the C-code. This would be probably the minimal fix. But either way, depending on your preference I am happy to implement it :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that's a good point actually: why is this change needed at all? You're basically just moving the same calculation from python to C?

Copy link
Author

@jkoeppeler jkoeppeler Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought doing it in C makes it easier if one (like me) wants to directly execute the C-binary. And then it is just more comfortable to pass a length parameter instead of deriving the count from the duration. But if preferred, then I would just add the calculation to the python code (or in the bash script?)
And as far as I see it: The python code is just passing the length as the count parameter which results in different behavior because the tc_iterate binary will capture 10 data points instead of capturing for 10 seconds.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're executing it manually? Why? :)

And, well, the line this patch removes from the python file already contains the exact same calculation? :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, :D you are correct, sorry I misread the code. Will close this PR then.

@jkoeppeler jkoeppeler closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants