Man pages for pbench-agent commands #3442

siddardh-ra · 2023-05-31T07:12:37Z

Add man_page.md file for the pbench-agent commands.

Note: This PR doesn't cover all the pbench-agent commands. For the 1st iteration, decided to cover the commonly used pbench-agent commands. If needed we can add more in subsequent iterations

pravins

Hi Siddardh,
Thanks for including reviews done earlier on the document.
I always get confused between pbench-results-move and pbench-move-results, can you add explanation their.
pbench-results-move used when pushing results using token.
pbench-move-results with ssh way.

pravins

Thanks Siddardh for the update. Looks good to me.

dbutenhof

This is a good start!

I'm not entirely sure we want a combined "man page" for all the agent commands in one file. I could see these in separate files, even building a directory tree for "performance tool management commands", "benchmark commands", and "publish to Pbench Server" commands... or at least a section/subsection hierarchy that makes the command "classes" easier to locate.

I'd like to see a more clear distinction between the 0.69 Server pbench-*-results commands and the 1.0 Server pbench-results-move command. I'm tempted to suggest a "Publish results to server" section with "Pbench Server 0.69" and "Pbench Server 1.0" sub-sections to keep them clearly separated and provide more context about why they're separated. E.g., "The Pbench Server 0.69 uses an SSH protocol to push tarballs, requiring that the agent have a private id_rsa key for the server's pbench account; the Pbench Server 1.0 uses an encrypted authentication token identifying a specific user account."

But also, if we're including pbench-user-benchmark why aren't we including pbench-fio and pbench-uperf and such?

docs/Agent/user-guide/man_page.md

webbnh

This is a good start. However, I have some thoughts:

First, there should be a table-of-contents at the top, grouped topically, with subsections or annotations pointing out which commands are v0.69 commands (and soon to be deprecated) and which are v1.0 commands.
Second, there are a bunch of places where we should be using monospaced font (e.g., anything in single quotes should probably be in "back quotes").
The text around "required" options should probably be reworked, since most of them pull their values from environment variables which are automatically defined for most users, so the options don't actually have to be specified in typical usage.

And, I'm concerned about how we are going to keep this file in sync with the help text produced by the commands. I'm wondering if this file shouldn't be generated automatically from the commands.

docs/Agent/user-guide/man_page.md

pravins

seems even better now.

dbutenhof

Some "nits", but I think this looks good enough as a base reference that I'm going to approve. It doesn't have to be "perfect" as long as it's not wrong...

docs/Agent/user-guide/man_page.md

webbnh

This is looking good, but I think it needs another round of polish.

docs/Agent/user-guide/man_page.md

pravins

Thanks Siddardh for enhancing it further.

docs/Agent/user-guide/man_page.md

dbutenhof

Sorry about this, Siddardh; I've discovered that the header levels mean more to GitHub than I'd thought, so I think we need to take this a bit more seriously. 😦

docs/Agent/user-guide/man_page.md

pravins · 2023-06-06T12:57:05Z

Already 131 comments into this PR.
With present review we have good things in this PR.
IMHO, lets go ahead with available contents in the PR and post a new fresh PR with fixes/enhancement.
Anyway we have to keep on updating this document regularly :)

webbnh

This is looking great, but with this latest change there are now places where we've lost the indications of which command options take values. I'm OK with using [OPTIONS] as a shorthand in the command line synopses, but, if we're going that direction, then the individual option synopses have to indicate when the options take values. Also, there are a few cases where the options were only noted in the command line synopses; we now need to ensure that they aren't missing from option descriptions. And, there is a similar problem with command arguments. (And, I noted a couple of new nits.)

docs/Agent/user-guide/man_page.md

dbutenhof

I made a bunch of comments I noticed on re-reading, but assuming that this gets merged soon enough I'll edit it in my --relay PR anyway and I'd be happy to do additional cleanup then ...

docs/Agent/user-guide/man_page.md

… for some descriptions

webbnh

OK, I think that last update covers everything. There's one improvement to be made, but Dave says that he will do a clean-up pass in his coming PR, so let's dump that on him. 😉

webbnh · 2023-06-07T19:24:57Z

