Correct pbench-fio and pbench-uperf handling of benchmark existence and version checks (take 2)
#2860
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Both `pbench-fio` and `pbench-uperf`: * No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior * No longer resolve and check that the benchmark binary is executable There was an ordering problem with the old `benchmark_bin` variable initial value and where it is used in the rest of the script (both for `pbench-fio` and `pbench-uperf`). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely required `benchmark_bin` to be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether. Mock commands for `fio` and `uperf` are provided on the `PATH` for tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failing `test-CL`). We also add failing tests for `uperf` and `fio` behaviors: * `pbench-fio` * `test-22` -- missing `fio` command * `test-50` -- `fio -V` reports a bad version * `pbench-uperf` * `test-02` -- missing `uperf` command * `test-51` -- `uperf -V` reports a bad version The existence check of the `fio` and `uperf` benchmark commands now occurs after any help requests or command usage errors. This fixes issue distributed-system-analysis#2841 [1]. Finally, we correct the way `pbench-uperf` checked the status of the `uperf` command execution of the benchmark by making sure the `local` declaration is performed before the assigment, so that the return code check is not overridden by the "status" of the `local` declaration. This fixes issue distributed-system-analysis#2842 [2]. [1] distributed-system-analysis#2841 [2] distributed-system-analysis#2842
portante
added
bug
Agent
Code Infrastructure
fio
portante
changed the title
portante
changed the title
pbench-fio and pbench-uperf handling of benchmark existence and version checks (take 2)
dbutenhof
approved these changes
May 24, 2022
portante
added a commit
to portante/pbench
that referenced
this pull request
May 25, 2022
This is a back-port of commit ba6a2b7 (PR distributed-system-analysis#2860) from `main`. Both `pbench-fio` and `pbench-uperf`: * No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior * No longer resolve and check that the benchmark binary is executable There was an ordering problem with the old `benchmark_bin` variable initial value and where it is used in the rest of the script (both for `pbench-fio` and `pbench-uperf`). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely required `benchmark_bin` to be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether. Mock commands for `fio` and `uperf` are provided on the `PATH` for tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failing `test-CL`). We also add failing tests for `uperf` and `fio` behaviors: * `pbench-fio` * `test-22` -- missing `fio` command * `test-50` -- `fio -V` reports a bad version * `pbench-uperf` * `test-02` -- missing `uperf` command * `test-51` -- `uperf -V` reports a bad version The existence check of the `fio` and `uperf` benchmark commands now occurs after any help requests or command usage errors. This fixes issue distributed-system-analysis#2841 [1]. Finally, we correct the way `pbench-uperf` checked the status of the `uperf` command execution of the benchmark by making sure the `local` declaration is performed before the assigment, so that the return code check is not overridden by the "status" of the `local` declaration. This fixes issue distributed-system-analysis#2842 [2]. [1] distributed-system-analysis#2841 [2] distributed-system-analysis#2842
portante
added a commit
to portante/pbench
that referenced
this pull request
May 25, 2022
This is a back-port of commit ba6a2b7 (PR distributed-system-analysis#2860) from `main`. Both `pbench-fio` and `pbench-uperf`: * No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior * No longer resolve and check that the benchmark binary is executable There was an ordering problem with the old `benchmark_bin` variable initial value and where it is used in the rest of the script (both for `pbench-fio` and `pbench-uperf`). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely required `benchmark_bin` to be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether. Mock commands for `fio` and `uperf` are provided on the `PATH` for tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failing `test-CL`). We also add failing tests for `uperf` and `fio` behaviors: * `pbench-fio` * `test-22` -- missing `fio` command * `test-50` -- `fio -V` reports a bad version * `pbench-uperf` * `test-02` -- missing `uperf` command * `test-51` -- `uperf -V` reports a bad version The existence check of the `fio` and `uperf` benchmark commands now occurs after any help requests or command usage errors. This fixes issue distributed-system-analysis#2841 [1]. Finally, we correct the way `pbench-uperf` checked the status of the `uperf` command execution of the benchmark by making sure the `local` declaration is performed before the assigment, so that the return code check is not overridden by the "status" of the `local` declaration. This fixes issue distributed-system-analysis#2842 [2]. [1] distributed-system-analysis#2841 [2] distributed-system-analysis#2842
portante
added a commit
to portante/pbench
that referenced
this pull request
May 25, 2022
This is a back-port of commit ba6a2b7 (PR distributed-system-analysis#2860) from `main`. Both `pbench-fio` and `pbench-uperf`: * No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior * No longer resolve and check that the benchmark binary is executable There was an ordering problem with the old `benchmark_bin` variable initial value and where it is used in the rest of the script (both for `pbench-fio` and `pbench-uperf`). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely required `benchmark_bin` to be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether. Mock commands for `fio` and `uperf` are provided on the `PATH` for tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failing `test-CL`). We also add failing tests for `uperf` and `fio` behaviors: * `pbench-fio` * `test-22` -- missing `fio` command * `test-50` -- `fio -V` reports a bad version * `pbench-uperf` * `test-02` -- missing `uperf` command * `test-51` -- `uperf -V` reports a bad version The existence check of the `fio` and `uperf` benchmark commands now occurs after any help requests or command usage errors. This fixes issue distributed-system-analysis#2841 [1]. Finally, we correct the way `pbench-uperf` checked the status of the `uperf` command execution of the benchmark by making sure the `local` declaration is performed before the assigment, so that the return code check is not overridden by the "status" of the `local` declaration. This fixes issue distributed-system-analysis#2842 [2]. [1] distributed-system-analysis#2841 [2] distributed-system-analysis#2842
portante
added a commit
that referenced
this pull request
May 25, 2022
This is a back-port of commit ba6a2b7 (PR #2860) from `main`. Both `pbench-fio` and `pbench-uperf`: * No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior * No longer resolve and check that the benchmark binary is executable There was an ordering problem with the old `benchmark_bin` variable initial value and where it is used in the rest of the script (both for `pbench-fio` and `pbench-uperf`). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely required `benchmark_bin` to be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether. Mock commands for `fio` and `uperf` are provided on the `PATH` for tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failing `test-CL`). We also add failing tests for `uperf` and `fio` behaviors: * `pbench-fio` * `test-22` -- missing `fio` command * `test-50` -- `fio -V` reports a bad version * `pbench-uperf` * `test-02` -- missing `uperf` command * `test-51` -- `uperf -V` reports a bad version The existence check of the `fio` and `uperf` benchmark commands now occurs after any help requests or command usage errors. This fixes issue #2841 [1]. Finally, we correct the way `pbench-uperf` checked the status of the `uperf` command execution of the benchmark by making sure the `local` declaration is performed before the assigment, so that the return code check is not overridden by the "status" of the `local` declaration. This fixes issue #2842 [2]. [1] #2841 [2] #2842
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Both
pbench-fioandpbench-uperf:No longer allow for the benchmark binary to be overriden by an environment variable -- this was an out-dated way for the unit tests to mock the respective benchmark behavior
No longer resolve and check that the benchmark binary is executable
There was an ordering problem with the old
benchmark_binvariable initial value and where it is used in the rest of the script (both forpbench-fioandpbench-uperf). The existence check was not always performed locally (no clients or servers specified which are local), but the commands run remotely requiredbenchmark_binto be set during creation of the commands for remote execution. By only checking for the existence of the benchmark binary when performing the version check, and allowing the shell to resolve the location of the benchmark binary at run time, we avoid the interdependency altogether.Mock commands for
fioanduperfare provided on thePATHfor tests. For the CLI tests, those mocks are removed so that we can verify that help and usage text is emitted before the checks for the particular command existence (which is shown by the failingtest-CL).We also add failing tests for
uperfandfiobehaviors:pbench-fiotest-22-- missingfiocommandtest-50--fio -Vreports a bad versionpbench-uperftest-02-- missinguperfcommandtest-51--uperf -Vreports a bad versionThe existence check of the
fioanduperfbenchmark commands now occurs after any help requests or command usage errors. This fixes issue #2841 [1].Finally, we correct the way
pbench-uperfchecked the status of theuperfcommand execution of the benchmark by making sure thelocaldeclaration is performed before the assigment, so that the return code check is not overridden by the "status" of thelocaldeclaration. This fixes issue #2842 [2].[1] #2841
[2] #2842
This is a respin of PR #2582 to correct the mistaken merge without squashing.