CA-404020: Do not fail when removing a non-existing datasource (#6199)

The latest changes in the metrics daemon changes how archived and live metrics are synchronised, and archived sources are created less often. This meant that on some cases, like SR.forget, the operations failed because they could query for existing sources, but the call to remove them failed, because the metrics only exist in live form, not archived one. (what happens with the live one, do they disappear when the SR is forgotten?) Signed-off-by: Pau Ruiz Safont <[email protected]>
xapi-project · Jan 3, 2025 · c264219 · c264219
2 parents 34b5b63 + e4e20ad
commit c264219
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 90 deletions.
diff --git a/ocaml/libs/xapi-rrd/lib/rrd.ml b/ocaml/libs/xapi-rrd/lib/rrd.ml
@@ -621,9 +621,9 @@ let rrd_add_ds_unsafe rrd timestamp newds =
         rrd.rrd_rras
   }
 
-(** Add in a new DS into a pre-existing RRD. Preserves data of all the other archives
-    and fills the new one full of NaNs. Note that this doesn't fill in the CDP values
-    correctly at the moment!
+(** Add in a new DS into a pre-existing RRD. Preserves data of all the other
+    archives and fills the new one full of NaNs. Note that this doesn't fill
+    in the CDP values correctly at the moment!
 *)
 
 let rrd_add_ds rrd timestamp newds =
@@ -632,28 +632,27 @@ let rrd_add_ds rrd timestamp newds =
   else
     rrd_add_ds_unsafe rrd timestamp newds
 
-(** Remove the named DS from an RRD. Removes all of the data associated with it, too *)
+(** Remove the named DS from an RRD. Removes all of the data associated with
+    it, too. THe function is idempotent. *)
 let rrd_remove_ds rrd ds_name =
-  let n =
-    Utils.array_index ds_name (Array.map (fun ds -> ds.ds_name) rrd.rrd_dss)
-  in
-  if n = -1 then
-    raise (Invalid_data_source ds_name)
-  else
-    {
-      rrd with
-      rrd_dss= Utils.array_remove n rrd.rrd_dss
-    ; rrd_rras=
-        Array.map
-          (fun rra ->
-            {
-              rra with
-              rra_data= Utils.array_remove n rra.rra_data
-            ; rra_cdps= Utils.array_remove n rra.rra_cdps
-            }
-          )
-          rrd.rrd_rras
-    }
+  match Utils.find_index (fun ds -> ds.ds_name = ds_name) rrd.rrd_dss with
+  | None ->
+      rrd
+  | Some n ->
+      {
+        rrd with
+        rrd_dss= Utils.array_remove n rrd.rrd_dss
+      ; rrd_rras=
+          Array.map
+            (fun rra ->
+              {
+                rra with
+                rra_data= Utils.array_remove n rra.rra_data
+              ; rra_cdps= Utils.array_remove n rra.rra_cdps
+              }
+            )
+            rrd.rrd_rras
+      }
 
 (** Find the RRA with a particular CF that contains a particular start
     time, and also has a minimum pdp_cnt. If it can't find an
@@ -698,18 +697,17 @@ let find_best_rras rrd pdp_interval cf start =
     List.filter (contains_time newstarttime) rras
 
 let query_named_ds rrd as_of_time ds_name cf =
-  let n =
-    Utils.array_index ds_name (Array.map (fun ds -> ds.ds_name) rrd.rrd_dss)
-  in
-  if n = -1 then
-    raise (Invalid_data_source ds_name)
-  else
-    let rras = find_best_rras rrd 0 (Some cf) (Int64.of_float as_of_time) in
-    match rras with
-    | [] ->
-        raise No_RRA_Available
-    | rra :: _ ->
-        Fring.peek rra.rra_data.(n) 0
+  match Utils.find_index (fun ds -> ds.ds_name = ds_name) rrd.rrd_dss with
+  | None ->
+      raise (Invalid_data_source ds_name)
+  | Some n -> (
+      let rras = find_best_rras rrd 0 (Some cf) (Int64.of_float as_of_time) in
+      match rras with
+      | [] ->
+          raise No_RRA_Available
+      | rra :: _ ->
+          Fring.peek rra.rra_data.(n) 0
+    )
 
 (******************************************************************************)
 (* Marshalling/Unmarshalling functions                                        *)
@@ -876,30 +874,26 @@ let from_xml input =
 
       (* Purge any repeated data sources from the RRD *)
       let ds_names = ds_names rrd in
-      let ds_names_set = Utils.setify ds_names in
-      let ds_name_counts =
-        List.map
-          (fun name ->
-            let x, _ = List.partition (( = ) name) ds_names in
-            (name, List.length x)
-          )
-          ds_names_set
-      in
-      let removals_required =
-        List.filter (fun (_, x) -> x > 1) ds_name_counts
-      in
-      List.fold_left
-        (fun rrd (name, n) ->
-          (* Remove n-1 lots of this data source *)
-          let rec inner rrd n =
-            if n = 1 then
-              rrd
-            else
-              inner (rrd_remove_ds rrd name) (n - 1)
-          in
-          inner rrd n
-        )
-        rrd removals_required
+      List.sort_uniq String.compare ds_names
+      |> List.filter_map (fun name ->
+             match List.filter (String.equal name) ds_names with
+             | [] | [_] ->
+                 None
+             | x ->
+                 Some (name, List.length x)
+         )
+      |> List.fold_left
+           (fun rrd (name, n) ->
+             (* Remove n-1 lots of this data source *)
+             let rec inner rrd n =
+               if n = 1 then
+                 rrd
+               else
+                 inner (rrd_remove_ds rrd name) (n - 1)
+             in
+             inner rrd n
+           )
+           rrd
     )
     input
 

diff --git a/ocaml/libs/xapi-rrd/lib/rrd_utils.ml b/ocaml/libs/xapi-rrd/lib/rrd_utils.ml
@@ -47,13 +47,13 @@ end
 
 let isnan x = match classify_float x with FP_nan -> true | _ -> false
 
-let array_index e a =
+let find_index f a =
   let len = Array.length a in
   let rec check i =
     if len <= i then
-      -1
-    else if a.(i) = e then
-      i
+      None
+    else if f a.(i) then
+      Some i
     else
       check (i + 1)
   in
@@ -62,23 +62,6 @@ let array_index e a =
 let array_remove n a =
   Array.append (Array.sub a 0 n) (Array.sub a (n + 1) (Array.length a - n - 1))
 
-let filter_map f list =
-  let rec inner acc l =
-    match l with
-    | [] ->
-        List.rev acc
-    | x :: xs ->
-        let acc = match f x with Some res -> res :: acc | None -> acc in
-        inner acc xs
-  in
-  inner [] list
-
-let rec setify = function
-  | [] ->
-      []
-  | x :: xs ->
-      if List.mem x xs then setify xs else x :: setify xs
-
 (** C# and JS representation of special floats are 'NaN' and 'Infinity' which
     are different from ocaml's native representation. Caml is fortunately more
     forgiving when doing a float_of_string, and can cope with these forms, so

diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml
@@ -51,10 +51,10 @@ let merge_new_dss rrdi dss =
     !Rrdd_shared.enable_all_dss || ds.ds_default
   in
   let default_dss = StringMap.filter should_enable_ds dss in
-  (* NOTE: It's enough to check if all the default datasources have been added
-     to the RRD_INFO, because if a non-default one has been enabled at runtime,
-     it's added to the RRD immediately and we don't need to bother *)
-  let new_dss =
+  (* NOTE: Only add enabled dss to the live rrd, ignoring non-default ones.
+     This is because non-default ones are added to the RRD when they are
+     enabled. *)
+  let new_enabled_dss =
     StringMap.filter
       (fun ds_name _ -> not (StringMap.mem ds_name rrdi.dss))
       default_dss
@@ -73,7 +73,7 @@ let merge_new_dss rrdi dss =
         rrd_add_ds_unsafe rrd timestamp
           (Rrd.ds_create ds.ds_name ds.Ds.ds_type ~mrhb:300.0 Rrd.VT_Unknown)
       )
-      new_dss rrdi.rrd
+      new_enabled_dss rrdi.rrd
   )
 
 module OwnerMap = Map.Make (struct

diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml
@@ -347,7 +347,7 @@ let send_host_rrd_to_master master_address =
 let fail_missing name = raise (Rrdd_error (Datasource_missing name))
 
 (** {add_ds rrdi ds_name} creates a new time series (rrd) in {rrdi} with the
-    name {ds_name}. The operation fails if rrdi does not contain any live
+    name {ds_name}. The operation fails if rrdi does not contain any
     datasource with the name {ds_name} *)
 let add_ds ~rrdi ~ds_name =
   match Rrd.StringMap.find_opt ds_name rrdi.dss with

diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml
@@ -77,10 +77,13 @@ let mutex = Mutex.create ()
 
 type rrd_info = {
     rrd: Rrd.rrd
+        (** Contains the live metrics, i.e. The datasources that are enabled
+            and being collected .*)
   ; mutable dss: (float * Ds.ds) Rrd.StringMap.t
-        (* Important: this must contain the entire list of datasources associated
-           with the RRD, even the ones disabled by default, as rrd_add_ds calls
-           can enable DSs at runtime *)
+        (** Important: this must contain the entire list of datasources
+           associated with the RRD, even the ones disabled by default, because
+           functions like rrd_add_ds or rrd_remove_ds expect disabled
+           datasources to be present. e.g. to enable DSs at runtime *)
   ; mutable domid: int
 }