CP-26352 Port xcp-networkd from Camlp4 to PPX #128

krizex · 2018-02-09T07:44:08Z

update implementation per interface changes
for interface implementation, move labeled parameter and optional parameter
to positional parameter and remove useless tail unit parameter

Signed-off-by: Yang Qian yang.qian@citrix.com

This PR should goes in with xapi-project/xcp-idl#189

This change is

mseri

I really don't like that we are removing all the named parameters here. is there any way to re-enable these?

mseri · 2018-02-09T11:24:43Z

lib/network_config.ml

-		config_json |> Jsonrpc.of_string |> config_t_of_rpc
+		match config_json |> Jsonrpc.of_string |> Rpcmarshal.unmarshal typ_of_config_t with
+		| Result.Ok v -> v
+		| Result.Error e -> raise Read_error


Please add a debug statement that records the error in e

Will do when I add code to fix the read error.

mseri · 2018-02-09T11:25:07Z

networkd/network_monitor.ml

-	let payload = stats |> rpc_of_stats_t |> Jsonrpc.to_string in
-	let checksum = payload |> Digest.string |> Digest.to_hex in
+	let payload = stats |> Rpcmarshal.marshal typ_of_stats_t |> Jsonrpc.to_string in
+    let checksum = payload |> Digest.string |> Digest.to_hex in


You should use tabs here

mseri · 2018-02-09T11:25:34Z

networkd/network_monitor_thread.ml

@@ -229,7 +229,7 @@ let rec monitor dbg () =
 			) devs
 		in

-		let bonds : (string * string list) list = Network_server.Bridge.get_all_bonds () dbg ~from_cache:true () in
+		let bonds : (string * string list) list = Network_server.Bridge.get_all_bonds dbg true in


Please keep the label if it does not confuse the compiler. That true on its own is confusing, I'd rather do
let from_cache = true in ... if we cannot keep the labels

Sadly we cannot keep the labels, so we should add the let statement

mseri · 2018-02-09T11:25:56Z

networkd/network_server.ml

@@ -68,13 +68,13 @@ let on_timer () =

 let reopen_logs _ () = true


Let's remove _ here as well

I'd rather remove the function as it is out of use.

mseri · 2018-02-09T11:26:44Z

networkd/network_server.ml

-			debug "Configuring IPv4 static routes for %s: %s" name (String.concat ", " (List.map (fun (i, p, g) ->
-				Printf.sprintf "%s/%d/%s" (Unix.string_of_inet_addr i) p (Unix.string_of_inet_addr g)) routes));
+			debug "Configuring IPv4 static routes for %s: %s" name (String.concat ", " (List.map (fun r ->
+				Printf.sprintf "%s/%d/%s" (Unix.string_of_inet_addr r.subnet) r.netmask (Unix.string_of_inet_addr r.gateway)) routes));


This looks much better now :)

minishrink

Looks good! I have a few suggestions for improvements to the code you haven't actually touched, mostly stylistic:

As List.remove_assoc is idempotent, you could probably remove the redundant List.mem_assoc here, though I'm not sure if you could also do it for the use here.
Functions like vlan_bug_workaround use if List.mem_assoc [...] then List.assoc [...] where you could just use try List.assoc [...] with Not_found -> None
We're throwing around a lot of mentions of "UP" and "DOWN", would it be better to globally declare let up_str = "UP" and let down_str = "DOWN"? That way, if the string is changed in future, we won't have to update all these functions, just the values themselves.
A thought: lines 969-971 of make_config look like they could be refactored with use of the built-in compare for options. This depends on whether the type of config is likely to change, so up to you/other people with knowledge on this.

Anyway, these are all optional changes, so it's up to you.

minishrink · 2018-02-09T11:45:45Z

networkd/network_server.ml

 		Debug.with_thread_associated dbg (fun () ->
 			match Linux_bonding.get_bond_master_of name with
 			| Some master -> Proc.get_bond_slave_mac master name
 			| None -> Ip.get_mac name
 		) ()

