Skip to content

CA-408126 - rrd: Do not lose ds_min/max when adding to the RRD #6349

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

Merged
edwintorok merged 1 commit into from
Mar 10, 2025
Merged

CA-408126 - rrd: Do not lose ds_min/max when adding to the RRD #6349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
CA-408126 - rrd: Do not lose ds_min/max when adding to the RRD 
Rrd.ds_create has optional min and max arguments (defaulting to neg_infinity
and infinity respectively). Several callers would omit these parameters,
resulting in ds_min and ds_max being lost during the conversion from Ds.ds to
Rrd.ds. Without these, metrics couldn't be kept in range, which would result in
some (such as CPU usage numbers) going negative when a domain would change
its domid (over a reboot), for example.

Make these parameters (alobg with mrhb) required, not optional. Requires
adjusting unit tests as well.

This latent behaviour was exposed during the major timestamp and plugin
refactoring last year.
Previously, the entire RRD was created at once by calling create_fresh_rrd. Now
create_fresh_rrd is only called for the first chunk, and other chunks of the
RRD call merge_new_dss, which omitted the optional arguments.
Rrdd_server.add_ds also ommitted these arguments, which meant that datasources
enabled at runtime would not be kept in range.

Signed-off-by: Andrii Sultanov <[email protected]>
  • Loading branch information
Andrii Sultanov committed Mar 10, 2025
commit 71f8d16dd9695659f500878d3813a52b8af9c58a
3 changes: 1 addition & 2 deletions ocaml/libs/xapi-rrd/lib/rrd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,8 +562,7 @@ let rra_create cf row_cnt pdp_cnt xff =
(* defer creation of the data until we know how many dss we're storing *)
}

let ds_create name ty ?(min = neg_infinity) ?(max = infinity) ?(mrhb = infinity)
init =
let ds_create name ty ~min ~max ~mrhb init =
{
ds_name= name
; ds_ty= ty
Expand Down
2 changes: 1 addition & 1 deletion ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ let ds =
Cb.(
map [ds_value; float; float; ds_type] (fun v x y typ ->
let min, max = castd2s x y in
ds_create (ds_type_to_string typ) ~min ~max typ v
ds_create (ds_type_to_string typ) ~min ~max ~mrhb:infinity typ v
)
)

Expand Down
87 changes: 64 additions & 23 deletions ocaml/libs/xapi-rrd/lib_test/unit_tests.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,18 @@ let absolute_rrd =
let rra3 = rra_create CF_Average 100 100 0.5 in
let rra4 = rra_create CF_Average 100 1000 0.5 in
let ts = 1000000000.0 in
let ds = ds_create "foo" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds2 = ds_create "bar" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds3 = ds_create "baz" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds4 = ds_create "boo" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds =
ds_create "foo" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds2 =
ds_create "bar" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds3 =
ds_create "baz" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds4 =
ds_create "boo" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let rrd = rrd_create [|ds; ds2; ds3; ds4|] [|rra; rra2; rra3; rra4|] 1L ts in
let id = Identity in
for i = 1 to 100000 do
Expand All @@ -143,10 +151,18 @@ let absolute_rrd_CA_404597 () =
let rra3 = rra_create CF_Average 100 100 0.5 in
let rra4 = rra_create CF_Average 100 1000 0.5 in
let ts = 1000000000.0 in
let ds = ds_create "foo" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds2 = ds_create "bar" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds3 = ds_create "baz" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds4 = ds_create "boo" Absolute ~mrhb:10.0 (VT_Float 0.0) in
let ds =
ds_create "foo" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds2 =
ds_create "bar" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds3 =
ds_create "baz" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds4 =
ds_create "boo" Absolute ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let rrd = rrd_create [|ds; ds2; ds3; ds4|] [|rra; rra2; rra3; rra4|] 1L ts in
let id = Identity in
for i = 1 to 100000 do
Expand Down Expand Up @@ -181,10 +197,18 @@ let gauge_rrd_CA_404597 () =
let rra3 = rra_create CF_Average 100 100 0.5 in
let rra4 = rra_create CF_Average 100 1000 0.5 in
let ts = 1000000000.0 in
let ds = ds_create "foo" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds2 = ds_create "bar" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds3 = ds_create "baz" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds4 = ds_create "boo" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds =
ds_create "foo" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds2 =
ds_create "bar" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds3 =
ds_create "baz" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds4 =
ds_create "boo" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let rrd = rrd_create [|ds; ds2; ds3; ds4|] [|rra; rra2; rra3; rra4|] 1L ts in
let id = Identity in
for i = 1 to 100000 do
Expand Down Expand Up @@ -217,10 +241,18 @@ let gauge_rrd =
let rra3 = rra_create CF_Average 100 100 0.5 in
let rra4 = rra_create CF_Average 100 1000 0.5 in
let ts = 1000000000.0 in
let ds = ds_create "foo" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds2 = ds_create "bar" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds3 = ds_create "baz" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds4 = ds_create "boo" Gauge ~mrhb:10.0 (VT_Float 0.0) in
let ds =
ds_create "foo" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds2 =
ds_create "bar" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds3 =
ds_create "baz" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let ds4 =
ds_create "boo" Gauge ~mrhb:10.0 ~min:0. ~max:infinity (VT_Float 0.0)
in
let rrd = rrd_create [|ds; ds2; ds3; ds4|] [|rra; rra2; rra3; rra4|] 1L ts in
let id = Identity in
for i = 1 to 100000 do
Expand Down Expand Up @@ -252,7 +284,9 @@ let _deserialize_verify_rrd =
let rra1 = rra_create CF_Average 100 1 0.5 in
let rra2 = rra_create CF_Min 100 1 0.5 in
let rra3 = rra_create CF_Max 100 1 0.5 in
let ds = ds_create "flip_flop" Derive (VT_Int64 0L) in
let ds =
ds_create "flip_flop" Derive ~min:0. ~max:infinity ~mrhb:5. (VT_Int64 0L)
in

let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L 0. in

Expand All @@ -269,7 +303,9 @@ let ca_322008_rrd =
let rra1 = rra_create CF_Average 100 1 0.5 in
let rra2 = rra_create CF_Min 100 1 0.5 in
let rra3 = rra_create CF_Max 100 1 0.5 in
let ds = ds_create "even or zero" Derive ~min:0. (VT_Int64 0L) in
let ds =
ds_create "even or zero" Derive ~min:0. ~max:infinity ~mrhb:5. (VT_Int64 0L)
in

let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L 0. in

Expand All @@ -287,7 +323,9 @@ let ca_329043_rrd_1 =
let rra1 = rra_create CF_Average 3 1 0.5 in
let rra2 = rra_create CF_Min 3 1 0.5 in
let rra3 = rra_create CF_Max 3 1 0.5 in
let ds = ds_create "derive_with_min" ~min:0. ~max:1. Derive VT_Unknown in
let ds =
ds_create "derive_with_min" ~min:0. ~max:1. ~mrhb:5. Derive VT_Unknown
in

let rrd = rrd_create [|ds|] [|rra1; rra2; rra3|] 5L 0. in

Expand All @@ -313,9 +351,10 @@ let create_rrd ?(rows = 2) values min max =
let rra2 = rra_create CF_Min rows 10 0.5 in
let rra3 = rra_create CF_Max rows 10 0.5 in
let rra4 = rra_create CF_Last rows 10 0.5 in
let ds1 = ds_create "derive" ~min ~max Derive VT_Unknown in
let ds2 = ds_create "absolute" ~min ~max Derive VT_Unknown in
let ds3 = ds_create "gauge" ~min ~max Derive VT_Unknown in
let mrhb = 5. in
let ds1 = ds_create "derive" ~min ~max ~mrhb Derive VT_Unknown in
let ds2 = ds_create "absolute" ~min ~max ~mrhb Derive VT_Unknown in
let ds3 = ds_create "gauge" ~min ~max ~mrhb Derive VT_Unknown in

let rrd =
rrd_create [|ds1; ds2; ds3|] [|rra1; rra2; rra3; rra4|] 5L init_time
Expand All @@ -339,7 +378,9 @@ let ca_329043_rrd_2 =

let ca_329813_rrd =
let rrd = create_rrd [0L; 5L; 10L] 0. 1. in
let new_ds = ds_create "new!" Derive VT_Unknown in
let new_ds =
ds_create "new!" Derive VT_Unknown ~min:0. ~max:infinity ~mrhb:5.
in
Rrd.rrd_add_ds rrd rrd.last_updated new_ds

let test_ca_322008 () =
Expand Down
4 changes: 3 additions & 1 deletion ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ let merge_new_dss rrdi dss =
(* SAFETY: verified that these datasources aren't enabled above
already, in a more efficient way than RRD does it *)
rrd_add_ds_unsafe rrd timestamp
(Rrd.ds_create ds.ds_name ds.Ds.ds_type ~mrhb:300.0 Rrd.VT_Unknown)
(Rrd.ds_create ds.ds_name ds.Ds.ds_type ~mrhb:300.0 ~min:ds.ds_min
~max:ds.ds_max Rrd.VT_Unknown
)
)
new_enabled_dss rrdi.rrd
)
Expand Down
4 changes: 3 additions & 1 deletion ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,9 @@ let add_ds ~rrdi ~ds_name =
fail_missing ds_name
| Some (timestamp, ds) ->
Rrd.rrd_add_ds rrdi.rrd timestamp
(Rrd.ds_create ds.ds_name ds.ds_type ~mrhb:300.0 Rrd.VT_Unknown)
(Rrd.ds_create ds.ds_name ds.ds_type ~min:ds.ds_min ~max:ds.ds_max
~mrhb:300.0 Rrd.VT_Unknown
)

let add rrds uuid domid ds_name rrdi =
let rrd = add_ds ~rrdi ~ds_name in
Expand Down
Loading