Skip to content

Fixing incorrect variable name 'size_gb' which actually represents 'size_mb' #4409

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.

Sign up for GitHub

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

Draft
SRIKKANTH wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fixing incorrect variable name 'size_gb' which actually represents 'size_mb' #4409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34396b8
Fixing incorrect variable name 'size_gb' which actually represents 's…
SRIKKANTH Apr 5, 2026
eaab109
Update cpusuite.py
SRIKKANTH Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lisa/microsoft/testsuites/cpu/cpusuite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ def verify_cpu_hot_plug(self, log: Logger, node: Node) -> None:
)
def verify_cpu_offline_storage_workload(self, log: Logger, node: Node) -> None:
# run fio process asynchronously on the node
fio_data_size_in_gb = 1
fio_data_size_in_mb = 1
try:
image_folder_path = node.find_partition_with_freespace(fio_data_size_in_gb)
image_folder_path = node.find_partition_with_freespace(fio_data_size_in_mb)
Copy link
Copy Markdown
Collaborator

@LiliDeng LiliDeng Apr 10, 2026

Choose a reason for hiding this comment

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

the unit of this method find_partition_with_freespace is gb

# Each CPU takes ~10 seconds to toggle offline-online
Comment on lines +101 to 104
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

node.find_partition_with_freespace takes size_in_gb, but this call now passes fio_data_size_in_mb. That makes the unit semantics incorrect (the value will be treated as GiB), and could cause unexpected failures or over-strict disk selection if fio_data_size_in_mb is adjusted. Consider keeping a separate required_space_gb variable (or converting MB→GB) when calling find_partition_with_freespace.

Copilot uses AI. Check for mistakes.
fio_run_time = 300 + (node.tools[Lscpu].get_thread_count() * 10)
fio_process = node.tools[Fio].launch_async(
Comment on lines +101 to 106
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

The constants fio_data_size_in_mb = 1 and the base 300 seconds in fio_run_time = 300 + (...) are magic numbers that control test behavior but aren’t explained inline. Please add brief rationale/comments (or named constants) so future changes don’t accidentally skew workload size or runtime assumptions.

Copilot generated this review using guidance from repository custom instructions.
Expand All @@ -111,7 +111,7 @@ def verify_cpu_offline_storage_workload(self, log: Logger, node: Node) -> None:
numjob=10,
time=fio_run_time,
block_size="1M",
size_gb=fio_data_size_in_gb,
size_mb=fio_data_size_in_mb,
group_reporting=False,
overwrite=True,
time_based=True,
Expand Down
2 changes: 1 addition & 1 deletion lisa/microsoft/testsuites/performance/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def perf_disk(
filename=filename,
mode=mode.name,
time=time,
size_gb=size_mb,
size_mb=size_mb,
block_size=f"{block_size}K",
iodepth=iodepth,
overwrite=overwrite,
Expand Down
2 changes: 1 addition & 1 deletion lisa/microsoft/testsuites/power/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ def run_storage_workload(node: Node) -> Decimal:
time=120,
block_size="1M",
overwrite=True,
size_gb=1,
size_mb=1,
)
return fio_result.iops

Expand Down
2 changes: 1 addition & 1 deletion lisa/microsoft/testsuites/storage/storagesuite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def verify_disk_with_fio_verify_option(self, node: Node) -> None:
numjob=0,
time=0,
block_size="",
size_gb=100,
size_mb=100,
group_reporting=False,
do_verify=True,
Comment on lines 154 to 158
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

verify_disk_with_fio_verify_option runs fio in a for _ in range(100) loop with time=0 (no --runtime) and size_mb=100 * 1024, so each iteration will attempt to write+verify ~100 GiB and the loop can generate ~10 TiB of IO. This is likely to make the functional test impractically slow and/or hit CI timeouts. Consider keeping the previous effective size (100 MiB), or switching to time_based=True with an explicit runtime, or reducing the iteration count/size with a clear rationale. Also the inline comment says “100GB” but the expression is 100*1024 MiB (GiB).

Copilot uses AI. Check for mistakes.
bsrange="512-256K",
Expand Down
14 changes: 7 additions & 7 deletions lisa/tools/fio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def launch(
time: int = 120,
ssh_timeout: int = 6400,
block_size: str = "4K",
size_gb: int = 0,
size_mb: int = 0,
direct: bool = True,
gtod_reduce: bool = False,
Comment on lines 96 to 101
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

PR hygiene: the PR is marked as a bug fix but the description lists no related issue. Consider linking an issue (or explicitly stating why none exists) for traceability.

Copilot uses AI. Check for mistakes.
group_reporting: bool = True,
Expand All @@ -118,7 +118,7 @@ def launch(
numjob,
time,
block_size,
size_gb,
size_mb,
direct,
gtod_reduce,
group_reporting,
Expand Down Expand Up @@ -154,7 +154,7 @@ def launch_async(
numjob: int,
time: int = 120,
block_size: str = "4K",
size_gb: int = 0,
size_mb: int = 0,
direct: bool = True,
gtod_reduce: bool = False,
group_reporting: bool = True,
Expand All @@ -176,7 +176,7 @@ def launch_async(
numjob,
time,
block_size,
size_gb,
size_mb,
direct,
gtod_reduce,
group_reporting,
Expand Down Expand Up @@ -326,7 +326,7 @@ def _get_command( # noqa: C901
numjob: int = 0,
time: int = 120,
block_size: str = "4K",
size_gb: int = 0,
size_mb: int = 0,
direct: bool = True,
gtod_reduce: bool = False,
group_reporting: bool = True,
Expand Down Expand Up @@ -358,8 +358,8 @@ def _get_command( # noqa: C901
cmd += " --direct=1"
if gtod_reduce:
cmd += " --gtod_reduce=1"
if size_gb:
cmd += f" --size={size_gb}M"
if size_mb:
cmd += f" --size={size_mb}M"
Copy link
Copy Markdown
Collaborator

@LiliDeng LiliDeng Apr 7, 2026

Choose a reason for hiding this comment

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

just change this into cmd += f" --size={size_gb}G" should be enough.

Copy link
Copy Markdown
Collaborator Author

@SRIKKANTH SRIKKANTH Apr 7, 2026

Choose a reason for hiding this comment

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

If we do this all fio tests will run against GB ( * 1024 ) of current values. Which will result in timeouts as the time required for fio file size in GBs will be more than MBs.
For example some tests use size_gb=8192 which will be 8TB instead of current test file size 8GB.
One of the NFS testcase uses 100MB now in a loop of 100 iteration. With this suggestion it will become 100 GB * 100 = 10TB data reads and writes.

Copy link
Copy Markdown
Collaborator

@LiliDeng LiliDeng Apr 8, 2026

Choose a reason for hiding this comment

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

Please check the original case parameters
image

Copy link
Copy Markdown
Collaborator Author

@SRIKKANTH SRIKKANTH Apr 8, 2026

Choose a reason for hiding this comment

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

"perf_disk()" accepts the value as 'size_mb' but passing the value to "fio.launch()" as 'size_gb'.
Please check: https://github.com/microsoft/lisa/blob/main/lisa/microsoft/testsuites/performance/common.py#L159

This is a typo issue not a test case issue.

Copy link
Copy Markdown
Collaborator Author

@SRIKKANTH SRIKKANTH Apr 8, 2026

Choose a reason for hiding this comment

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

If you want to run current fio tests against 1023GB like we had in lisav2, I can raise a different PR for that after through testing. This is for just fixing the typo which impacts testcases not just disk perf tests but CPU and Power test cases as well.

Copy link
Copy Markdown
Collaborator

@LiliDeng LiliDeng Apr 10, 2026

Choose a reason for hiding this comment

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

@SRIKKANTH thanks for fixing this, but I still would like to just change cmd += f" --size={size_gb}M" into cmd += f" --size={size_gb}G"

Copy link
Copy Markdown
Collaborator Author

@SRIKKANTH SRIKKANTH Apr 14, 2026

Choose a reason for hiding this comment

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

Let me put the PR in draft until I finish the validation with you suggestion.

if group_reporting:
cmd += " --group_reporting"
if overwrite:
Expand Down
Loading