CA-400124 - rrd: Serialize transform parameter for data sources #6043
Conversation
|
Will test tomorrow to see if this was indeed the cause of currently misreported CPU metrics and if I haven't missed anything else. |
| type ds_transform_function = Inverse | Identity | ||
|
|
||
| let apply_transform_function f x = | ||
| match f with Inverse -> 1.0 -. x | Identity -> x |
There was a problem hiding this comment.
This works with the current sources, we might want to use max instead of 1.0 (but that has problems when the max is infinite, so we need more considerations here)
It's probably a better idea to change how xen provides stats to provide busy time instead, or the plugin report idle time. I'd prefer the former
There was a problem hiding this comment.
Or we just transform the data in the new plugin, where we collect the data from xen? Then we don't need complicated generic transform functionality.
There was a problem hiding this comment.
Or we just transform the data in the new plugin, where we collect the data from xen? Then we don't need complicated generic transform functionality.
This is worth trying, but it needs precise tracking of time to calculate the difference between the previous read and the current one to be able to calculate the busy time, from the idle time in the current period.
There was a problem hiding this comment.
I agree that this is an awkward workaround (even though it's implementing the actual specified parameters), just increasing the size of the memory-mapped files (I'm not sure if we could omit transform: Identity, taking it as the default, for example, to optimize this). Once I get the timestamps of the plugin to be reliably taken into account, I can drop this and move the transform logic into the plugins.
For now, to accommodate taking inverse from different values, perhaps the variant could take a parameter it would take the inverse from: Inverse x?
last-genius
force-pushed
the
private/asultanov/rrdd-fix
branch
from
2d948ff to
d7e679f
Compare
last-genius
changed the title
|
This passes BST+BVT, and also shows sensible CPU load metrics, compare xentop's output (5.3% in the screenshot) with the running rrd2csv percentages (out of bounds values are a different, larger issue): 
Percentages go up to 100% under full load too. |
| exception Invalid_data_source of string | ||
|
|
||
| (** Inverse is (fun x -> 1.0 - x) *) | ||
| type ds_transform_function = Inverse | Identity |
There was a problem hiding this comment.
How about
type transform = Inv of float -> float | Id of float -> float
that is, make the actual transformation part of the type?
There was a problem hiding this comment.
I'd be wary of adding more information in the pages, we're filling the memory pages, for not a big benefit. I'm even thinking whether serializing it as a number to reduce the amount of bytes added would be worth it (0 being Identity, 1 the inverse)
There was a problem hiding this comment.
then it'd need to be serialized. the intention is to make this as simple as possible to transfer between plugin and server
|
This adds data to the memory-mapped files, can you check that it's not causing problems by running a boot storm or configuration limits test suite? |
|
Running the config limits test suite, but also pushed an improvement where default |
|
|
|
You're right, we recently updated the uuidm library, will take a look at it |
|
Fix is in #6045 |
|
Tested the optimization, works as intended, cpu metrics are still correctly reported and: |
Previously, the transform function was not serialized in the plugin-server protocol (it was only used in xcp_rrdd itself, not in plugins). This issue was revealed by the previous work around splitting cpu metrics into a separate plugin. Instead of allowing arbitrary functions (which would be difficult to serialize), for 'fun x' offer just two options: * Inverse (1.0 - x), and * Identity (x) Default (if the parameter is not provided, either in OCaml or JSON), is Identity. Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
This saves on space, since only cpuX and cpu_avg datasources currently have transform=inverse, all the others have the default identity function specified. Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
last-genius
force-pushed
the
private/asultanov/rrdd-fix
branch
from
b04e5b5 to
fe79b0f
Compare
|
Rebased on top of master to get the fix for CI failure |
|
config limits test suite passed too, even without the optimization. |
The tests will pass even if there are failing metrics, could you check those in the logs? (They might log to daemon.log rather than xenserver.log) |
All the rrdd plugin daemons seem to have started and not produced any errors (Run #206094) |
4 participants
Previously, the transform function was not serialized in the plugin-server protocol (it was only used in xcp_rrdd itself, not in plugins). This issue was revealed by the previous work around splitting cpu metrics into a separate plugin.
Instead of allowing arbitrary functions (which would be difficult to serialize), for 'fun x' offer just two options:
Default (if the parameter is not provided, either in OCaml or JSON), is Identity.