Add the option to Zathras to turn on and off pcp in the wrappers.

[dvalinrh]

Description

Add the option to Zathras to turn on and off pcp in the wrappers. PCP will be on by default.

Requires redhat-performance/test_tools-wrappers#112 to be fixed at the same time.

Before/After Comparison

Before: Zathras did nothing with the --use_pcp option to the wrappers.
After: Zathras will nw set by default --use_pcp, user may override via --pcp_state 0

Documentation Check

Yes. Both usage for burden and the documentation has been updated.

Clerical Stuff

This closes #314

Relates to JIRA: RPOPC-683

[github-actions]

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the proposed changes. Here's my analysis:

Overview

This PR introduces a new command-line option, --pcp_state, to control whether Performance Co-Pilot (PCP) is used by the test wrappers. This allows users to disable PCP integration if desired. The changes involve modifications to the burden script, the test_generic Ansible role, and the command_line_reference.md documentation file.

Analysis

1. Code Quality and Maintainability:

• The changes seem straightforward and relatively easy to understand.
• The use of a global variable gl_pcp_state in burden is consistent with other global options.
• The addition of the pcp_state variable to ansible_vars_main.yml seems logical.
• The conditional inclusion of {{ config_info.pcp_state }} in the cmd_line fact within test_generic/tasks/main.yml is a clean way to enable/disable PCP.

2. Potential Bugs or Security Issues:

• The default value of gl_pcp_state is set to "--use_pcp". This means that PCP will be enabled by default unless the user explicitly disables it. This seems reasonable.
• It is important to ensure that the test wrappers correctly handle the --use_pcp option (or absence thereof). It should not cause errors if the option is present but PCP is not installed on the system under test (SUT). This is outside the scope of the current diff, but should be verified in separate tests.
• There are no obvious security concerns.

3. Adherence to Project Conventions:

• The changes appear to follow the existing coding style and conventions.
• The use of Ansible roles and Jinja2 templating is consistent with other parts of the project.
• The addition of a command-line option and its corresponding handling in burden is similar to how other options are implemented.

4. Documentation Completeness:

• The command_line_reference.md file is updated to include documentation for the new --pcp_state option.
• The documentation clearly explains the purpose of the option and its possible values.

Specific Feedback

• Missing Default in grab_cli_data(): The grab_cli_data() function in bin/burden should include "pcp_state" to ensure the value is correctly passed into the Ansible context.

• Consider using a boolean value: Instead of using strings like "--use_pcp" and "", consider using boolean values (true/false) for gl_pcp_state and config_info.pcp_state. This could simplify the logic and make it more readable. The Jinja2 template could then use a conditional to include the --use_pcp option only when the boolean value is true.

• Consider a more descriptive name: While pcp_state is concise, a more descriptive name like enable_pcp or use_performance_copilot might improve readability.

Revised Code Suggestion

Here's a revised snippet from bin/burden incorporating the suggestions:

--- a/bin/burden
+++ b/bin/burden
@@ -140,7 +140,7 @@ gl_spot_recover=1
 gl_cloud_placement=${value_not_set}
 gl_cloud_networks_sys="duplicate"
 gl_preflight=0
-gl_pcp_state="--use_pcp"
+gl_pcp_state="true"
 #
 # Global values
 #   gl_sys_file: sysctl files to use.
@@ -1930,7 +1930,7 @@ create_ansible_options()
 			echo "  test_exec_location: none"  >> ansible_vars_main.yml
 		fi
 
-		echo "  pcp_state: ${gl_pcp_state}" >> ansible_vars_main.yml
+		echo "  pcp_state: ${gl_pcp_state}" >> ansible_vars_main.yml

And the corresponding change in ansible_roles/roles/test_generic/tasks/main.yml:

--- a/ansible_roles/roles/test_generic/tasks/main.yml
+++ b/ansible_roles/roles/test_generic/tasks/

**Vote**: +1

**[PullHero](https://github.com/pullhero/)**

[github-actions]

This relates to RPOPC-683

[kdvalin]

Change option to --use_pcp simplifies down the number of options across Zathras and wrappers

[github-actions]

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the proposed changes. Here's my analysis:

Overview

This PR introduces a new command-line option, --use_pcp, to control whether Performance Co-Pilot (PCP) is used by the test wrappers. This allows users to disable PCP if it's not desired or causing issues. The changes involve modifying the burden script to handle the new option and passing it to the test wrappers through the test_generic Ansible role. The documentation has also been updated to reflect this new functionality.

Analysis

Code Quality and Maintainability

• The code changes are relatively small and focused, making them easy to understand.
• The implementation is straightforward and doesn't introduce unnecessary complexity.
• The gl_use_pcp variable is used consistently throughout the code.
• The addition of the --use_pcp option to the usage() function improves discoverability.

Potential Bugs or Security Issues

• There are no obvious security issues introduced by these changes.
• It is important to ensure that all test wrappers correctly handle the --use_pcp option. This PR itself doesn't guarantee that.

Adherence to Project Conventions

• The changes generally adhere to the project's conventions.
• The use of gl_ prefix for global variables is consistent.

Documentation Completeness

• The documentation has been updated to include the new --use_pcp option.
• The description of the option in docs/command_line_reference.md is clear and concise.

Specific Feedback

• Test Wrappers: It's crucial to verify that all existing test wrappers correctly handle the --use_pcp option, either by enabling/disabling PCP functionality or by simply ignoring the option without errors. This PR doesn't include modifications to test wrappers themselves. It would be beneficial to add a note to the documentation emphasizing this requirement.
• Default Value: The default value of gl_use_pcp is set to --use_pcp, implying PCP is enabled by default. This is fine, but it should be explicitly stated in the code comments and documentation.
• Error Handling: Consider adding error handling to the set_general_value() function in bin/burden to check if the value provided for --use_pcp is either 0 or 1. This would improve the robustness of the code.

Suggested Improvements

1. Add a note to the docs/command_line_reference.md and docs/test_config_files.md emphasizing that test wrappers must correctly handle the --use_pcp option.
2. Add a code comment in bin/burden explaining the default value of gl_use_pcp and its implication.
3. Implement input validation in set_general_value() to ensure the value of --use_pcp is either 0 or 1.

Conclusion

The changes are well-structured and achieve the intended functionality. Addressing the recommendations would improve the robustness and clarity of the changes.

Vote: +1

Vote: +1

PullHero

[kdvalin]

LGTM