feature(ArtifactsTest): IOTune parameters validation #10121
Conversation
|
Demo: |
k0machi
added
backport/2024.2
| @@ -325,6 +326,14 @@ def test_scylla_service(self): | |||
| with self.subTest("check ENA support"): | |||
| assert self.node.ena_support, "ENA support is not enabled" | |||
|
|
|||
| if ("gce" in backend or "aws" in backend or "azure" in backend): | |||
There was a problem hiding this comment.
is it going to work with e.g. k8s-local-kind-aws?
I'd propose to use if backend in ("gce", "aws", "azure")
Also, shouldn't it be used only with predefined images (if params.get('use_preinstalled_scylla')?
There was a problem hiding this comment.
Good catch, I was testing and removed use_preinstalled_scylla, will put it back
| diff = (val / preset_val - 1) * 100 | ||
| LOGGER.info("[%s] %s: %s (%.0f)", mountpoint, key, val, diff) | ||
|
|
||
| def _submit_results_to_argus(self): |
There was a problem hiding this comment.
this should be done outside of IOTuneValidator which should do only one thing: validating correctness of predefined io_properties against actual values: all things like submitting or printing in console could be done separately to not blur purpose of this class.
There was a problem hiding this comment.
okay, I'll move submission to the tester body.
sdcm/utils/validators/iotune.py
Outdated
| ValidationRules = { | ||
| "read_iops": ValidationRule(best_pct=10, fixed_limit=bottom_limit(preset_disk.get("read_iops"))), | ||
| "read_bandwidth": ValidationRule(best_pct=10, fixed_limit=bottom_limit(preset_disk.get("read_iops"))), | ||
| "write_iops": ValidationRule(best_pct=10, fixed_limit=bottom_limit(preset_disk.get("read_iops"))), | ||
| "write_bandwidth": ValidationRule(best_pct=10, fixed_limit=bottom_limit(preset_disk.get("read_iops"))), | ||
| } |
There was a problem hiding this comment.
I thing best_pct rule is not right.
I'd propose different approach: instead of sending raw metrics, send differences in absolute value. This way higher_is_better could be set to False and fixed limit (calculated by predefined io_properties.yaml metric * x%). This should work for any instance size.
In that case table name should be renamed to f"{self.params.get('cluster_backend')} - {self.node.db_node_instance_type} IO Tune absolute deviation"
Drawback is only when we need to know exact value we would need to look into logs (could be tackled by publishing error event with full details in that case)
There was a problem hiding this comment.
We can send both I think, and that's what I was thinking as well - I'll look into doing it that way.
| if tested_mountpoint != preset_disk["mountpoint"]: | ||
| LOGGER.warning("Disks differ - probably a mistake: %s vs %s, will not submit iotune results", | ||
| tested_mountpoint, preset_disk["mountpoint"]) | ||
| return |
There was a problem hiding this comment.
I think we shouldn't fail silently here. Better raise error event
k0machi
force-pushed
the
artifacts-iotune-params-verification
branch
from
597579b to
a527d67
Compare
This commits adds a new subtest inside Artifacts test, that does the check of machine image io_properties.yaml by comparing them to the actual machine values and showing the deviation Fixes scylladb/qa-tasks#1787
k0machi
force-pushed
the
artifacts-iotune-params-verification
branch
from
a527d67 to
791a3cc
Compare
This commits adds a new subtest inside Artifacts test, that does the
check of machine image io_properties.yaml by comparing them to the
actual machine values and showing the deviation. The results are both
printed to console with deviation value and submitted to argus as a
GenericResultTable.
Fixes scylladb/qa-tasks#1787
Testing
PR pre-checks (self review)
backportlabelsReminders
sdcm/sct_config.py)unit-test/folder)