docs/Agent/user-guide/man_page.md

@@ -45,7 +45,7 @@
 This command clears the results directory from `/var/lib/pbench-agent` directory.

 **OPTIONS**  
- `-C`, `--config` PATH  
+ `-C`, `--config PATH`  


The way it was wasn't bad...the thing is, if the user uses -C, s/he still needs to provide PATH...so we need some way to convey that -C and --config are synonyms, each requiring a value.

Something like

`[-C | --config] <PATH>`

or

`-C <PATH>`, `--config <PATH>`

I've also seen man pages (like du(1)) which add text like "Mandatory arguments to long options are mandatory for short options too" and then the option synopses look like -B, --block-size=SIZE.

Yeah, I was moderately happy with the old "-C, --config PATH" although something like Webb's more explicit alternation syntax plus a symbolic placeholder would be nicer. Like [-C | --config] <path>. But that's a detail, and I can clean this up after I rebase when I add the new options.

dbutenhof

Yeah; good enough. I'll tweak stuff after I rebase. Thanks a lot for all the work, Siddardh! Despite the volume of comments, this is a great base to work from. (And to misquote a famous agrammatical American movie line, "all your hard work ain't been in vain for nothing." 😆

dbutenhof · 2023-06-07T19:39:29Z

docs/Agent/user-guide/man_page.md

@@ -45,7 +45,7 @@
 This command clears the results directory from `/var/lib/pbench-agent` directory.

 **OPTIONS**  
- `-C`, `--config` PATH  
+ `-C`, `--config PATH`  


Yeah, I was moderately happy with the old "-C, --config PATH" although something like Webb's more explicit alternation syntax plus a symbolic placeholder would be nicer. Like [-C | --config] <path>. But that's a detail, and I can clean this up after I rebase when I add the new options.

docs/Agent/user-guide/man_page.md

PBENCH-1142 This changes the Pbench Agent `results` mechanism to support a new mode, refactoring the `CopyResults` class into a hierarchy with `CopyResultToServer` and `CopyResultToRelay` and an overloaded `push` to do the work. The `--token` option is now required only when `--relay` is not specified. In addition to `--relay <relay>` to push a manifest and tarball to a Relay, I added a `--server <server>` to override the default config file value, which should allow us to deploy a containerized Pbench Agent without needing to map in a customized config file just to set the Pbench Server URI. The agent "man pages" have been updated with the new options, and some general cleanup left over from distributed-system-analysis#3442. _NOTE_: with this change we have a full end-to-end relay mechanism, but it's simplistic. You need to start a relay server manually from the file relay repo at `distributed-system-analysis/file-relay`, and supply that URI to the `pbench-results-move --relay <relay>` command. In the future we'd like to package the relay and allow management and hosting through Pbench Agent commands within a standard container.

* Add basic relay support to Pbench Agent PBENCH-1142 This changes the Pbench Agent `results` mechanism to support a new mode, refactoring the `CopyResults` class into a hierarchy with `CopyResultToServer` and `CopyResultToRelay` and an overloaded `push` to do the work. The `--token` option is now required only when `--relay` is not specified. In addition to `--relay <relay>` to push a manifest and tarball to a Relay, I added a `--server <server>` to override the default config file value, which should allow us to deploy a containerized Pbench Agent without needing to map in a customized config file just to set the Pbench Server URI. The agent "man pages" have been updated with the new options, and some general cleanup left over from #3442. _NOTE_: with this change we have a full end-to-end relay mechanism, but it's simplistic. You need to start a relay server manually from the file relay repo at `distributed-system-analysis/file-relay`, and supply that URI to the `pbench-results-move --relay <relay>` command. In the future we'd like to package the relay and allow management and hosting through Pbench Agent commands within a standard container.

siddardh-ra requested review from riya-17 and pravins May 31, 2023 07:12

siddardh-ra self-assigned this May 31, 2023

siddardh-ra added Agent Documentation labels May 31, 2023

pravins reviewed May 31, 2023

View reviewed changes

siddardh-ra requested a review from pravins May 31, 2023 10:24

pravins previously approved these changes May 31, 2023

View reviewed changes

dbutenhof requested changes May 31, 2023

View reviewed changes

webbnh requested changes May 31, 2023

View reviewed changes

siddardh-ra dismissed pravins’s stale review via 8389597 June 2, 2023 14:03

siddardh-ra requested review from webbnh, dbutenhof and pravins June 2, 2023 14:03

pravins previously approved these changes Jun 2, 2023

View reviewed changes

dbutenhof previously approved these changes Jun 2, 2023

View reviewed changes

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

webbnh reviewed Jun 2, 2023

View reviewed changes

siddardh-ra dismissed stale reviews from dbutenhof and pravins via 3496998 June 2, 2023 20:37

siddardh-ra requested review from webbnh, dbutenhof and pravins June 2, 2023 20:38

pravins previously approved these changes Jun 3, 2023

View reviewed changes

dbutenhof requested changes Jun 3, 2023

View reviewed changes

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

siddardh-ra dismissed pravins’s stale review via e1e87bc June 5, 2023 07:32

siddardh-ra requested a review from dbutenhof June 5, 2023 07:39

dbutenhof requested changes Jun 5, 2023

View reviewed changes

docs/Agent/user-guide/man_page.md Outdated Show resolved Hide resolved

siddardh-ra force-pushed the PBENCH-1154 branch from e3fa965 to d574753 Compare June 5, 2023 13:06

siddardh-ra requested review from dbutenhof and pravins June 5, 2023 13:08

siddardh-ra force-pushed the PBENCH-1154 branch from 64d2b08 to bd0e6da Compare June 6, 2023 23:42

siddardh-ra dismissed pravins’s stale review via 093ed3d June 7, 2023 00:05

siddardh-ra requested review from webbnh and dbutenhof June 7, 2023 00:05

siddardh added 10 commits June 7, 2023 21:43

Man pages for pbench-agent commands

f6d59bb

Review comments addressed and beautified md file ..

2c56e42

Added ToC to man page and addressed review comments

c4469cf

Sorted in alphabetical order and addressed review comments

2a5972c

Some missed review corrections

08d5cb9

Few more review comments

fdb6131

Updated header levels in the appropriate format

2ef7643

Another set of review comments ...

5777521

Some minor updates for Capital letters in starting of the sentence

ddfc710

Beautified this file and addressed review comments

b09209c

siddardh-ra force-pushed the PBENCH-1154 branch from 093ed3d to b09209c Compare June 7, 2023 16:13

webbnh reviewed Jun 7, 2023

View reviewed changes

dbutenhof previously approved these changes Jun 7, 2023

View reviewed changes

Imporved consistency across the page and mentioned [To be supplied]…

49cf3b1

… for some descriptions

siddardh-ra dismissed dbutenhof’s stale review via 49cf3b1 June 7, 2023 19:14

webbnh approved these changes Jun 7, 2023

View reviewed changes

dbutenhof approved these changes Jun 7, 2023

View reviewed changes

dbutenhof merged commit 4826076 into distributed-system-analysis:main Jun 7, 2023

dbutenhof mentioned this pull request Jun 8, 2023

Add basic relay support to Pbench Agent #3460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Man pages for pbench-agent commands #3442

Man pages for pbench-agent commands #3442

siddardh-ra commented May 31, 2023

pravins left a comment

pravins left a comment

dbutenhof left a comment

webbnh left a comment

pravins left a comment

dbutenhof left a comment

webbnh left a comment

pravins left a comment

dbutenhof left a comment

pravins commented Jun 6, 2023

webbnh left a comment

dbutenhof left a comment

webbnh left a comment

webbnh Jun 7, 2023

dbutenhof Jun 7, 2023

dbutenhof left a comment

dbutenhof Jun 7, 2023

Man pages for pbench-agent commands #3442

Man pages for pbench-agent commands #3442

Conversation

siddardh-ra commented May 31, 2023

pravins left a comment

Choose a reason for hiding this comment

pravins left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

pravins left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

pravins left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

pravins commented Jun 6, 2023

webbnh left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh Jun 7, 2023

Choose a reason for hiding this comment

dbutenhof Jun 7, 2023

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

dbutenhof Jun 7, 2023

Choose a reason for hiding this comment