networkd: Add bridge port priority setting

[xnox]

Allow setting bridge port priority in the Bridge section of the network file,
similar to e.g. bridge port path cost setting.

[xnox]

I've tested this successfully works as intended on Ubuntu development release (zesty 17.04)

[martinpitt]

LGTM, thanks! But this could do with a test case in test/networkd-test.py. I suppose the priority can be verified through bridge link show or sysfs?

[xnox]

@martinpitt there are no bridge tests in test/networkd-test.py do you want me to create interface(s) and add them into a bridge to test these?

[martinpitt]

@xnox: Ah right (seems we seriously lack bridge tests then). But that should be straightforward to add, so why not start with this feature. Are you generally able to run those? Unless you use networkd, they should be safe to run on your laptop, otherwise you can run them in lxd or an autopkgtest VM (Debian's systemd package also runs it as autopkgtest).

[xnox]

@martinpitt I don't think the new bridge tests need to be part of this merge proposal. It seems like bridge test for networkd-test.py deserve a separate merge proposal.

[ssahani]

We can set 0 too .

[xnox]

If I am reading the kernel source code right, the default priority is 32, so I could set 32 as the default value in the gperf . Then change this condition and only set priority when it's not 32? Or e.g. always set it? (But then default priority definition effectively moves from kernel to systemd)

[xnox]

Actually using an invalid value by default makes sense. Thus I now set priority at 128, which means don't update the priority such that whichever priority the device is at, it would stick. This is similar to using 0 in the path_cost, as 0 is an invalid setting for the path_cost.

[ssahani]

That ok as we already have such settings #define DEFAULT_TNL_HOP_LIMIT 64.
Default priority 32768 .

sudo ip link add name bridge_name type bridge

$ ip  -d link show bridge_name | grep priority
    bridge forward_delay 1500 hello_time 200 max_age 2000 ageing_time 30000 stp_state 0 priority 32768 vlan_filtering 0 vlan_protocol 802.1Q bridge_id 8000.0:0:0:0:0:0 designated_root 8000.0:0:0:0:0:0 root_port 0 root_path_cost 0 topology_change 0 topology_change_detected 0 hello_timer    0.00 tcn_timer    0.00 topology_change_timer    0.00 gc_timer    0.00 vlan_default_pvid 1 group_fwd_mask 0 group_address 01:80:c2:00:00:00 mcast_snooping 1 mcast_router 1 mcast_query_use_ifaddr 0 mcast_querier 0 mcast_hash_elasticity 4 mcast_hash_max 512 mcast_last_member_count 2 mcast_startup_query_count 2 mcast_last_member_interval 100 mcast_membership_interval 26000 mcast_querier_interval 25500 mcast_query_interval 12500 mcast_query_response_interval 1000 mcast_startup_query_interval 3125 nf_call_iptables 0 nf_call_ip6tables 0 nf_call_arptables 0 addrgenmode eui64 numtxqueues 1 numrxqueues 1 

[ssahani]

The default as it looks 32768

ip -d link show bridge_name
8: bridge_name: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 6a:67:73:86:30:e2 brd ff:ff:ff:ff:ff:ff promiscuity 0 
    bridge forward_delay 1500 hello_time 200 max_age 2000 ageing_time 30000 stp_state 0 priority 32768 vlan_filtering 0 vlan_protocol 802.1Q bridge_id 8000.0:0:0:0:0:0 designated_root 8000.0:0:0:0:0:0 root_port 0 root_path_cost 0 topology_change 0 topology_change_detected 0 hello_timer    0.00 tcn_timer    0.00 topology_change_timer    0.00 gc_timer    0.00 vlan_default_pvid 1 group_fwd_mask 0 group_address 01:80:c2:00:00:00 mcast_snooping 1 mcast_router 1 mcast_query_use_ifaddr 0 mcast_querier 0 mcast_hash_elasticity 4 mcast_hash_max 512 mcast_last_member_count 2 mcast_startup_query_count 2 mcast_last_member_interval 100 mcast_membership_interval 26000 mcast_querier_interval 25500 mcast_query_interval 12500 mcast_query_response_interval 1000 mcast_startup_query_interval 3125 nf_call_iptables 0 nf_call_ip6tables 0 nf_call_arptables 0 addrgenmode eui64 numtxqueues 1 numrxqueues 1 

Please use #define DEFAULT_ABC 100

[xnox]

@ssahani that is Bridge's priority. I'm working on Bridge Port's priority which is 32 with $ ip -d link show eth0 in the line prefixed "bridge_slave". Please see updated testsuite. Note, previously systemd would use [BridgePort] section in the .network file to define bridge port settings, but that was renamed to just [Bridge]. The sysfs file for it is /sys/class/net/port1/brport/priority and 128 is an out of range value for it.

[ssahani]

yes actually I mistaken thanks .

[xnox]

@martinpitt I have addressed your request for a testcase which is now part of this merge proposal.

[martinpitt]

Looks great, thanks for adding those! I have a couple of nitpicks, but the biggest issue is that the test seems to have a race condition because it doesn't wait for the bridge to become online?

[martinpitt]

Please replace the magic 128 with a macro like LINK_PRIORITY_INVALID.

[martinpitt]

LINK_PRIORITY_INVALID again, please.

[martinpitt]

assert_link_states(link="managed") is much more natural and cleaner to read.

[xnox]

ack

[xnox]

actually no, I cannot use "nice" syntax. As I don't want to pass {"link":"managed"} kwarg, but i want to pass {*link: "managed"} kwarg, e.g. when read_attr("eth0", "foo") is passed, I want to call assert_link_states(eth0="managed"), not assert_link_states(link="managed").

[martinpitt]

Oh I see, this needs to evaluate link. OK then.

[martinpitt]

os.path.join() is the standard Python way to build paths.

[martinpitt]

None of the tests call NETWORKD_WAIT_ONLINE, so how is that not racy?

[xnox]

All of them do indirectly...
They call read_attr, which always calls to assert_link_states, which calls NETWORKD_WAIT_ONLINE

[martinpitt]

OK. Can you please add this as a comment to read_attr()'s docstring, as it's not really evident at first sight? Thanks!

[martinpitt]

Heh, here you use the "nice" syntax :-)