-	let is_up _ dbg ~name =
+	let is_up dbg name =
 		Debug.with_thread_associated dbg (fun () ->
 			if List.mem name (Sysfs.list ()) then
 				Ip.is_up name
 			else
 				false


This behaviour is already encoded in Ip.is_up as below, so it might be more efficient to remove lines 149, 151-152?
let is_up dev = try (List.mem "UP" (get_link_flags dev) with _ -> false

No, if change it as you did, will remove the check of the dev existence in sysfs.

minishrink · 2018-02-09T13:48:00Z

Could you name this PR to something like "CP-26352: Port xcp-networkd from Camlp4 to PPX", so it's relevant to the repo you've modified? That way we can tell all your PRs apart by title.

robhoes · 2018-02-09T13:52:25Z

lib/network_config.ml

@@ -114,7 +116,9 @@ let write_config config =
 let read_config () =
 	try
 		let config_json = Xapi_stdext_unix.Unixext.string_of_file config_file_path in
-		config_json |> Jsonrpc.of_string |> config_t_of_rpc
+		match config_json |> Jsonrpc.of_string |> Rpcmarshal.unmarshal typ_of_config_t with


This is the part that we need to test very well: it needs to be able to read a JSON file with config that was written by the old version (pre-PPX) of the software, because that is what happens on upgrade.

An issue here should make the update/upgrade tests in bvt and bst fail right?

Most likely.

Yes, I think it should be OK except the interface_config_t.ipv4_routes because it changes from tuple list to a struct list.
I will test and add code to fix it.

I have tested this case by following:

install XS of ring3 master branch

Configure static_routes for network by:
xe network-param-set uuid=a09c23c3-9a46-e1aa-c7c8-6efb5b3256de other-config:static-routes="192.168.2.0/24/192.168.2.1"

plug the PIF in the network

send kill signal to networkc process so it will persist the configuration to networkd.db

replace xapi and xcp-networkd by that of my private branch

restart toolstack

check xensource.log

Expect result: We should get Result.Error when unmarshalling
Actual result: Got Result.Ok

The test point is the type change of interface_config_t .ipv4_routes
from
(Unix.inet_addr * int * Unix.inet_addr) list
to

type ipv4_route_t = { subnet : Unix.inet_addr; netmask : int; gateway : Unix.inet_addr; } [@@deriving rpcty]

It is expected to get some error at runtime but actually not.

Thd network.db file content is:
{"interface_config":{"xenbr0":{"ipv4_conf":"DHCP4","ipv6_conf":"Linklocal6","ipv4_routes":[["192.168.2.0",24,"192.168.2.1"]],"dns":[[],[]],"mtu":1500,"ethtool_settings":{},"ethtool_offload":{"gro":"on","lro":"off"},"persistent_i":true},"eth0":{"ipv4_conf":"None4","ipv6_conf":"None6","ipv4_routes":[],"dns":[[],[]],"mtu":1500,"ethtool_settings":{},"ethtool_offload":{"gro":"on","lro":"off"},"persistent_i":true},"eth1":{"ipv4_conf":"None4","ipv6_conf":"None6","ipv4_routes":[],"dns":[[],[]],"mtu":1500,"ethtool_settings":{},"ethtool_offload":{"gro":"on","lro":"off"},"persistent_i":true},"xenbr1":{"ipv4_conf":"None4","ipv6_conf":"Linklocal6","ipv4_routes":[],"dns":[[],[]],"mtu":1500,"ethtool_settings":{},"ethtool_offload":{"gro":"on","lro":"off"},"persistent_i":true}},"bridge_config":{"xenbr0":{"ports":{"eth0":{"interfaces":["eth0"],"bond_properties":{},"kind":"Basic"}},"bridge_mac":"c8:1f:66:ba:ff:2b","igmp_snooping":false,"other_config":{"static-routes":"192.168.2.0/24/192.168.2.1","memory-ratio-hvm":"0.25","memory-ratio-pv":"0.25","network-uuids":"a09c23c3-9a46-e1aa-c7c8-6efb5b3256de"},"persistent_b":true},"xenbr1":{"ports":{"eth1":{"interfaces":["eth1"],"bond_properties":{},"kind":"Basic"}},"bridge_mac":"c8:1f:66:ba:ff:2c","igmp_snooping":false,"other_config":{"memory-ratio-hvm":"0.25","memory-ratio-pv":"0.25","network-uuids":"92958b17-e115-0a17-acf8-12b2bbeea0c8"},"persistent_b":true}},"gateway_interface":"xenbr0","dns_interface":"xenbr0"}

Update 1.
OK, now I find the root cause is we defined a default value for interface_config_t .ipv4_routes so if unmarshal error, it will get the default value [].
The code generated by rewriter is

(fun interface_config_t_dns -> (match getter.Rpc.Types.fget "ipv4_routes" (let open! Rpc.Types in List (let open! Rpc.Types in typ_of_ipv4_route_t)) with | Result.Ok _ as y -> y | Result.Error _ -> Result.Ok [])

I have 2 proposed solutions:

Can we add some rewriter or hook code to https://github.com/krizex/xcp-idl/blob/private/yangqi/CP-26352/network/network_interface.ml#L107 to handle this case?

Convert the configuration file to new format (for ipv4_routes field) before unmarshalling it.

Any thoughts ^ @robhoes @mseri ?

I have updated this PR by implemented option 2.
090b340

krizex · 2018-02-11T07:18:35Z

I really don't like that we are removing all the named parameters here. is there any way to re-enable these?

@mseri Yes, two options:

ocamlrpc implements labeled parameter support
keep the interface as before and add wrapper function so that we can bind the interface implementation to the wrapper function, and call the actual implementation (which keep the labeled parameter) inside.

krizex · 2018-02-11T07:40:14Z

@minishrink Thanks, I will update the title and commit message. Wrt to the optional comments you mentioned, I'd like to keep the code as is because that's not in the scope of this pull request. I'd like to keep this PR as small as possible. We could apply the minor changes in the future.

1. update implementation per interface changes 1. for interface implementation, move labeled parameter and optional parameter to positional parameter and remove useless tail unit parameter Signed-off-by: Yang Qian <yang.qian@citrix.com>

krizex · 2018-02-11T07:42:50Z

Update with some minor changes.

Signed-off-by: Yang Qian <yang.qian@citrix.com>

krizex · 2018-02-27T06:23:10Z

Ring3 BVT passed in job 79965
Ring3 BST passed job 79962
Network regression passed job 80071 except infrastructure issues of bond cases on BOURNE04, I have spoke it to Rama and got confirmed, the ticket has been assigned to him.

mseri · 2018-02-27T12:37:30Z

I am going to do a manual merge to master and close the PR to ppx_network

krizex mentioned this pull request Feb 9, 2018

CP-26352 Port xcp-idl/network interface from Camlp4 to PPX xapi-project/xcp-idl#189

Closed

mseri requested changes Feb 9, 2018

View reviewed changes

minishrink reviewed Feb 9, 2018

View reviewed changes

robhoes reviewed Feb 9, 2018

View reviewed changes

krizex changed the title ~~CP-26352 Port xcp-idl/network interface from Camlp4 to PPX~~ CP-26352 Port xcp-networkd from Camlp4 to PPX Feb 11, 2018

CP-26352 Port xcp-networkd from Camlp4 to PPX

018c20d

1. update implementation per interface changes 1. for interface implementation, move labeled parameter and optional parameter to positional parameter and remove useless tail unit parameter Signed-off-by: Yang Qian <yang.qian@citrix.com>

krizex force-pushed the private/yangqi/CP-26685 branch from 9ffeba6 to 018c20d Compare February 11, 2018 07:41

Convert configuration file to adapt ipv4_route structure changes

090b340

Signed-off-by: Yang Qian <yang.qian@citrix.com>

mseri closed this Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-26352 Port xcp-networkd from Camlp4 to PPX #128

CP-26352 Port xcp-networkd from Camlp4 to PPX #128

krizex commented Feb 9, 2018 •

edited by robhoes

Loading

mseri left a comment

mseri Feb 9, 2018

krizex Feb 11, 2018

mseri Feb 9, 2018

krizex Feb 11, 2018

mseri Feb 9, 2018

krizex Feb 11, 2018

mseri Feb 9, 2018

krizex Feb 11, 2018 •

edited

Loading

mseri Feb 9, 2018

minishrink left a comment •

edited

Loading

minishrink Feb 9, 2018

krizex Feb 11, 2018

minishrink commented Feb 9, 2018

robhoes Feb 9, 2018

mseri Feb 9, 2018

robhoes Feb 9, 2018

krizex Feb 11, 2018

krizex Feb 12, 2018 •

edited

Loading

krizex Feb 22, 2018

krizex Feb 22, 2018

krizex commented Feb 11, 2018 •

edited

Loading

krizex commented Feb 11, 2018 •

edited

Loading

krizex commented Feb 11, 2018 •

edited

Loading

krizex commented Feb 27, 2018

mseri commented Feb 27, 2018

		@@ -68,13 +68,13 @@ let on_timer () =

		let reopen_logs _ () = true

CP-26352 Port xcp-networkd from Camlp4 to PPX #128

CP-26352 Port xcp-networkd from Camlp4 to PPX #128

Conversation

krizex commented Feb 9, 2018 • edited by robhoes Loading

mseri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizex Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minishrink left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minishrink commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizex Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizex commented Feb 11, 2018 • edited Loading

krizex commented Feb 11, 2018 • edited Loading

krizex commented Feb 11, 2018 • edited Loading

krizex commented Feb 27, 2018

mseri commented Feb 27, 2018

krizex commented Feb 9, 2018 •

edited by robhoes

Loading

krizex Feb 11, 2018 •

edited

Loading

minishrink left a comment •

edited

Loading

krizex Feb 12, 2018 •

edited

Loading

krizex commented Feb 11, 2018 •

edited

Loading

krizex commented Feb 11, 2018 •

edited

Loading

krizex commented Feb 11, 2018 •

edited

Loading