Skip to content

Conversation

@szmijews
Copy link

@szmijews szmijews commented Dec 2, 2025

Introduce methods for measuring CPU usage with esxtop tool + unit tests
fixes #3

Copilot AI review requested due to automatic review settings December 2, 2025 14:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces CPU measurement functionality for ESXi hosts using the esxtop tool. The implementation allows starting a CPU measurement process, collecting data over time, and parsing the results to calculate average CPU usage for specific VMs.

Key Changes:

  • Added methods to start/stop CPU measurement processes using esxtop command
  • Implemented parsing logic to extract and average CPU usage data from the last 4 samples
  • Added comprehensive unit tests covering normal operation, edge cases, and error handling

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
mfd_host/feature/cpu/esxi.py Implements three new methods for CPU measurement: start, stop, and parse operations with error handling and process management
tests/unit/test_mfd_host/test_feature/test_cpu/test_esxi.py Adds 8 unit tests covering measurement lifecycle, parsing logic, error conditions, and edge cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +116 to +121
command = (
f"cut -d, -f`awk -F, '{{for (i=1;i<=NF;i++){{if ($i ~/Group Cpu.*{vm_name}).*Used/) "
f"{{print i}}}}}}' {process.log_path}` {process.log_path}>{parsed_file_path}"
)
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The vm_name parameter is directly interpolated into a shell command without sanitization, creating a command injection vulnerability. Validate or escape vm_name before using it in the command, or use a safer approach to construct the command.

Copilot uses AI. Check for mistakes.
@szmijews szmijews force-pushed the cpu_usage_per_vm branch 2 times, most recently from c50ac5b to 90776eb Compare December 2, 2025 14:06
@szmijews szmijews requested a review from Copilot December 2, 2025 14:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

f"cut -d, -f`awk -F, '{{for (i=1;i<=NF;i++){{if ($i ~/Group Cpu.*{vm_name}).*Used/) "
f"{{print i}}}}}}' {process.log_path}` {process.log_path}>{parsed_file_path}"
)
self._connection.execute_command(command=command, shell=True)
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Using shell=True with command string interpolation can create command injection vulnerabilities. The vm_name parameter is directly interpolated into the command without sanitization. Consider validating/sanitizing vm_name or using a safer approach that doesn't rely on shell=True with user-provided input.

Copilot uses AI. Check for mistakes.
@szmijews szmijews requested a review from Copilot December 2, 2025 14:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

:param process: Process handle
:return: average CPU usage perventage
"""
parsed_file_path = "/tmp/parsed_output.txt"
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The hardcoded path '/tmp/parsed_output.txt' creates a security risk as it could be exploited through race conditions or predictable file paths. Consider using tempfile.NamedTemporaryFile() or generating unique filenames with timestamps or UUIDs to prevent potential file overwrites or unauthorized access.

Suggested change
parsed_file_path = "/tmp/parsed_output.txt"
# Use remote tempfile to generate a unique temporary file path
remote_tempfile = self._connection.modules().tempfile
temp_file = remote_tempfile.NamedTemporaryFile(delete=False)
parsed_file_path = temp_file.name
temp_file.close()

Copilot uses AI. Check for mistakes.
Comment on lines +119 to +124
self._connection.execute_command(command=command, shell=True)
p = self._connection.path(process.log_path)
p.unlink()
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The original log file is deleted before attempting to read the parsed output. If the execute_command fails or the parsed file is not created, there's no way to debug or recover. Consider deleting the original log file after successfully reading the parsed output, or handle the case where the parsed file might not exist.

Copilot uses AI. Check for mistakes.
Introduce methods for measuring CPU usage with esxtop tool + unit tests

Signed-off-by: szmijews <szymon.zmijewski@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ESXi] CPU usage measurement

1 participant