[Flent-users] [tohojo/flent] Draft: Runners: stop stats collection iterators after test-time is completed (#237)

[Flent-users] [tohojo/flent] Draft: Runners: stop stats collection iterators after test-time is completed (#237)

[Shashank D]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Closes #236 

Signed-off-by: Shashank D <shashankindiamanoj@gmail.com>
You can view, comment on, or merge this pull request online at:

  https://github.com/tohojo/flent/pull/237

-- Commit Summary --

  * <a href="https://github.com/tohojo/flent/pull/237/commits/83def6ef91865dc6dc51bb74f8803518a095d9ea">RunnerScripts: Stop collecting stats based on test endtime</a>

-- File Changes --

    M flent/runners.py (7)
    M flent/scripts/ss_iterate.sh (9)
    M flent/scripts/tc_iterate.sh (6)

-- Patch Links --

https://github.com/tohojo/flent/pull/237.patch
https://github.com/tohojo/flent/pull/237.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tohojo/flent/pull/237

[-- Attachment #2: Type: text/html, Size: 3251 bytes --]

[Flent-users] Re: [tohojo/flent] Runners: stop stats collection iterators after test-time is completed (#237)

[Toke Høiland-Jørgensen]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

@tohojo requested changes on this pull request.

Unfortunately this approach doesn't work on openwrt; but then the existing timestamped output doesn't either, we need to use the tc_iterate C program on there anyway. So maybe that's okay, and since the binary uses a timerfd, maybe we don't even need to switch that over to using 'length' either (since the 'count' should be accurate enough when the kernel runs the timer).

Some comments on the code below...

>  }
 
 while getopts "c:I:H:t:p:f:" opt; do
     case $opt in
-        c) count="$OPTARG" ;;
+        l) length="$OPTARG" ;;

You also need to change the opts string passed to getopts (`c:` to `l:`)

> @@ -10,6 +12,7 @@ while getopts "i:c:I:C:H:" opt; do
     case $opt in
         i) interface=$OPTARG ;;
         c) count=$OPTARG ;;
+        l) length=$OPTARG ;;

Same thing as above re: changing the optstring, also here you're not removing the `count` option...

> @@ -1,7 +1,9 @@
 #!/bin/bash
+# set -x

Leftover debug setting?

> @@ -41,7 +41,8 @@ then
 fi
 
 command_string=$(cat <<EOF
-for i in \$(seq $count); do
+endtime=\$(date -d "\$length sec" +%s%N);

Here you shouldn't be escaping the `$` - we want it evaluated while setting the command, not on the remote host.

> @@ -22,7 +25,8 @@ buffer=""
 
 command_string=$(cat <<EOF
 which tc_iterate >/dev/null && exec tc_iterate $buffer -i $interface -c $count -I $interval -C $command;
-for i in \$(seq $count); do
+endtime=\$(date -d "\$length sec" +%s%N);

As above

> @@ -10,6 +12,7 @@ while getopts "i:c:I:C:H:" opt; do
     case $opt in
         i) interface=$OPTARG ;;
         c) count=$OPTARG ;;
+        l) length=$OPTARG ;;

Ah, you're still passing `-c` to the C binary. Guess that should be changed as well to support the `-l` option. Which means we'll need to detect if it understands it; ugh...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tohojo/flent/pull/237#pullrequestreview-778537436

[-- Attachment #2: Type: text/html, Size: 4464 bytes --]

[Flent-users] Re: [tohojo/flent] Runners: stop stats collection iterators after test-time is completed (#237)

[Shashank D]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

@shashank68 commented on this pull request.



> @@ -41,7 +41,8 @@ then
 fi
 
 command_string=$(cat <<EOF
-for i in \$(seq $count); do
+endtime=\$(date -d "\$length sec" +%s%N);

Oh, But wouldn't it be better if the endtime was calculated in the remote host (For e.g if ssh took few seconds to connect, then the stats collection time will be reduced) or am I missing something?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tohojo/flent/pull/237#discussion_r728330344

[-- Attachment #2: Type: text/html, Size: 2018 bytes --]

[Flent-users] Re: [tohojo/flent] Runners: stop stats collection iterators after test-time is completed (#237)

[Shashank D]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

@shashank68 pushed 1 commit.

794042fc001c08b4b2457ef3e27936dcacd02d8d  RunnerScripts: Stop collecting stats based on test endtime


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/tohojo/flent/pull/237/files/83def6ef91865dc6dc51bb74f8803518a095d9ea..794042fc001c08b4b2457ef3e27936dcacd02d8d

[-- Attachment #2: Type: text/html, Size: 1976 bytes --]

[Flent-users] Re: [tohojo/flent] Runners: stop stats collection iterators after test-time is completed (#237)

[Toke Høiland-Jørgensen]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

@tohojo commented on this pull request.



> @@ -41,7 +41,8 @@ then
 fi
 
 command_string=$(cat <<EOF
-for i in \$(seq $count); do
+endtime=\$(date -d "\$length sec" +%s%N);

Ah, yes, the sorry, the first `$` should be escaped, but not the second one. I.e., `$length` should be evaluated straight away...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tohojo/flent/pull/237#discussion_r728442510

[-- Attachment #2: Type: text/html, Size: 1967 bytes --]

[Flent-users] Re: [tohojo/flent] Runners: stop stats collection iterators after test-time is completed (#237)

[Shashank D]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

@shashank68 pushed 1 commit.

0c164f45268b578bffdb5bbe857a86af3c4b629d  RunnerScripts: Stop collecting stats based on test endtime


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/tohojo/flent/pull/237/files/794042fc001c08b4b2457ef3e27936dcacd02d8d..0c164f45268b578bffdb5bbe857a86af3c4b629d

[-- Attachment #2: Type: text/html, Size: 1976 bytes --]