Skip to content

Conversation

@portante
Copy link
Member

@portante portante commented May 25, 2022

Provides back-ports of PRs #2863, #2856, #2860, and #2870.


Note that with the backport of #2860, where the resolve_benchmark_bin function was removed from agent/base, we still have three deprecated commands, pbench-dbench, pbench-iozone, and pbench-netperf, which were referencing that function. Since we don't want to introduce the which RPM as a dependency, we just use command -v in each of those commands instead.

@portante portante added bug Agent fio pbench-fio benchmark related uperf pbench-uperf benchmark related Tool Meister Of and relating to the Tool Meister sub-system labels May 25, 2022
@portante portante added this to the v0.71 milestone May 25, 2022
@portante portante self-assigned this May 25, 2022
@portante portante force-pushed the set-02-backports-for-b0.71 branch from 3969812 to 08fcd92 Compare May 25, 2022 17:15
@ndokos ndokos self-requested a review May 25, 2022 17:19
ndokos
ndokos previously approved these changes May 25, 2022
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

Typo in a comment. There is also a typo in the commit message of the #2863 backport (mdoule instead of module), but I certainly won't insist that either of those be fixed here.

dbutenhof
dbutenhof previously approved these changes May 25, 2022
webbnh
webbnh previously approved these changes May 25, 2022
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks OK to me (although, I skipped the unit tests...).

I have one comment which is probably better applied to main, but I only noticed it here.

portante added 3 commits May 25, 2022 14:47
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
This is a back-port of commit b51b116 (PR distributed-system-analysis#2856) from `main`.

We add a local and remote pre-check function for linpack.

We now wait at most 60 seconds for the linpack process to start, and
then we wait for the PID to exit instead of a file to exist.  That way
if a file is never written that we expect, but the PID has died, we'll
not wait forever.

Further, we move the "version" check (of sorts) to the linpack driver
script to allow for the remote check to work without having the code in
two places.
This is a back-port of commit c84c6ed (PR distributed-system-analysis#2863) from `main`.

When `pbench-tool-meister-stop` is invoked where the Redis server was
created locally by `pbench-tool-meister-start`, we always want to use
the `localhost` IP address to talk to it.

Also added unit tests for the `tool_meister_stop.py` module's
`RedisServer` class, and corrected the `pbench-tool-meister-stop` CLI
param default value for the `--redis-server` switch.

Fixes issue distributed-system-analysis#2861 [1].

[1] distributed-system-analysis#2861
portante added a commit to portante/pbench that referenced this pull request May 25, 2022
The review of the back-port PR distributed-system-analysis#2867 found a couple of minor fixes that
also need to be applied to `main`.
portante added a commit that referenced this pull request May 25, 2022
The review of the back-port PR #2867 found a couple of minor fixes that
also need to be applied to `main`.
@portante portante dismissed stale reviews from webbnh, dbutenhof, and ndokos via e98c83f May 25, 2022 19:00
@portante portante force-pushed the set-02-backports-for-b0.71 branch from 08fcd92 to e98c83f Compare May 25, 2022 19:00
@portante portante requested review from dbutenhof, ndokos and webbnh May 25, 2022 19:01
@portante portante merged commit 9d90a97 into distributed-system-analysis:b0.71 May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Agent bug fio pbench-fio benchmark related Tool Meister Of and relating to the Tool Meister sub-system uperf pbench-uperf benchmark related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants