From 2c651f25b90819155212a902722a389b57083007 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Wed, 2 Oct 2024 15:37:17 +0100 Subject: [PATCH 1/2] 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 --- ocaml/networkd/bin/network_monitor_thread.ml | 103 +------------- ocaml/xapi-idl/network/network_stats.ml | 16 +-- ocaml/xcp-rrdd/bin/rrdp-netdev/dune | 4 +- ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml | 132 +++++++++++++++++- 4 files changed, 142 insertions(+), 113 deletions(-) diff --git a/ocaml/networkd/bin/network_monitor_thread.ml b/ocaml/networkd/bin/network_monitor_thread.ml index 43b471be21a..9c4b7c3352d 100644 --- 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,7 +217,7 @@ let rec monitor dbg () = check_for_changes ~dev ~stat ; (dev, stat) ) else - (dev, stat) + (dev, default_stats) ) devs in @@ -309,12 +225,7 @@ let rec monitor dbg () = 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 index 1e10cb8a755..5c6fbaafa26 100644 --- 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 index fe68c431ab4..c5acc80a8be 100644 --- 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 index 55be1e88a0b..718fd574afd 100644 --- 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) -> From 271523424309da48667670206a426390fcb53a94 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Thu, 10 Oct 2024 10:04:40 +0100 Subject: [PATCH 2/2] fixup! IH-715 - rrdp-netdev: Remove double (de)serialization Signed-off-by: Andrii Sultanov --- ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml b/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml index 718fd574afd..5b138aebbe0 100644 --- a/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml +++ b/ocaml/xcp-rrdd/bin/rrdp-netdev/rrdp_netdev.ml @@ -44,12 +44,10 @@ let monitor_whitelist = ; "vif" (* This includes "tap" owing to the use of standardise_name below *) ] +(** Transform names of the form 'tapX.X' to 'vifX.X' so these can be handled + consistently later *) 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 + try Scanf.sscanf name "tap%d.%d" @@ Printf.sprintf "vif%d.%d" with _ -> name let get_link_stats () = let open Netlink in @@ -93,24 +91,17 @@ let get_link_stats () = let make_bond_info devs (name, interfaces) = let devs' = List.filter (fun (name', _) -> List.mem name' interfaces) devs in + let sum_list f = + List.fold_left (fun ac (_, stat) -> Int64.add ac (f stat)) 0L 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' + rx_bytes= sum_list (fun stat -> stat.rx_bytes) + ; rx_pkts= sum_list (fun stat -> stat.rx_pkts) + ; rx_errors= sum_list (fun stat -> stat.rx_errors) + ; tx_bytes= sum_list (fun stat -> stat.tx_bytes) + ; tx_pkts= sum_list (fun stat -> stat.tx_pkts) + ; tx_errors= sum_list (fun stat -> stat.tx_errors) } in (name, eth_stat)