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 1 commit
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
2 changes: 1 addition & 1 deletion lisa/microsoft/testsuites/cpu/cpusuite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_gb,
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,
Comment thread
SRIKKANTH marked this conversation as resolved.
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,
Comment on lines 89 to 100
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.

This PR is labeled as a bug fix, but the PR description lists no related issue. Consider linking the related issue (or a tracking work item) for traceability, especially since this changes a public API keyword argument on Fio.launch/launch_async.

Copilot uses AI. Check for mistakes.
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.
Comment on lines -89 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.

Renaming the public Fio.launch(...)/launch_async(...)/_get_command(...) parameter from size_gb to size_mb is a breaking change for any out-of-tree callers using the old keyword. If backward compatibility is desired, consider temporarily supporting both names (e.g., keep size_gb as a deprecated alias that maps to size_mb with a warning) and remove it in a later major release.

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