IH-715 - rrdp-netdev: Remove double (de)serialization

networkd generates metrics for two users simultaneously: * xapi db * rrdd Both of these read from the same shared file, but use non-overlapping stats. Having moved network metrics collection from xcp-rrdd itself into a plugin, these metrics were serialized twice - moving from networkd to the plugin and from the plugin to the server. Instead generate metrics in the plugin itself and drop this generation from networkd. Signed-off-by: Andrii Sultanov <[email protected]>
xapi-project · Oct 9, 2024 · 2c651f2 · 2c651f2
1 parent 8e3cb5f
commit 2c651f2
Show file tree

Hide file tree

Showing 4 changed files with 142 additions and 113 deletions.
diff --git a/ocaml/networkd/bin/network_monitor_thread.ml b/ocaml/networkd/bin/network_monitor_thread.ml
@@ -109,7 +109,6 @@ let standardise_name name =
   with _ -> name
 
 let get_link_stats () =
-  let open Network_monitor in
   let open Netlink in
   let s = Socket.alloc () in
   Socket.connect s Socket.NETLINK_ROUTE ;
@@ -124,101 +123,20 @@ let get_link_stats () =
     let is_vlan name =
       Astring.String.is_prefix ~affix:"eth" name && String.contains name '.'
     in
-    List.map (fun link -> (standardise_name (Link.get_name link), link)) links
+    List.map (fun link -> standardise_name (Link.get_name link)) links
     |> (* Only keep interfaces with prefixes on the whitelist, and exclude VLAN
           devices (ethx.y). *)
-    List.filter (fun (name, _) -> is_whitelisted name && not (is_vlan name))
-  in
-  let devs =
-    List.map
-      (fun (name, link) ->
-        let convert x = Int64.of_int (Unsigned.UInt64.to_int x) in
-        let eth_stat =
-          {
-            default_stats with
-            rx_bytes= Link.get_stat link Link.RX_BYTES |> convert
-          ; rx_pkts= Link.get_stat link Link.RX_PACKETS |> convert
-          ; rx_errors= Link.get_stat link Link.RX_ERRORS |> convert
-          ; tx_bytes= Link.get_stat link Link.TX_BYTES |> convert
-          ; tx_pkts= Link.get_stat link Link.TX_PACKETS |> convert
-          ; tx_errors= Link.get_stat link Link.TX_ERRORS |> convert
-          }
-        in
-        (name, eth_stat)
-      )
-      links
+    List.filter (fun name -> is_whitelisted name && not (is_vlan name))
   in
-  Cache.free cache ; Socket.close s ; Socket.free s ; devs
+  Cache.free cache ; Socket.close s ; Socket.free s ; links
 
 let rec monitor dbg () =
   let open Network_interface in
   let open Network_monitor in
   ( try
-      let make_bond_info devs (name, interfaces) =
-        let devs' =
-          List.filter (fun (name', _) -> List.mem name' interfaces) devs
-        in
-        let eth_stat =
-          {
-            default_stats with
-            rx_bytes=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.rx_bytes)
-                0L devs'
-          ; rx_pkts=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.rx_pkts)
-                0L devs'
-          ; rx_errors=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.rx_errors)
-                0L devs'
-          ; tx_bytes=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.tx_bytes)
-                0L devs'
-          ; tx_pkts=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.tx_pkts)
-                0L devs'
-          ; tx_errors=
-              List.fold_left
-                (fun ac (_, stat) -> Int64.add ac stat.tx_errors)
-                0L devs'
-          }
-        in
-        (name, eth_stat)
-      in
-      let add_bonds bonds devs = List.map (make_bond_info devs) bonds @ devs in
-      let transform_taps devs =
-        let newdevnames =
-          Xapi_stdext_std.Listext.List.setify (List.map fst devs)
-        in
+      let get_stats bonds devs =
         List.map
