CP-44103: Ordering network devices #6381
Conversation
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
from
b8aa61c to
1d1da55
Compare
minglumlu
changed the base branch from
master
to
feature/host-network-device-ordering
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
from
1d1da55 to
898b31d
Compare
|
The code is non-trivial, with lots of complexity. I think a design of the algorithm that can rationalize the different uses-cases is needed before accepting the code |
A separate PR #6387 is raised for this. |
|
|
||
| open D | ||
|
|
||
| let last_file_path = "/var/lib/xcp/last_ordered_network_devices.json" |
There was a problem hiding this comment.
Can we have all information in a single file? I would expect that we save the current order plus ethX slots that were previously used but are currently empty. Why do we need two files?
There was a problem hiding this comment.
In a separate conversation I have suggested extending the existing config_t record with extra fields for these new lists. This is serialised as JSON to the file networkd.db along with other networkd state.
|
|
||
| let index_of_string ty value = | ||
| match ty with | ||
| | ty when ty = "pci" -> |
There was a problem hiding this comment.
Strings can be matched directly without when
|
The list of interfaces per host is short. So I am not sure we need sophisticated data structures if we have lists with 10 elements at most. I also believe, the fundamental function that assigns NICs to ETH slots should be short and take in: the list of NICs and the previous assignment and create a new assignment (which then is persisted to a file). I do think it is a good idea to have modules for the different objects involved, like PCI slots, NICs, and MACs. So I believe less code could solve the task. |
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
2 times, most recently
from
8283b5a to
79254e5
Compare
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
3 times, most recently
from
77284fd to
bcb1845
Compare
| (** Type of one mapping configuration. *) | ||
| type t = {position: int; index: index} | ||
|
|
||
| val rules_of_file : path:string -> t list |
There was a problem hiding this comment.
A simpler name like "read" (because this is inside the Rule module already would work, too.
| (** [Duplicate_position] is raised when duplicate position is specified in the rules. *) | ||
| exception Duplicate_position | ||
|
|
||
| val matched : mac:Macaddr.t -> pci:Pciaddr.t -> label:string -> t -> bool |
There was a problem hiding this comment.
Would "matches" work better purely from a grammatical point of view?
| ; mac: Network_interface.mac_address | ||
| ; pci: Xcp_pci.address | ||
| ; bios_eth_order: int | ||
| ; multinic: bool |
| name: Network_interface.iface | ||
| ; mac: Network_interface.mac_address | ||
| ; pci: Xcp_pci.address | ||
| ; bios_eth_order: int |
There was a problem hiding this comment.
Coud this be negative? Otherwise add a comment that documents the range. Is this zero- or one-based?
There was a problem hiding this comment.
I added a doc for this field.
| exception Duplicate_mac_address | ||
| end | ||
|
|
||
| val generate : OrderedDev.t list -> OrderedDev.t list * (string * string) list |
There was a problem hiding this comment.
Would suggest "order" or "sort" as a name.
| (acc_ordered, List.rev_append unordered_devs acc_unordered) | ||
| ) | ||
| (ListToPciaddrMap.to_1n_map ~by:(fun v -> v.Dev.pci) multinics) | ||
| ([], []) |
There was a problem hiding this comment.
This function is a little complicated. Try to understand:
The function handles the remaining devices that are multi-nics after assign_position_by_rules, mac and pci
s1) Totally, these devices need to rearrange the N of ethN based on mac addresses because biosdevname's outcome is not stable for the multi-nics(see doc) and go to assign_position_for_remaining.
s2) There is a special scenario to handle: all mac addresses of multinics are changed, then assign the last positions to them. All other secanrios follow s1.
Notice that assign_position_by_pci skips the multinics.
There was a problem hiding this comment.
If all MAC addresses change, they will be considered as new devices.
| Alcotest.(check bool) "is Ok" true (Result.is_ok order) ; | ||
| let order = Result.get_ok order in | ||
| Alcotest.(check int) "6 devices in the order" 6 (List.length order) ; | ||
| Alcotest.(check int) "position assigned" 5 (pos_of_mac mac_addr0 order) ; |
There was a problem hiding this comment.
I don't like all this hardcoding, This means that the order is in two places now (the source of truth, rules in this case), and in the alcotest checks. Not only this is confusing, but it allows for errors
I'm preparing a branch with a more parametrized way of writing the tests.
It looks like this:
let test_default () =
let order =
sort' ~currents:current_slots ~rules:[] ~last_order:[] |> Result.get_ok
in
let test_position_and_presence mac =
test_position_and_presence (expected_of_dev_mac current_slots mac) order mac
in
Alcotest.(check int) "6 devices in the order" 6 (List.length order) ;
test_position_and_presence mac_addr0 ;
test_position_and_presence mac_addr1 ;
test_position_and_presence mac_addr2 ;
test_position_and_presence mac_addr3 ;
test_position_and_presence mac_addr4 ;
test_position_and_presence mac_addr5There was a problem hiding this comment.
Thanks. I updated the testing code based on the suggestions.
| name= "eno6" | ||
| ; pci= pci_addr6 | ||
| ; mac= mac_addr6 | ||
| ; bios_eth_order= 1 |
There was a problem hiding this comment.
This is inconsistent with currents !
There was a problem hiding this comment.
This is intended to test new adding devices (first seen). They are expected to be sorted by bios_eth_order.
There was a problem hiding this comment.
The issue is that currents contains dev1 which has bios_eth_order= 1. This means that when concatenating the values to form a new currents, there are two interfaces in the same order / slot. I thought this went against what biosdevname does. (we could add a unit-test ensuring this as well)
There was a problem hiding this comment.
I think tests need to improve, they have duplication, which lead to mistakes and some scenarios are not clear to me. I've prepared a branch with suggestions which include comments marked with XXX that should be resolved / explained.
https://github.com/psafont/xen-api/pull/new/private/mingl/CP-44103
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
2 times, most recently
from
cfc0849 to
77e417f
Compare
| ; pci= pci_addr4 | ||
| ; mac= mac_addr4 | ||
| ; bios_eth_order= 4 | ||
| ; multi_nic= true (* multinic: share PCI address with dev4 *) |
There was a problem hiding this comment.
| ; multi_nic= true (* multinic: share PCI address with dev4 *) | |
| ; multi_nic= true (* multinic: share PCI address with dev5 *) |
| @@ -0,0 +1,526 @@ | |||
| (** Generete an order for host network devices and keep the order as stable as possible. | |||
There was a problem hiding this comment.
"Generate"
Also, we need copyright header on top of the new files.
|
Squashing the fixup commits ... |
Signed-off-by: Ming Lu <[email protected]>
This part sorts host network devices in xcp-networkd. Previously, the ordering was handled by the interface-rename functionality. This will now be replaced by an equivalent function in xcp-networkd, but without renaming the network devices. The renmaing performed by the interface-rename was used to record the sorting result as the name of the NICs like "eth<N>". Now the sorting result will be saved in xcp-networkd database. Signed-off-by: Ming Lu <[email protected]>
The "test_network_device_order_inherited.ml" is inherited from the interface-rename functionality. Signed-off-by: Ming Lu <[email protected]>
minglumlu
force-pushed
the
private/mingl/CP-44103
branch
from
9ef757f to
bdb023a
Compare
minglumlu
merged commit c2756c6
into
xapi-project:feature/host-network-device-ordering
This part sorts host network devices in xcp-networkd. Previously, the
ordering was handled by the interface-rename functionality. This will
now be replaced by an equivalent function in xcp-networkd, but without
renaming the network devices. The renmaing performed by the
interface-rename was used to record the sorting result as the name of
the NICs like "eth". Now the sorting result will be recorded as
internal files.