-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: Convert current pbench MarkDown content to readthedocs and reStructuredText format #1730
Docs: Convert current pbench MarkDown content to readthedocs and reStructuredText format #1730
Conversation
Thanks @haoweiqiu! I'll review soon. |
Can you also consider the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just skimmed this; comments and questions on content and formatting.
Maybe I'm unclear on the context of this review, but I see missing "pbench-" prefixes on tools, jumbled sets of command lines without line breaks, and odd sample output including Pbench debug statements. Just a few examples are marked in this review. I'll read more later.
The first step is to install the necessary dependencies that are | ||
required to run on the system: | ||
|
||
sudo yum install -y perl-JSON sudo yum install -y python3-pip sudo pip3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hard to read as a merged paragraph. Seems this should be a '::' code literal with line breaks.
This project uses the flake8 method of code style enforcement, linting | ||
and checking, `flake8 <http://flake8.pycqa.org/en/latest>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually a combination of black
and flake8
, with black set to detect if any files would be modified by the black code formatting rules, and flake8 to check for other Python style errors.
tox -e agent (for pbench agent) tox -e server (for pbench server) tox -e | ||
pytest\_agent (for pytest pbench agent) tox -e pytest\_server (for | ||
pytest pbench server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a "::" code block? Although with the parenthetical descriptions it's not actually code, the merged paragraph is still difficult to follow.
sar tool is now registered in group default | ||
[debug]tool_opts: default --interval="3" | ||
[debug]checking to see if tool is installed... | ||
iostat is installed | ||
iostat tool is now registered in group default | ||
[debug]tool_opts: default --interval="3" | ||
[debug]checking to see if tool is installed... | ||
mpstat is installed | ||
mpstat tool is now registered in group default | ||
[debug]tool_opts: default --interval="3" | ||
[debug]checking to see if tool is installed... | ||
pidstat is installed | ||
pidstat tool is now registered in group default | ||
[debug]tool_opts: default --interval="3" | ||
[debug]checking to see if tool is installed... | ||
proc-vmstat tool is now registered in group default | ||
[debug]tool_opts: default --interval="3" | ||
[debug]checking to see if tool is installed... | ||
proc-interrupts tool is now registered in group default | ||
[debug]tool_opts: default --record-opts="record -a --freq=100" | ||
[debug]checking to see if tool is installed... | ||
perf tool is now registered in group default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I do this I don't get any of the [debug]
lines, and I wouldn't expect to see them under normal circumstances. Is this right?
|
||
:: | ||
|
||
# list-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be pbench-list-tools
|
||
:: | ||
|
||
# list-tools --with-options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, pbench-list-tools
|
||
:: | ||
|
||
# register-tool --name=pidstat -- --interval=10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench-register-tool
Also, again, I don't think the [debug]
log lines in the output are correct.
If this is the first time running ``fio`` via the ``pbench-agent``, | ||
``pbench-agent`` will attempt to download and compile ``fio``. You may | ||
see quite a bit of output from this. Once ``fio`` is installed, | ||
``pbench-agent`` will run several tests by default. Output for each will | ||
look something like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a question for Pete or Nick, but my understanding is that pbench wasn't doing this anymore; and for me this just gives me an error that fio-3.19 is not installed.
register-tool-set | ||
user-benchmark -C test1 -- ./your_cmd.sh | ||
move-results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these are pbench-
prefixed commands.
|
||
:: | ||
|
||
pbench_fio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench-fio, not underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start.
This adds a new directory, docs
. What are you thinking we should do with the existing doc
directory? I was hoping we could make these changes in the doc
directory, just moving all of the current "mark down" files we have (.md
) to reStructuredText that is supported by GitHub.
Does GitHub's rendering of the new documents work as expected?
@@ -0,0 +1,35 @@ | |||
@ECHO OFF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this file from the commit, we won't be supporting a build on Windows in the foreseeable future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is based on a very old set of docs that does not follow current conventions. It needs a fair amount of alterations (the lack of the pbench-
prefix on most commands is the most obvious omission).
# -- Project information ----------------------------------------------------- | ||
|
||
project = 'pbench-docs' | ||
copyright = '2020, Haowei' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the copyright be assigned to a RH entity?
|
||
:: | ||
|
||
move-results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench- prefix missing
|
||
:: | ||
|
||
copy-results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench- prefix missing
|
||
register-tool-set | ||
user-benchmark --config=foo -- myscript.sh | ||
move-results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench- prefix missing on last three lines
results_repo_dir=/pbench/public_html/incoming | ||
ssh_opts='-o StrictHostKeyChecking=no' | ||
|
||
These are now specified in the config file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section does not sound right: base
does set a bunch of variables but most of these are not specified in the config file. In particular, date
, hostname
, results_repo
and results_repo_dir
are not specified in the config file. The last two I can't find anywhere, either in the config file or in base
.
ssh_opts='-o StrictHostKeyChecking=no' | ||
|
||
These are now specified in the config file | ||
``/opt/pbench-agent/config/pbench.conf``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config file is /opt/pbench-agent/config/pbench-agent.cfg
.
=============== | ||
|
||
The configured default set of tools (what you would get by running | ||
``register-tool-set``) is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pbench-
What is the process of publishing these documents on |
This has been fixed with #3428 |
This PR converts current pbench MarkDown content to readthedocs and reStructuredText format.
The ReadtheDocs link is: https://pbench-docs.readthedocs.io/