-          (fun name ->
-            let devs' = List.filter (fun (n, _) -> n = name) devs in
-            let tot =
-              List.fold_left
-                (fun acc (_, b) ->
-                  {
-                    default_stats with
-                    rx_bytes= Int64.add acc.rx_bytes b.rx_bytes
-                  ; rx_pkts= Int64.add acc.rx_pkts b.rx_pkts
-                  ; rx_errors= Int64.add acc.rx_errors b.rx_errors
-                  ; tx_bytes= Int64.add acc.tx_bytes b.tx_bytes
-                  ; tx_pkts= Int64.add acc.tx_pkts b.tx_pkts
-                  ; tx_errors= Int64.add acc.tx_errors b.tx_errors
-                  }
-                )
-                default_stats devs'
-            in
-            (name, tot)
-          )
-          newdevnames
-      in
-      let add_other_stats bonds devs =
-        List.map
-          (fun (dev, stat) ->
+          (fun dev ->
             if not (Astring.String.is_prefix ~affix:"vif" dev) then (
               let open Network_server.Bridge in
               let bond_slaves =
@@ -242,7 +160,6 @@ let rec monitor dbg () =
                   let links_up = if carrier then 1 else 0 in
                   let interfaces = [dev] in
                   {
-                    stat with
                     carrier
                   ; speed
                   ; duplex
@@ -286,7 +203,6 @@ let rec monitor dbg () =
                     List.map (fun info -> info.slave) bond_slaves
                   in
                   {
-                    stat with
                     carrier
                   ; speed
                   ; duplex
@@ -301,20 +217,15 @@ let rec monitor dbg () =
               check_for_changes ~dev ~stat ;
               (dev, stat)
             ) else
-              (dev, stat)
+              (dev, default_stats)
           )
           devs
       in
       let from_cache = true in
       let bonds : (string * string list) list =
         Network_server.Bridge.get_all_bonds dbg from_cache
       in
-      let devs =
-        get_link_stats ()
-        |> add_bonds bonds
-        |> transform_taps
-        |> add_other_stats bonds
-      in
+      let devs = get_link_stats () |> get_stats bonds in
       ( if List.length bonds <> Hashtbl.length bonds_status then
           let dead_bonds =
             Hashtbl.fold

diff --git a/ocaml/xapi-idl/network/network_stats.ml b/ocaml/xapi-idl/network/network_stats.ml
@@ -35,13 +35,7 @@ let checksum_bytes = 32
 let length_bytes = 8
 
 type iface_stats = {
-    tx_bytes: int64  (** bytes emitted *)
-  ; tx_pkts: int64  (** packets emitted *)
-  ; tx_errors: int64  (** error emitted *)
-  ; rx_bytes: int64  (** bytes received *)
-  ; rx_pkts: int64  (** packets received *)
-  ; rx_errors: int64  (** error received *)
-  ; carrier: bool
+    carrier: bool
   ; speed: int
   ; duplex: duplex
   ; pci_bus_path: string
@@ -55,13 +49,7 @@ type iface_stats = {
 
 let default_stats =
   {
-    tx_bytes= 0L
-  ; tx_pkts= 0L
-  ; tx_errors= 0L
-  ; rx_bytes= 0L
-  ; rx_pkts= 0L
-  ; rx_errors= 0L
-  ; carrier= false
+    carrier= false
   ; speed= 0
   ; duplex= Duplex_unknown
   ; pci_bus_path= ""

diff --git a/ocaml/xcp-rrdd/bin/rrdp-netdev/dune b/ocaml/xcp-rrdd/bin/rrdp-netdev/dune
@@ -3,14 +3,16 @@
   (name rrdp_netdev)
   (libraries
     astring
+    integers
+    netlink
     rrdd-plugin
     rrdd_plugin_xenctrl
     rrdd_plugins_libs
-    xapi-idl
     xapi-idl.network
     xapi-idl.rrd
     xapi-log
     xapi-rrd
+    xapi-stdext-std
     xenctrl
   )
 )

diff --git a/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml b/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml
@@ -18,6 +18,128 @@ module D = Debug.Make (struct let name = "xcp-rrdp-netdev" end)
 
 module Process = Rrdd_plugin.Process (struct let name = "xcp-rrdd-netdev" end)
 
+type iface_stats = {
+    tx_bytes: int64  (** bytes emitted *)
+  ; tx_pkts: int64  (** packets emitted *)
+  ; tx_errors: int64  (** error emitted *)
+  ; rx_bytes: int64  (** bytes received *)
+  ; rx_pkts: int64  (** packets received *)
+  ; rx_errors: int64  (** error received *)
+}
+
+let default_stats =
+  {
+    tx_bytes= 0L
+  ; tx_pkts= 0L
+  ; tx_errors= 0L
+  ; rx_bytes= 0L
+  ; rx_pkts= 0L
+  ; rx_errors= 0L
+  }
+
+let monitor_whitelist =
+  ref
+    [
+      "eth"
+    ; "vif" (* This includes "tap" owing to the use of standardise_name below *)
+    ]
+
+let standardise_name name =
+  try
+    let d1, d2 = Scanf.sscanf name "tap%d.%d" (fun d1 d2 -> (d1, d2)) in
+    let newname = Printf.sprintf "vif%d.%d" d1 d2 in
+    newname
+  with _ -> name
+
+let get_link_stats () =
+  let open Netlink in
+  let s = Socket.alloc () in
+  Socket.connect s Socket.NETLINK_ROUTE ;
+  let cache = Link.cache_alloc s in
+  let links = Link.cache_to_list cache in
+  let links =
+    let is_whitelisted name =
+      List.exists
+        (fun s -> Astring.String.is_prefix ~affix:s name)
+        !monitor_whitelist
+    in
+    let is_vlan name =
+      Astring.String.is_prefix ~affix:"eth" name && String.contains name '.'
+    in
+    List.map (fun link -> (standardise_name (Link.get_name link), link)) links
+    |> (* Only keep interfaces with prefixes on the whitelist, and exclude VLAN
+          devices (ethx.y). *)
+    List.filter (fun (name, _) -> is_whitelisted name && not (is_vlan name))
+  in
+  let devs =
+    List.map
+      (fun (name, link) ->
+        let convert x = Int64.of_int (Unsigned.UInt64.to_int x) in
+        let eth_stat =
+          {
+            rx_bytes= Link.get_stat link Link.RX_BYTES |> convert
+          ; rx_pkts= Link.get_stat link Link.RX_PACKETS |> convert
+          ; rx_errors= Link.get_stat link Link.RX_ERRORS |> convert
+          ; tx_bytes= Link.get_stat link Link.TX_BYTES |> convert
+          ; tx_pkts= Link.get_stat link Link.TX_PACKETS |> convert
+          ; tx_errors= Link.get_stat link Link.TX_ERRORS |> convert
+          }
+        in
+        (name, eth_stat)
+      )
+      links
+  in
+  Cache.free cache ; Socket.close s ; Socket.free s ; devs
+
+let make_bond_info devs (name, interfaces) =
+  let devs' = List.filter (fun (name', _) -> List.mem name' interfaces) devs in
+  let eth_stat =
+    {
+      rx_bytes=
+        List.fold_left (fun ac (_, stat) -> Int64.add ac stat.rx_bytes) 0L devs'
+    ; rx_pkts=
+        List.fold_left (fun ac (_, stat) -> Int64.add ac stat.rx_pkts) 0L devs'
+    ; rx_errors=
+        List.fold_left
+          (fun ac (_, stat) -> Int64.add ac stat.rx_errors)
+          0L devs'
+    ; tx_bytes=
+        List.fold_left (fun ac (_, stat) -> Int64.add ac stat.tx_bytes) 0L devs'
+    ; tx_pkts=
+        List.fold_left (fun ac (_, stat) -> Int64.add ac stat.tx_pkts) 0L devs'
+    ; tx_errors=
+        List.fold_left
+          (fun ac (_, stat) -> Int64.add ac stat.tx_errors)
+          0L devs'
+    }
+  in
+  (name, eth_stat)
+
+let add_bonds bonds devs = List.map (make_bond_info devs) bonds @ devs
+
+let transform_taps devs =
+  let newdevnames = Xapi_stdext_std.Listext.List.setify (List.map fst devs) in
+  List.map
+    (fun name ->
+      let devs' = List.filter (fun (n, _) -> n = name) devs in
+      let tot =
+        List.fold_left
+          (fun acc (_, b) ->
+            {
+              rx_bytes= Int64.add acc.rx_bytes b.rx_bytes
+            ; rx_pkts= Int64.add acc.rx_pkts b.rx_pkts
+            ; rx_errors= Int64.add acc.rx_errors b.rx_errors
+            ; tx_bytes= Int64.add acc.tx_bytes b.tx_bytes
+            ; tx_pkts= Int64.add acc.tx_pkts b.tx_pkts
+            ; tx_errors= Int64.add acc.tx_errors b.tx_errors
+            }
+          )
+          default_stats devs'
+      in
+      (name, tot)
+    )
+    newdevnames
+
 let generate_netdev_dss doms () =
   let uuid_of_domid domains domid =
     let _, uuid, _ =
@@ -28,8 +150,14 @@ let generate_netdev_dss doms () =
     in
     uuid
   in
-  let open Network_stats in
-  let stats = Network_stats.read_stats () in
+
+  let dbg = "rrdp_netdev" in
+  let from_cache = true in
+  let bonds : (string * string list) list =
+    Network_client.Client.Bridge.get_all_bonds dbg from_cache
+  in
+
+  let stats = get_link_stats () |> add_bonds bonds |> transform_taps in
   let dss, sum_rx, sum_tx =
     List.fold_left
       (fun (dss, sum_rx, sum_tx) (dev, stat) ->