[Flent-users] [tohojo/flent] Added --test-payload option (PR #243)

[Flent-users] [tohojo/flent] Added --test-payload option (PR #243)

[Hrishikesh Athalye]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Toke Høiland-Jørgensen]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Hrishikesh Athalye]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Hrishikesh Athalye]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Toke Høiland-Jørgensen]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Hrishikesh Athalye]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Hrishikesh Athalye]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Toke Høiland-Jørgensen]

[-- 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 --]

[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

[Toke Høiland-Jørgensen]

[-- 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 --]