Skip to content
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

Fix handling of labeled hosts by pbench-postprocess-tools (fwd port of #3456) #3472

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

ndokos
Copy link
Member

@ndokos ndokos commented Jun 27, 2023

Fixes #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 ':' as one element in the path but pbench-postprocess-tools looks for a '' 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 PR 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.

It also adds a "functional" test for pbench-list-tools to verify the output when labeled hosts have registered tools and 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.

Bump the version to 0.72.1

PBENCH-1178

…rt of distributed-system-analysis#3456)

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 PR 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.

It also adds a "functional" test for pbench-list-tools to verify
the output when labeled hosts have registered tools and
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.

Bump the version to 0.72.1

PBENCH-1178
@ndokos ndokos added bug Agent tools Of and related to the operation and behavior of various tools (iostat, sar, etc.) labels Jun 27, 2023
@ndokos ndokos added this to the v0.73 milestone Jun 27, 2023
@ndokos ndokos self-assigned this Jun 27, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I don't remember this being a flakey test, and there's not much info in the log. I guess I'll retry, but this may need investigation.

Testing bench-scripts/postprocess/fio-postprocess under sample perf-fio-0 ...
Duration: 967 seconds
FAIL - perf-fio-0

agent/VERSION Show resolved Hide resolved
@ndokos
Copy link
Member Author

ndokos commented Jun 27, 2023

The Duration message is printed by the unittests script if the duration of the test was more than MAX_PERFTEST_SECONDS (which by default is 600s). I guess, it's forcing the test to fail to attract attention to the fact that the processing takes so long, but I'm not sure why it is taking so long. In any case, it succeeded on the restarted run and also on the run after I rebased, but Github is still expecting a notification :-(

dbutenhof
dbutenhof previously approved these changes Jun 27, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I have sometimes gotten "half-a-bit" failures in some postprocessing scripts when comparing floating-point numbers: is the failure you are seeing of that kind or is it something else?

There weren't any details, so it's hard to say. In any case, it clearly didn't reproduce. Though I don't doubt that we'll see it again sooner or later ...

agent/VERSION Show resolved Hide resolved
@webbnh
Copy link
Member

webbnh commented Jun 27, 2023

I don't remember this being a flakey test

It is. It's much less flakey than "Area-51" (and -52), but it does occasionally hang and time out.

Make the version be 1.0.
webbnh
webbnh previously approved these changes Jun 27, 2023
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 good. Here are some nits and small things for consideration.

agent/util-scripts/pbench-postprocess-tools Outdated Show resolved Hide resolved
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
dbutenhof
dbutenhof previously approved these changes Jun 28, 2023
@ndokos ndokos dismissed stale reviews from dbutenhof and webbnh via af0547f June 29, 2023 00:49
I neglected to include this change in the previous commit.
@ndokos ndokos requested review from webbnh and dbutenhof June 29, 2023 01:05
dbutenhof
dbutenhof previously approved these changes Jun 29, 2023
lib/pbench/cli/agent/commands/tools/list.py Show resolved Hide resolved
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
webbnh
webbnh previously approved these changes Jun 29, 2023
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.

I have a pointed question, and I code suggestion.

lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
@ndokos ndokos dismissed stale reviews from webbnh and dbutenhof via 8bb4e4f June 30, 2023 19:01
dbutenhof
dbutenhof previously approved these changes Jun 30, 2023
lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
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.

Ship it!

lib/pbench/cli/agent/commands/tools/list.py Outdated Show resolved Hide resolved
@ndokos ndokos merged commit 775da21 into distributed-system-analysis:main Jul 3, 2023
@ndokos ndokos deleted the fwd3456 branch July 3, 2023 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agent bug tools Of and related to the operation and behavior of various tools (iostat, sar, etc.)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] v0.72.0 Using labels with pbench-register-tool is broken
3 participants