Flent-users discussion archives
 help / color / mirror / Atom feed
* [Flent-users] [tohojo/flent] Added --test-payload option (PR #243)
@ 2021-10-19 20:11 Hrishikesh Athalye
  2021-10-19 22:30 ` [Flent-users] " Toke Høiland-Jørgensen
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Hrishikesh Athalye @ 2021-10-19 20:11 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

Added the --test-payload option for sending a custom payload by using netperf's buffer prefill option
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * <a href="https://github.com/tohojo/flent/pull/243/commits/200c8bf67ea360c573c41399e0153a5e476731b3">Added --test-payload option</a>

-- File Changes --

    M flent/runners.py (9)
    M flent/settings.py (6)
    M flent/testenv.py (2)

-- Patch Links --

https://github.com/tohojo/flent/pull/243.patch
https://github.com/tohojo/flent/pull/243.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/243

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
@ 2021-10-19 22:30 ` Toke Høiland-Jørgensen
  2021-10-20  2:49 ` Hrishikesh Athalye
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-19 22:30 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

@tohojo requested changes on this pull request.

Please add a paragraph of text to the commit message describing why the feature is useful.

A few nits on the code, but otherwise looks good! :)

>                  # Sanity check; is /dev/urandom readable? If so, use it to
                 # pre-fill netperf's buffers
-                self.run_simple(['dd', 'if=/dev/urandom', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
-                netperf['buffer'] = '-F /dev/urandom'
+                fillFile = args['test_payload']
+                self.run_simple(['dd', f'if={fillFile}', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
+                netperf['buffer'] = f'-F {fillFile}'

Flent runs on Python versions as old as 3.5, so we can't use f-strings, sadly...

> @@ -1022,10 +1023,14 @@ def check(self):
                 netperf['-e'] = True
 
             try:
+                # If --test-payload option is specified, use data from that file
+                # else use the default value /dev/urandom. 
+                # Below sanity check will be performed in either case.

I'm not sure if the sanity check makes sense for a user-specified file: it means we'll just silently ignore the option if the file is not readable. I think it's better to fail noisily. We could do this either by doing the check but erroring out if it fails (it's the default), or we could skip the test for custom values and just let netperf fail when it can't open it.

> @@ -378,6 +378,12 @@ def __call__(self, parser, namespace, values, option_string=None):
     "to decrease the socket buffer size. Can be specified multiple times, "
     "with each value corresponding to a stream of a test.")
 
+test_group.add_argument(
+    "--test-payload",
+    action="store", type=unicode, dest="TEST_PAYLOAD", default='/dev/urandom',
+    help="Path to file containing payload to pre-fill the netperf buffers with"

Missing space at the end of the first string here

>                  # Sanity check; is /dev/urandom readable? If so, use it to
                 # pre-fill netperf's buffers
-                self.run_simple(['dd', 'if=/dev/urandom', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
-                netperf['buffer'] = '-F /dev/urandom'
+                fillFile = args['test_payload']

Variables are generally lowercase and underscore-separated rather then camelCase...

-- 
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/243#pullrequestreview-783839744

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
  2021-10-19 22:30 ` [Flent-users] " Toke Høiland-Jørgensen
@ 2021-10-20  2:49 ` Hrishikesh Athalye
  2021-10-20 14:50 ` Hrishikesh Athalye
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Hrishikesh Athalye @ 2021-10-20  2:49 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Push

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

@hrishikeshathalye pushed 1 commit.

c40bc60697802034e7bb0948c37783c357cd337b  This commit adds a feature to give the user the option to specify


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/tohojo/flent/pull/243/files/ff214ce3ddf33ad8b441c2e8f8b8dc18135c2528..c40bc60697802034e7bb0948c37783c357cd337b

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
  2021-10-19 22:30 ` [Flent-users] " Toke Høiland-Jørgensen
  2021-10-20  2:49 ` Hrishikesh Athalye
@ 2021-10-20 14:50 ` Hrishikesh Athalye
  2021-10-20 21:20 ` Toke Høiland-Jørgensen
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Hrishikesh Athalye @ 2021-10-20 14:50 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

Thank you for the suggestions. Have modified the code accordingly.

-- 
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/243#issuecomment-947745301

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
                   ` (2 preceding siblings ...)
  2021-10-20 14:50 ` Hrishikesh Athalye
@ 2021-10-20 21:20 ` Toke Høiland-Jørgensen
  2021-10-21  3:40 ` Hrishikesh Athalye
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-20 21:20 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

@tohojo requested changes on this pull request.

That's better! One more small nit below. Also, the commit message you added is great, but please squash the two commits together so that it becomes one commit that keeps the heading of the first one, and has the text of the second commit as its body.

Also, this sentence:

> Using the option will cause netperf to fail noisily, in case the specified file is not readable or does not exist.

is slightly misleading. It's not netperf that fails noisily, it's the check in flent. So maybe "Flent will check if the file is readable before passing it to netperf, and fail the test run if it isn't" ?

>              except RunnerCheckError:
-                netperf['buffer'] = ''
+                if(fill_file == '/dev/urandom'):
+                    netperf['buffer'] = ''
+                else:
+                    # If the custom file is not readable, fail noisily
+                    raise RunnerCheckError("The specified fill file does not exist or is not readable.")

The option is --test-payload, so referring to "fill file" in the error message is going to confuse people. Better stick to "test payload" here as well :)

