Forward port to main some changes from b0.72 #3457
Conversation
* Fixes for user-tool The tool meister was treating tools options as a concatenated string that was passed as a single argument to `pbench-install/start/stop-tools`. They are now split into separate arguments, allowing the scripts to work correctly. `pbench-postprocess-tools` was being called without empty tool options. Instead of burrowing even deeper into the filesystem to grab the options, we get rid of most of that by rewriting it to use `pbench-list-tools -g <group> -o` and parsing the output of that script. The parsing is ugly but is arguably less ugly than the file system ad-hockery that we used to go through. * Fix failures in the python tests `test_install` was using a NullObject for a logger, but the `install` method now logs its command. So we set up a basic logger to satisfy that need. Two other tests (`test_start` and `test_stop`) failed on an `is` test. We now test for equality, rather than identity and the tests pass. I'm not sure what caused these two failures, but the cause is probably the breakup of the options into separate arguments. * Fix legacy util-scripts tests Tests -51, -52 and -61 were failing because of the additional `install` logging. Tests -53, -56 and -57 now pass: the new output looks correct to me and the old output looks wrong. The util-scripts also pass in the CI container (run locally with `jenkins/run tox -- agent util-scripts`), except for -51 and -52 which fail with identical errors: - pbench-tool-meister-stop: waiting for tool-data-sink (#####) to exit The line is missing for some reason - I hope that the real Jenkins will pass them, but it's not clear to me why they fail locally. OTOH, they do pass locally when run with `tox -- agent util-scripts`. Explanations/theories/guesses are welcome. PBENCH-1172
Fixes distributed-system-analysis#3454 pbench-postprocess-tools mishandles hosts with labels (added by tool registration commands): it ignores the label and complains that it cannot find the tool output directory. The tool output directory path contains '<label>:<host>' as one element in the path but pbench-postprocess-tools looks for a '<host>' element. pbench-postprocess-tools parses the output of pbench-list-tools to get the tool info it needs, but pbench-list-tools omits the label from its output. This commit fixes pbench-list-tools to add the label to its output and pbench-postprocess-tools to parse that output, derive the label and use it to construct the path of the tool output directory. The next commit deals with tests. PBENCH-1178
This commit adds a "functional" test for pbench-list-tools to verify the output when labeled hosts have registered tools. It also fixes an incorrect legacy test (test-07) for pbench-postprocess-tools: the test was using a labeled host, but it was testing the wrong tool output directory (without the label). It adds a similar legacy test (test-07.1) to test the unlabeled host case.
Fixes distributed-system-analysis#3443 Modify the help message of pbench-register-tool to warn against using operational options (i.e. options used internally by pbench) for a tool when it is registered. Modify the help message of base-tool (and everybody who is symlinked to it) to clarify which are operational options and which are tool-specific options: operational options should not be specified when registering a tool: they are used by pbench internally; only tool-specific options are specifed when registering the tool (on the RHS of `--` in the invocation of `pbench-register-tool`). PBENCH-1173
ndokos
added
bug
enhancement
Agent
Documentation
tools
There are, as it currently stands, 58 files in this PR!...I vote for subdividing it. Also, when this PR is merged, I think we're going to want it in multiple commits -- I would recommend posting a PR for each commit (or set of to-be-squashed commits) that you want to merge...which I think is three. |
|
Yes, I was planning to do three commits, one for each original b0.72 PR. I think these three are related (well, two are related in that they fix bugs in |
The problem with that is, if there is any feedback in the PR which prompts you to add another commit, you need to add it to the end of the branch to avoid disrupting the review process, and then, once you've got approval, you need to squash the new commit(s) into the appropriate original commit(s), which then requires another round of review/approvals. I think it's easier if we plan and intend for each PR to produce one commit. Then, any feeback which results in additional commits can be rolled up using GitHub's squash-and-merge function without requiring an extra round of review.
Clearly, getting the first 40+ files out into their own PR would be a huge help. Whether we take the remaining 16 as one PR or two...I guess it depends on whether you really think of them as "one change" or not. |
|
I'll submit a forward port of #3444 to main as a separate PR. I do think of the other two as one change, so I will try to squash them into a single commit and see how that looks. Depending on how that goes, I'll figure out what to do with them. Thanks for the comments! |
This is a forward port to main of three b0.72 PRs:
They are cherry-picks of the corresponding b0.72 commits with no changes.
Set to draft mode for now to wait for #3456 to be merged first and to decide whether a single PR is a reasonable approach. It seems reasonable to me, but if there are objections, please let me know.