From 0fc9c5f21047f96799591ae9edaad79787a11bc4 Mon Sep 17 00:00:00 2001
From: Andrii Sultanov <andrii.sultanov@cloud.com>
Date: Thu, 16 Jan 2025 13:08:45 +0000
Subject: [PATCH] CA-404597: rrd - Pass Gauge and Absolute data source values
 as-is

Some recent changes related to RRDs likely exposed a long-standing latent issue
where the RRD library would process the passed-in values for Gauge and Absolute
data sources incorrectly leading to constant values changing from update to
update, for example:
```
$ rrd2csv memory_total_kib
timestamp, AVERAGE:host:8b533333-91e1-4698-bd17-95b9732ffbb6:memory_total_kib
2025-01-15T08:41:40Z, 33351000
2025-01-15T08:41:45Z, 33350000
2025-01-15T08:41:50Z, 33346000
2025-01-15T08:41:55Z, 33352000
```

Instead of treating Gauge and Absolute data sources as a
variation on the rate-based Derive data source type, expecting time-based
calculations to cancel each other out, do not undertake any calculations on
non-rate data sources at all.

This makes the unit test added in the previous commit pass.

Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
---
 ocaml/libs/xapi-rrd/lib/rrd.ml | 39 +++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/ocaml/libs/xapi-rrd/lib/rrd.ml b/ocaml/libs/xapi-rrd/lib/rrd.ml
index 75610964fc1..1c9e536d28b 100644
--- a/ocaml/libs/xapi-rrd/lib/rrd.ml
+++ b/ocaml/libs/xapi-rrd/lib/rrd.ml
@@ -341,9 +341,10 @@ let rra_update rrd proc_pdp_st elapsed_pdp_st pdps =
   Array.iter updatefn rrd.rrd_rras
 
 (* We assume that the data being given is of the form of a rate; that is,
-   it's dependent on the time interval between updates. To be able to
-   deal with gauge DSs, we multiply by the interval so that it cancels
-   the subsequent divide by interval later on *)
+   it's dependent on the time interval between updates.
+   Gauge and Absolute data sources are simply kept as is without any
+   time-based calculations, while Derive data sources will be changed according
+   to the time passed since the last measurement. (see CA-404597) *)
 let process_ds_value ds value interval new_rrd =
   if interval > ds.ds_mrhb then
     nan
@@ -360,10 +361,8 @@ let process_ds_value ds value interval new_rrd =
 
     let rate =
       match (ds.ds_ty, new_rrd) with
-      | Absolute, _ | Derive, true ->
+      | Absolute, _ | Derive, true | Gauge, _ ->
           value_raw
-      | Gauge, _ ->
-          value_raw *. interval
       | Derive, false -> (
         match (ds.ds_last, value) with
         | VT_Int64 x, VT_Int64 y ->
@@ -433,7 +432,14 @@ let ds_update rrd timestamp valuesandtransforms new_rrd =
       if Utils.isnan value then
         ds.ds_unknown_sec <- pre_int
       else
-        ds.ds_value <- ds.ds_value +. (pre_int *. value /. interval)
+        (* CA-404597 - Gauge and Absolute values should be passed as-is,
+           without being involved in time-based calculations at all.
+           This applies to calculations below as well *)
+        match ds.ds_ty with
+        | Gauge | Absolute ->
+            ds.ds_value <- value
+        | Derive ->
+            ds.ds_value <- ds.ds_value +. (pre_int *. value /. interval)
     )
     v2s ;
 
@@ -450,7 +456,13 @@ let ds_update rrd timestamp valuesandtransforms new_rrd =
             let raw =
               let proc_pdp_st = get_float_time last_updated rrd.timestep in
               let occu_pdp_st = get_float_time timestamp rrd.timestep in
-              ds.ds_value /. (occu_pdp_st -. proc_pdp_st -. ds.ds_unknown_sec)
+
+              match ds.ds_ty with
+              | Gauge | Absolute ->
+                  ds.ds_value
+              | Derive ->
+                  ds.ds_value
+                  /. (occu_pdp_st -. proc_pdp_st -. ds.ds_unknown_sec)
             in
             (* Apply the transform after the raw value has been calculated *)
             let raw = apply_transform_function transform raw in
@@ -472,10 +484,13 @@ let ds_update rrd timestamp valuesandtransforms new_rrd =
         if Utils.isnan value then (
           ds.ds_value <- 0.0 ;
           ds.ds_unknown_sec <- post_int
-        ) else (
-          ds.ds_value <- post_int *. value /. interval ;
-          ds.ds_unknown_sec <- 0.0
-        )
+        ) else
+          match ds.ds_ty with
+          | Gauge | Absolute ->
+              ds.ds_value <- value
+          | Derive ->
+              ds.ds_value <- post_int *. value /. interval ;
+              ds.ds_unknown_sec <- 0.0
       )
       v2s
   )