[xnox]

Pushed updates; and rebutted one comment. The comments above were misplaced a bit. Still need to retest the updated code.

[poettering]

a few notes.

(yeah, it's a bit unfair, as the Cost= field doesn't get all this right either, but we really should fix that too, and get it right for Priority= from the beginning.)

(Bonus points if you fix the Cost= type chaos too btw... ;-))

[poettering]

please document here that this an integer, which range is permissible and whether the higher priority is referred to by higher or lower numbers.

[poettering]

(extra bonus points if you add the same for Cost=, where this was forgotten...)

[poettering]

should be config_parse_uint16 I figure (see other comment)

[poettering]

hmm, you define that as "unsigned" here (i.e. 32bit), but the netlink type appears to be 16bit only. Hence please use uint16_t here too, so that we use the same and correct type all the way

[poettering]

(and yeah, "cost" appears broken too, the netlink type appears to be uint32_t, and not unsigned, and we should use uint32_t here hence too...)

[xnox]

.... and despite netlink type uint16_t, the actual bridge port priority valid values are 0..63. And brctl manpage is a lie.

[poettering]

patch looks pretty OK to me, except for the type issues, see comments. I'll leave the review of the python test code to @martinpitt, he's our python test guru! ;-)

[martinpitt]

Tests look good to me, aside from the read_attr docstring. Please fix that along with Lennart's review points, then this should be golden. Thanks!

[martinpitt]

assert_link_states(link="managed") is much more natural and cleaner to read.

[xnox]

ack

[xnox]

actually no, I cannot use "nice" syntax. As I don't want to pass {"link":"managed"} kwarg, but i want to pass {*link: "managed"} kwarg, e.g. when read_attr("eth0", "foo") is passed, I want to call assert_link_states(eth0="managed"), not assert_link_states(link="managed").

[martinpitt]

Oh I see, this needs to evaluate link. OK then.

[martinpitt]

None of the tests call NETWORKD_WAIT_ONLINE, so how is that not racy?

[xnox]

All of them do indirectly...
They call read_attr, which always calls to assert_link_states, which calls NETWORKD_WAIT_ONLINE

[martinpitt]

OK. Can you please add this as a comment to read_attr()'s docstring, as it's not really evident at first sight? Thanks!

[xnox]

I think I have addressed all the comments from the last round of review now.

[martinpitt]

Thanks @xnox for the updates! @poettering's and my comments are addressed, tests are green.

[poettering]

Uh, there are two issues with this...

[poettering]

uh, this is problematic. You also have to change the type in the Network structure now... The config_parse_xxx() call always should match the type used. Meaning: if there's a variable of type "uint32_t" you should use config_parse_uint32 and if there's one of type "unsigned" you should use config_parse_unsigned... Hence, please fix the type here too...

Yes, on all archs we support uint32_t and unsigned are the same size, but still, we should always use the appropriate types through the entire pipeline

[xnox]

But I have changed offsetof(Network, cost) in src/network/networkd-network.h whilst doing so. No further actions needed here, then?

-        unsigned cost;
+        uint32_t cost;
+        uint16_t priority;

[poettering]

If the priority value only as a small permitted range, then please provide a smal config_parse_bridge_priority() call that does what config_parse_uint16 does, but also filters out the invalid range...

[xnox]

I was pondering that, such that logs will print that invalid values have been specified.

[poettering]

I opened #5729 now, given that this PR is already commited but needs fixing, and we can track this. @xnox any chance you can prep a quick fix for that?