Fixes for user-tool #3440
Merged
Fixes for user-tool #3440
Conversation
This file contains 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
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. This is a draft for now: I've done some "field" testing but I have not run/fixed the tests and it certainly could use more field testing by the original reporter to make sure it addresses the reported problems PBENCH-1172
webbnh
previously approved these changes
May 30, 2023
- Inline the functions and fix leading white space issues - Fix trap - Redirect stderr of pbench-list-tools to /dev/null.
`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.
Tests -51, -52 and -61 were failing because of the additional `install` logging. There are three more (-53, -56 and -57) that are failing, probably for the same reason, but I don't quite understand the output differences yet: TBD.
|
This forced push was necessitated by my changing the commit message of the first commit (to add the issue ID and reword slightly). The code in that first commit should be identical. There are three more commits: one to address review comments, one to fix the three broken python tests and one to fix three of the six broken legacy tests. The other three are still broken, so still in draft mode. |
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.
dbutenhof
previously approved these changes
May 31, 2023
webbnh
previously approved these changes
May 31, 2023
I've "pre-resolved" them, since merging |
dbutenhof
approved these changes
Jun 1, 2023
|
Area 51 strikes again... I'll rebase to pick up the latest and start the tests again. |
ndokos
added a commit
to ndokos/pbench
that referenced
this pull request
Jun 7, 2023
* 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
ndokos
added a commit
to ndokos/pbench
that referenced
this pull request
Jun 15, 2023
* 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
ndokos
added a commit
to ndokos/pbench
that referenced
this pull request
Jun 15, 2023
* 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 with 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
ndokos
added a commit
to ndokos/pbench
that referenced
this pull request
Jun 26, 2023
* 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 with 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
ndokos
added a commit
that referenced
this pull request
Jun 27, 2023
* 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 with 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The tool meister was treating tools options as a concatenated string that was passed as a single argument to
pbench-start/stop-tools. They are now split into separate arguments, allowing the scripts to work correctly.pbench-postprocess-toolswas being called without any 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 usepbench-list-tools -g <group> -oand 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.This has undergone some "field" testing but could certainly use more. All the tests are now passing in Jenkins (although see the last commit for some problems).
PBENCH-1172