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

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 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.

Author: edwintorok

ocaml/libs/xapi-rrd/lib/rrd.ml

@@ -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

ocaml/libs/xapi-rrd/lib_test/crowbar_tests.ml

@@ -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
     )
   )
 

ocaml/libs/xapi-rrd/lib_test/unit_tests.ml

@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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
@@ -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 () =

ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml

@@ -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
   )

ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml

@@ -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