From 686751fbfc423b1c5ab9e9797b83387cad79fd15 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Tue, 8 Oct 2024 08:22:47 +0100 Subject: [PATCH 1/2] CA-400124: rrd: Serialize transform parameter for data sources 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 --- ocaml/libs/xapi-rrd/lib/rrd.ml | 18 +++++++++++++++--- ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml | 2 +- ocaml/libs/xapi-rrd/lib_test/unit_tests.ml | 10 +++++----- ocaml/xapi-idl/rrd/ds.ml | 4 ++-- ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml | 8 -------- ocaml/xcp-rrdd/bin/rrdp-cpu/rrdp_cpu.ml | 7 ++----- ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml | 10 ++++++++++ .../lib/transport/base/rrd_protocol_v2.ml | 7 ++++++- ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.ml | 10 ++++++++++ ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.mli | 2 ++ ocaml/xenopsd/xc/mem_stats.ml | 3 ++- 11 files changed, 55 insertions(+), 26 deletions(-) diff --git a/ocaml/libs/xapi-rrd/lib/rrd.ml b/ocaml/libs/xapi-rrd/lib/rrd.ml index 3c2f8d707a8..0b67cc9efc5 100644 --- a/ocaml/libs/xapi-rrd/lib/rrd.ml +++ b/ocaml/libs/xapi-rrd/lib/rrd.ml @@ -22,6 +22,12 @@ exception No_RRA_Available exception Invalid_data_source of string +(** Inverse is (fun x -> 1.0 - x) *) +type ds_transform_function = Inverse | Identity + +let apply_transform_function f x = + match f with Inverse -> 1.0 -. x | Identity -> x + type ds_owner = VM of string | Host | SR of string (** Data source types - see ds datatype *) @@ -84,6 +90,12 @@ let ds_value_to_string = function | _ -> "0.0" +let ds_transform_function_to_string = function + | Inverse -> + "inverse" + | Identity -> + "identity" + (** The CDP preparation scratch area. The 'value' field should be accumulated in such a way that it always contains the value that will eventually be the CDP. This means that @@ -417,7 +429,7 @@ let ds_update rrd timestamp values transforms new_domid = ) in (* Apply the transform after the raw value has been calculated *) - let raw = transforms.(i) raw in + let raw = apply_transform_function transforms.(i) raw in (* Make sure the values are not out of bounds after all the processing *) if raw < ds.ds_min || raw > ds.ds_max then nan @@ -450,7 +462,7 @@ let ds_update_named rrd timestamp ~new_domid valuesandtransforms = valuesandtransforms |> List.to_seq |> StringMap.of_seq in let get_value_and_transform {ds_name; _} = - Option.value ~default:(VT_Unknown, Fun.id) + Option.value ~default:(VT_Unknown, Identity) (StringMap.find_opt ds_name valuesandtransforms) in let ds_values, ds_transforms = @@ -519,7 +531,7 @@ let rrd_create dss rras timestep inittime = } in let values = Array.map (fun ds -> ds.ds_last) dss in - let transforms = Array.make (Array.length values) (fun x -> x) in + let transforms = Array.make (Array.length values) Identity in ds_update rrd inittime values transforms true ; rrd diff --git a/ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml b/ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml index d3f01762d29..6ff917eccfc 100644 --- a/ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml +++ b/ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml @@ -81,7 +81,7 @@ let rrd = List.iteri (fun i v -> let t = 5. *. (init_time +. float_of_int i) in - ds_update rrd t [|VT_Int64 v|] [|Fun.id|] (i = 0) + ds_update rrd t [|VT_Int64 v|] [|Identity|] (i = 0) ) values ; rrd diff --git a/ocaml/libs/xapi-rrd/lib_test/unit_tests.ml b/ocaml/libs/xapi-rrd/lib_test/unit_tests.ml index b48ebf17688..089d8047468 100644 --- a/ocaml/libs/xapi-rrd/lib_test/unit_tests.ml +++ b/ocaml/libs/xapi-rrd/lib_test/unit_tests.ml @@ -131,7 +131,7 @@ let gauge_rrd = let rrd = rrd_create [|ds; ds2; ds3; ds4|] [|rra; rra2; rra3; rra4|] 1L 1000000000.0 in - let id x = x in + let id = Identity in for i = 1 to 100000 do let t = 1000000000.0 +. (0.7 *. float_of_int i) in let v1 = VT_Float (0.5 +. (0.5 *. sin (t /. 10.0))) in @@ -159,7 +159,7 @@ let _deserialize_verify_rrd = let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L init_time in - let id x = x in + let id = Identity in for i = 1 to 100 do let t = init_time +. float_of_int i in let t64 = Int64.of_float t in @@ -178,7 +178,7 @@ let ca_322008_rrd = let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L init_time in - let id x = x in + let id = Identity in for i = 1 to 100000 do let t = init_time +. float_of_int i in @@ -198,7 +198,7 @@ let ca_329043_rrd_1 = let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L init_time in - let id x = x in + let id = Identity in let time_value_of_i i = let t = 5. *. (init_time +. float_of_int i) in @@ -228,7 +228,7 @@ let create_rrd ?(rows = 2) values min max = rrd_create [|ds1; ds2; ds3|] [|rra1; rra2; rra3; rra4|] 5L init_time in - let id x = x in + let id = Identity in List.iteri (fun i v -> diff --git a/ocaml/xapi-idl/rrd/ds.ml b/ocaml/xapi-idl/rrd/ds.ml index 620ba3fcc0c..0aef7dd5884 100644 --- a/ocaml/xapi-idl/rrd/ds.ml +++ b/ocaml/xapi-idl/rrd/ds.ml @@ -25,11 +25,11 @@ type ds = { ; ds_min: float ; ds_max: float ; ds_units: string - ; ds_pdp_transform_function: float -> float + ; ds_pdp_transform_function: Rrd.ds_transform_function } let ds_make ~name ~description ~value ~ty ~default ~units ?(min = neg_infinity) - ?(max = infinity) ?(transform = fun x -> x) () = + ?(max = infinity) ?(transform = Rrd.Identity) () = { ds_name= name ; ds_description= description diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml index 5d445e0f7dc..34a44e92dfe 100644 --- a/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml +++ b/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml @@ -77,14 +77,6 @@ module OwnerMap = Map.Make (struct String.compare a b end) -let owner_to_string () = function - | Host -> - "host" - | VM uuid -> - "VM " ^ uuid - | SR uuid -> - "SR " ^ uuid - (** Updates all of the hosts rrds. We are passed a list of uuids that is used as the primary source for which VMs are resident on us. When a new uuid turns up that we haven't got an RRD for in our hashtbl, we create a new one. When diff --git a/ocaml/xcp-rrdd/bin/rrdp-cpu/rrdp_cpu.ml b/ocaml/xcp-rrdd/bin/rrdp-cpu/rrdp_cpu.ml index 8faf484f2b0..8b56119a76e 100644 --- a/ocaml/xcp-rrdd/bin/rrdp-cpu/rrdp_cpu.ml +++ b/ocaml/xcp-rrdd/bin/rrdp-cpu/rrdp_cpu.ml @@ -142,8 +142,7 @@ let dss_pcpus xc = ~description:("Physical cpu usage for cpu " ^ string_of_int i) ~value:(Rrd.VT_Float (Int64.to_float v /. 1.0e9)) ~min:0.0 ~max:1.0 ~ty:Rrd.Derive ~default:true - ~transform:(fun x -> 1.0 -. x) - () + ~transform:Rrd.Inverse () ) :: acc , i + 1 @@ -158,9 +157,7 @@ let dss_pcpus xc = , Ds.ds_make ~name:"cpu_avg" ~units:"(fraction)" ~description:"Average physical cpu usage" ~value:(Rrd.VT_Float (avg_array /. 1.0e9)) - ~min:0.0 ~max:1.0 ~ty:Rrd.Derive ~default:true - ~transform:(fun x -> 1.0 -. x) - () + ~min:0.0 ~max:1.0 ~ty:Rrd.Derive ~default:true ~transform:Rrd.Inverse () ) in avgcpu_ds :: dss diff --git a/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml b/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml index 1b0f531383e..b6d10953723 100644 --- a/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml +++ b/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml @@ -44,6 +44,15 @@ let ds_owner x = string "sr %s" sr ) +let ds_transform x = + ( "transform" + , match x with + | Rrd.Identity -> + string "identity" + | Rrd.Inverse -> + string "inverse" + ) + let bool b = string "%b" b (* Should use `Bool b *) let float x = string "%.2f" x @@ -63,6 +72,7 @@ let ds_to_json (owner, ds) = [ description ds.Ds.ds_description ; [ds_owner owner] + ; [ds_transform ds.Ds.ds_pdp_transform_function] ; ds_value ds.Ds.ds_value ; [ds_type ds.Ds.ds_type] ; [ diff --git a/ocaml/xcp-rrdd/lib/transport/base/rrd_protocol_v2.ml b/ocaml/xcp-rrdd/lib/transport/base/rrd_protocol_v2.ml index 1dc6f2d25dc..1c6774d525a 100644 --- a/ocaml/xcp-rrdd/lib/transport/base/rrd_protocol_v2.ml +++ b/ocaml/xcp-rrdd/lib/transport/base/rrd_protocol_v2.ml @@ -193,8 +193,13 @@ let uninitialised_ds_of_rpc ((name, rpc) : string * Rpc.t) : let default = bool_of_string (Rrd_rpc.assoc_opt ~key:"default" ~default:"false" kvs) in + let transform = + Rrd_rpc.transform_of_string + (Rrd_rpc.assoc_opt ~key:"transform" ~default:"identity" kvs) + in let ds = - Ds.ds_make ~name ~description ~units ~ty ~value ~min ~max ~default () + Ds.ds_make ~name ~description ~units ~ty ~value ~min ~max ~default + ~transform () in (owner, ds) diff --git a/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.ml b/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.ml index b8b1db7de2c..36ba1b42e59 100644 --- a/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.ml +++ b/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.ml @@ -53,3 +53,13 @@ let owner_of_string (s : string) : Rrd.ds_owner = Rrd.SR uuid | _ -> raise Rrd_protocol.Invalid_payload + +(* Converts a string to value of ds_transform_function type. *) +let transform_of_string (s : string) : Rrd.ds_transform_function = + match s with + | "inverse" -> + Rrd.Inverse + | "identity" -> + Rrd.Identity + | _ -> + raise Rrd_protocol.Invalid_payload diff --git a/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.mli b/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.mli index 8863b65bf4f..0d7d7493ead 100644 --- a/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.mli +++ b/ocaml/xcp-rrdd/lib/transport/base/rrd_rpc.mli @@ -21,3 +21,5 @@ val assoc_opt : key:string -> default:string -> (string * Rpc.t) list -> string val ds_ty_of_string : string -> Rrd.ds_type val owner_of_string : string -> Rrd.ds_owner + +val transform_of_string : string -> Rrd.ds_transform_function diff --git a/ocaml/xenopsd/xc/mem_stats.ml b/ocaml/xenopsd/xc/mem_stats.ml index 9e01d14473e..8daca47aff6 100644 --- a/ocaml/xenopsd/xc/mem_stats.ml +++ b/ocaml/xenopsd/xc/mem_stats.ml @@ -291,7 +291,8 @@ let observe_stats l = | Rrd.VT_Unknown -> nan in - ds.Ds.ds_pdp_transform_function f |> Printf.sprintf "%.0f" + Rrd.apply_transform_function ds.Ds.ds_pdp_transform_function f + |> Printf.sprintf "%.0f" ) in D.debug "stats header: %s" (String.concat "," names) ; From fe79b0f40dd1efb913a2153524d5c204a70b2e96 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Tue, 8 Oct 2024 11:25:06 +0100 Subject: [PATCH 2/2] CA-400124 - rrdd: only serialize transform when it's not default 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 --- ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml b/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml index b6d10953723..15f95e3de46 100644 --- a/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml +++ b/ocaml/xcp-rrdd/lib/transport/base/rrd_json.ml @@ -45,13 +45,13 @@ let ds_owner x = ) let ds_transform x = - ( "transform" - , match x with - | Rrd.Identity -> - string "identity" - | Rrd.Inverse -> - string "inverse" - ) + match x with + | Rrd.Identity -> + [] + (* This is the default when transform is absent, and not including it + makes the file smaller *) + | Rrd.Inverse -> + [("transform", string "inverse")] let bool b = string "%b" b (* Should use `Bool b *) @@ -72,7 +72,7 @@ let ds_to_json (owner, ds) = [ description ds.Ds.ds_description ; [ds_owner owner] - ; [ds_transform ds.Ds.ds_pdp_transform_function] + ; ds_transform ds.Ds.ds_pdp_transform_function ; ds_value ds.Ds.ds_value ; [ds_type ds.Ds.ds_type] ; [