-- 
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/243#pullrequestreview-785005703

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
                   ` (3 preceding siblings ...)
  2021-10-20 21:20 ` Toke Høiland-Jørgensen
@ 2021-10-21  3:40 ` Hrishikesh Athalye
  2021-10-21  3:41 ` Hrishikesh Athalye
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Hrishikesh Athalye @ 2021-10-21  3:40 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

@hrishikeshathalye commented on this pull request.



>              except RunnerCheckError:
-                netperf['buffer'] = ''
+                if(fill_file == '/dev/urandom'):
+                    netperf['buffer'] = ''
+                else:
+                    # If the custom file is not readable, fail noisily
+                    raise RunnerCheckError("The specified fill file does not exist or is not readable.")

Ah yes. That would be more accurate. Edited.

-- 
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/243#discussion_r733296393

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
                   ` (4 preceding siblings ...)
  2021-10-21  3:40 ` Hrishikesh Athalye
@ 2021-10-21  3:41 ` Hrishikesh Athalye
  2021-10-21 10:40 ` Toke Høiland-Jørgensen
  2021-10-21 11:20 ` Toke Høiland-Jørgensen
  7 siblings, 0 replies; 9+ messages in thread
From: Hrishikesh Athalye @ 2021-10-21  3:41 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Push

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

@hrishikeshathalye pushed 1 commit.

42e0f29e50bf98a47ff961a3d677b537f8604c94  Added --test-payload option


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/tohojo/flent/pull/243/files/c46b8e76e768b6f9bda121af85cc5ba81773472c..42e0f29e50bf98a47ff961a3d677b537f8604c94

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
                   ` (5 preceding siblings ...)
  2021-10-21  3:41 ` Hrishikesh Athalye
@ 2021-10-21 10:40 ` Toke Høiland-Jørgensen
  2021-10-21 11:20 ` Toke Høiland-Jørgensen
  7 siblings, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-21 10:40 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

@tohojo approved this pull request.





-- 
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/243#pullrequestreview-785515354

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)
  2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
                   ` (6 preceding siblings ...)
  2021-10-21 10:40 ` Toke Høiland-Jørgensen
@ 2021-10-21 11:20 ` Toke Høiland-Jørgensen
  7 siblings, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-21 11:20 UTC (permalink / raw)
  To: tohojo/flent; +Cc: Subscribed

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

Merged #243 into master.

-- 
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/243#event-5497802546

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-10-21 11:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 20:11 [Flent-users] [tohojo/flent] Added --test-payload option (PR #243) Hrishikesh Athalye
2021-10-19 22:30 ` [Flent-users] " Toke Høiland-Jørgensen
2021-10-20  2:49 ` Hrishikesh Athalye
2021-10-20 14:50 ` Hrishikesh Athalye
2021-10-20 21:20 ` Toke Høiland-Jørgensen
2021-10-21  3:40 ` Hrishikesh Athalye
2021-10-21  3:41 ` Hrishikesh Athalye
2021-10-21 10:40 ` Toke Høiland-Jørgensen
2021-10-21 11:20 ` Toke Høiland-Jørgensen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox