feat(tests): add disk performance benchmark tests#324
feat(tests): add disk performance benchmark tests#324Millefeuille42 wants to merge 10 commits intomasterfrom
Conversation
Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
c89c4e6 to
385f290
Compare
|
@Nambrok @AnthoineB I added usage of the SR, VM, VBD, etc. fixtures. For now it's only using a local SR but I could make it use a specific SR (shared or local). I still need to add some stuff like making all parameters editable through the config file / flags. Feel free to add feedback on the current state of the PR if needed |
539c38d to
68ecbbc
Compare
|
Please squash some of the |
- fix(disk_perf): add configurable numjob - fix(disk_perf): styling and readability - misc(disk_perf): simplify fixtures - misc(disk_perf): styling - fix(disk_perf): usage of double quote in nested string Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
- style(disk_perf): sort imports and minor style fixes Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
- fix(disk_perf): csv paths and add logging Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
68ecbbc to
efa4b26
Compare
Done |
jobs.py
Outdated
| "params": { | ||
| "--vm": "single/small_vm", | ||
| }, | ||
| "paths": ["tests/storage"], |
There was a problem hiding this comment.
path should point to benchmark subdirectory directly in the job
| "paths": ["tests/storage"], | |
| "paths": ["tests/storage/benchmarks"], |
tests/storage/benchmarks/conftest.py
Outdated
|
|
||
|
|
||
| # use vhd, qcow2, raw... when image_format support will be available | ||
| @pytest.fixture(scope="module", params=["vdi"]) |
There was a problem hiding this comment.
The temporary one could have been vhd ^^
tests/storage/benchmarks/conftest.py
Outdated
| ("command -v apk", "apk add fio", "apk del fio"), | ||
| ) | ||
|
|
||
| for check_cmd, install_cmd, remove in install_cmds: |
There was a problem hiding this comment.
There is already code allowing to install things on a VM.
You have for example lib/vm.py:VM.detect_package_manager() that exist.
You can see example of its usage in tests/guest_tools/unix/test_guest_tools_unix.py.
To uninstall, you can make a snapshot before installing your package and revert at the end to have the VM reset to its initial state (There might be a fixture already doing this).
tests/storage/benchmarks/conftest.py
Outdated
| @pytest.fixture(scope="module") | ||
| def vdi_on_local_sr(host, local_sr_on_hostA1, image_format): | ||
| sr = local_sr_on_hostA1 | ||
| vdi = sr.create_vdi("testVDI", MAX_LENGTH) |
There was a problem hiding this comment.
The image_format has to be given to create_vdi
| vdi = sr.create_vdi("testVDI", MAX_LENGTH) | |
| vdi = sr.create_vdi("testVDI", MAX_LENGTH, image_format=image_format) |
There was a problem hiding this comment.
Image format is not supported yet, resulting in an error. I will provide it as a comment though.
tests/storage/benchmarks/conftest.py
Outdated
| def vdi_on_local_sr(host, local_sr_on_hostA1, image_format): | ||
| sr = local_sr_on_hostA1 | ||
| vdi = sr.create_vdi("testVDI", MAX_LENGTH) | ||
| vdi.image_format = image_format |
There was a problem hiding this comment.
| vdi.image_format = image_format |
- change job path - set "vhd" as the placeholder image_format - remove leftover code Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
…rt install Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
tests/storage/benchmarks/conftest.py
Outdated
| "--file-sizes", | ||
| action="store", | ||
| type=lambda value: str_to_tuple(value, sep=","), | ||
| default=("1G", "4G", f"{int((system_memory // (1024.**3)) * 2)}G"), |
There was a problem hiding this comment.
Small comment
| default=("1G", "4G", f"{int((system_memory // (1024.**3)) * 2)}G"), | |
| default=("1G", "4G", f"{int((system_memory // (1024.**3)) * 2)}G"), # (2*Memory) GiB |
| vm = running_unix_vm_with_fio | ||
| vbd = plugged_vbd | ||
| device = f"/dev/{vbd.param_get(param_name='device')}" | ||
| test_type = "{}-{}-{}-{}".format(block_size, file_size, rw_mode, image_format) |
There was a problem hiding this comment.
Do we want to add a prefix here, like "bench-fio-", to have a common part of the name. If there is other tests we can have something like "bench-newtool-".
There was a problem hiding this comment.
I know that adding more in the test_type could look heavy but shouldn't we add iodepth and numjob in the test_type string?
There was a problem hiding this comment.
We should add the bench- prefix indeed as well as the numjob and iodepth. I forgot to add the latter part when I added those options. I will fix that.
| logging.info("Performance difference summary:") | ||
| for k, v in diffs.items(): | ||
| sign = "+" if v < 0 else "-" | ||
| logging.info(f"- {k}: {sign}{abs(v):.2f}%") |
There was a problem hiding this comment.
It may be interesting to have a condition that log a special message if the performance are above a threshold to signal a perf improvement.
There was a problem hiding this comment.
That could be interesting. Both threshold (for failure and future positive message) could be changed through flags too
Co-authored-by: Anthoine <anthoine.bourgeois@vates.tech> Signed-off-by: Millefeuille <millefeuille42@proton.me>
- add improvement threshold - adjust wording in messages - add additional information in test_type - fix percentage calculation - styling Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
tests/storage/benchmarks/conftest.py
Outdated
| MAX_LENGTH = 64 * (1024**3) # 64GiB | ||
|
|
||
|
|
||
| # use vhd, qcow2, raw... when image_format support will be available |
There was a problem hiding this comment.
Should have a TODO: label in this case, otherwise "will" is incorrect. Otherwise we can block this PR as long as the desired code is not merged I suppose.
There was a problem hiding this comment.
As for blocking the PR, idk since the support of image formats is not required to make it work for now. If we don't block this PR we should be cautious to not loose track of this TODO (maybe make a card about this?)
There was a problem hiding this comment.
Afaik, qcow2 is now available for testing. Is it hard to also include it here ? Otherwise, I'm fine with a todo.
There was a problem hiding this comment.
I will add it
Signed-off-by: Mathieu Labourier <mathieu.labourier@vates.tech>
glehmann
left a comment
There was a problem hiding this comment.
That's a very good start for performance monitoring.
Later, we'll probably need something more accessible than a local file to store the results, to be able to run from multiple hosts and raise alerts based on more data than just the previous one.
| def pytest_addoption(parser): | ||
| system_memory = os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES") | ||
|
|
||
| parser.addoption( |
There was a problem hiding this comment.
IMO, instead of adding options to pytest, we should have multiple tests with predefined values that we can run in our CI.
No description provided.