Merge ~raharper/netplan:fix/interval-sec into ~netplan-developers/netplan/+git/netplan:master

[Ryan Harper]

Update docs and netplan to use milliseconds for time-based intervals

While networkd does accept time values for bonds and bridges at the 1 second
granularity, it also allows users to apply a suffix, such as 'ms' to indicate
the unit. netplan already documents that these time values are milliseconds for
the NetworkManager renderer, so this patch updates documentation to indicate
that all of the time values are now in millisecond form. The networkd renderer
is updated to append 'ms' to the interger value that's parsed.

LP: #1745597

[Mathieu Trudel-Lapierre (cyphermox)]

I'm worried about breaking stuff for those who already use these parameters, so that would be one argument /for/ parsing using strings with time units.

On the other hand, the current behavior is quite limited in the systemd backend using only seconds.

review: Needs Information

[Mathieu Trudel-Lapierre (cyphermox)]

Closing, this got merged with changes on github...

diff --git a/doc/netplan.md b/doc/netplan.md
index 7f37ae1..afe9fcc 100644
--- a/doc/netplan.md
+++ b/doc/netplan.md
@@ -247,10 +247,9 @@ Properties for device type ``bridges:``
 
 ``parameters`` (mapping)
 
-:    Customization parameters for special bridging options. Using the
-     NetworkManager renderer, parameter values for time intervals should be
-     expressed in milliseconds; for the systemd renderer, they should be in
-     seconds unless otherwise specified.
+:    Customization parameters for special bridging options.
+     Time-based values for intervals should be expressed in milliseconds
+     unless otherwise specified.
 
      ``ageing-time`` (scalar)
      :    Set the period of time to keep a MAC address in the forwarding
@@ -268,20 +267,18 @@ Properties for device type ``bridges:``
           designated port and root port selection algorithms.
 
      ``forward-delay`` (scalar)
-     :    Specify the period of time the bridge will remain in Listening and
-          Learning states before getting to the Forwarding state. This value
-          should be set in seconds for the systemd backend, and in milliseconds
-          for the NetworkManager backend.
+     :    Specify the period of time (milliseconds) the bridge will remain in
+          Listening and Learning states before getting to the Forwarding state.
 
      ``hello-time`` (scalar)
-     :    Specify the interval between two hello packets being sent out from
-          the root and designated bridges. Hello packets communicate
-          information about the network topology.
+     :    Specify the interval (milliseconds) between two hello packets being
+          sent out from the root and designated bridges. Hello packets
+          communicate information about the network topology.
 
      ``max-age`` (scalar)
-     :    Set the maximum age of a hello packet. If the last hello packet is
-          older than that value, the bridge will attempt to become the root
-          bridge.
+     :    Set the maximum age (millliseconds) of a hello packet. If the last
+          hello packet is older than that value, the bridge will attempt to
+          become the root bridge.
 
      ``path-cost`` (scalar)
      :    Set the cost of a path on the bridge. Faster interfaces should have
@@ -313,10 +310,9 @@ Properties for device type ``bonds:``
 
 ``parameters`` (mapping)
 
-:    Customization parameters for special bonding options. Using the
-     NetworkManager renderer, parameter values for intervals should be
-     expressed in milliseconds; for the systemd renderer, they should be in
-     seconds unless otherwise specified.
+:    Customization parameters for special bonding options.
+     Time-based values for intervals should be expressed in milliseconds
+     unless otherwise specified.
 
      ``mode`` (scalar)
      :    Set the bonding mode used for the interfaces. The default is
@@ -330,9 +326,9 @@ Properties for device type ``bonds:``
           and ``fast`` (every second).
 
      ``mii-monitor-interval`` (scalar)
-     :    Specifies the interval for MII monitoring (verifying if an interface
-          of the bond has carrier). The default is ``0``; which disables MII
-          monitoring.
+     :    Specifies the interval (milliseconds) for MII monitoring (verifying
+          if an interface of the bond has carrier). The default is ``0``; which
+          disables MII monitoring.
 
      ``min-links`` (scalar)
      :    The minimum number of links up in a bond to consider the bond
@@ -356,8 +352,9 @@ Properties for device type ``bonds:``
           behavior in most situations.
 
      ``arp-interval`` (scalar)
-     :    Set the interval value for how frequently ARP link monitoring should
-          happen. The default value is ``0``, which disables ARP monitoring.
+     :    Set the interval value (milliseconds) for how frequently ARP link
+          monitoring should happen. The default value is ``0``, which disables
+          ARP monitoring.
 
      ``arp-ip-targets`` (sequence of scalars)
      :    IPs of other hosts on the link which should be sent ARP requests in
@@ -379,12 +376,12 @@ Properties for device type ``bonds:``
           enabled. Possible values are ``any`` and ``all``.
 
      ``up-delay`` (scalar)
-     :    Specify the delay before enabling a link once the link is physically
-          up. The default value is ``0``.
+     :    Specify the delay (milliseconds) before enabling a link once the link
+          is physically up. The default value is ``0``.
 
      ``down-delay`` (scalar)
-     :    Specify the delay before disabling a link once the link has been
-          lost. The default value is ``0``.
+     :    Specify the delay (milliseconds) before disabling a link once the
+          link has been lost. The default value is ``0``.
 
      ``fail-over-mac-policy`` (scalar)
      :    Set whether to set all slaves to the same MAC address when adding
@@ -412,10 +409,10 @@ Properties for device type ``bonds:``
           possible values are ``always``, ``better``, and ``failure``.
 
      ``learn-packet-interval`` (scalar)
-     :    Specify the interval between sending learning packets to each slave.
-          The value range is between ``1`` and ``0x7fffffff``. The default
-          value is ``1``. This option only affects ``balance-tlb`` and
-          ``balance-alb`` modes.
+     :    Specify the interval (milliseconds) between sending learning packets
+          to each slave.  The value range is between ``1`` and ``0x7fffffff``.
+          The default value is ``1``. This option only affects ``balance-tlb``
+          and ``balance-alb`` modes.
 
      ``primary`` (scalar)
      :    Specify a device to be used as a primary slave, or preferred device
diff --git a/src/networkd.c b/src/networkd.c
index f29f2db..7164c39 100644
--- a/src/networkd.c
+++ b/src/networkd.c
@@ -65,7 +65,7 @@ write_bridge_params(GString* s, net_definition* def)
         params = g_string_sized_new(200);
 
         if (def->bridge_params.ageing_time)
-            g_string_append_printf(params, "AgeingTimeSec=%u\n", def->bridge_params.ageing_time);
+            g_string_append_printf(params, "AgeingTimeSec=%ums\n", def->bridge_params.ageing_time);
 #if 0
 	/* FIXME: Priority= is not valid for the bridge itself, although it should work as the
          *        STP priority of the bridge itself. It's not supported by networkd, but let's
@@ -75,11 +75,11 @@ write_bridge_params(GString* s, net_definition* def)
             g_string_append_printf(params, "Priority=%u\n", def->bridge_params.priority);
 #endif
         if (def->bridge_params.forward_delay)
-            g_string_append_printf(params, "ForwardDelaySec=%u\n", def->bridge_params.forward_delay);
+            g_string_append_printf(params, "ForwardDelaySec=%ums\n", def->bridge_params.forward_delay);
         if (def->bridge_params.hello_time)
-            g_string_append_printf(params, "HelloTimeSec=%u\n", def->bridge_params.hello_time);
+            g_string_append_printf(params, "HelloTimeSec=%ums\n", def->bridge_params.hello_time);
         if (def->bridge_params.max_age)
-            g_string_append_printf(params, "MaxAgeSec=%u\n", def->bridge_params.max_age);
+            g_string_append_printf(params, "MaxAgeSec=%ums\n", def->bridge_params.max_age);
         g_string_append_printf(params, "STP=%s\n", def->bridge_params.stp ? "true" : "false");
 
         g_string_append_printf(s, "\n[Bridge]\n%s", params->str);
@@ -131,7 +131,7 @@ write_bond_parameters(net_definition* def, GString* s)
     if (def->bond_params.lacp_rate)
         g_string_append_printf(params, "\nLACPTransmitRate=%s", def->bond_params.lacp_rate);
     if (def->bond_params.monitor_interval)
-        g_string_append_printf(params, "\nMIIMonitorSec=%d", def->bond_params.monitor_interval);
+        g_string_append_printf(params, "\nMIIMonitorSec=%dms", def->bond_params.monitor_interval);
     if (def->bond_params.min_links)
         g_string_append_printf(params, "\nMinLinks=%d", def->bond_params.min_links);
     if (def->bond_params.transmit_hash_policy)
@@ -141,7 +141,7 @@ write_bond_parameters(net_definition* def, GString* s)
     if (def->bond_params.all_slaves_active)
         g_string_append_printf(params, "\nAllSlavesActive=%d", def->bond_params.all_slaves_active);
     if (def->bond_params.arp_interval)
-        g_string_append_printf(params, "\nARPIntervalSec=%d", def->bond_params.arp_interval);
+        g_string_append_printf(params, "\nARPIntervalSec=%dms", def->bond_params.arp_interval);
     if (def->bond_params.arp_ip_targets && def->bond_params.arp_ip_targets->len > 0) {
         g_string_append_printf(params, "\nARPIPTargets=");
         for (unsigned i = 0; i < def->bond_params.arp_ip_targets->len; ++i) {
@@ -155,9 +155,9 @@ write_bond_parameters(net_definition* def, GString* s)
     if (def->bond_params.arp_all_targets)
         g_string_append_printf(params, "\nARPAllTargets=%s", def->bond_params.arp_all_targets);
     if (def->bond_params.up_delay)
-        g_string_append_printf(params, "\nUpDelaySec=%d", def->bond_params.up_delay);
+        g_string_append_printf(params, "\nUpDelaySec=%dms", def->bond_params.up_delay);
     if (def->bond_params.down_delay)
-        g_string_append_printf(params, "\nDownDelaySec=%d", def->bond_params.down_delay);
+        g_string_append_printf(params, "\nDownDelaySec=%dms", def->bond_params.down_delay);
     if (def->bond_params.fail_over_mac_policy)
         g_string_append_printf(params, "\nFailOverMACPolicy=%s", def->bond_params.fail_over_mac_policy);
     if (def->bond_params.gratuitious_arp)
@@ -170,7 +170,7 @@ write_bond_parameters(net_definition* def, GString* s)
     if (def->bond_params.resend_igmp)
         g_string_append_printf(params, "\nResendIGMP=%d", def->bond_params.resend_igmp);
     if (def->bond_params.learn_interval)
-        g_string_append_printf(params, "\nLearnPacketIntervalSec=%d", def->bond_params.learn_interval);
+        g_string_append_printf(params, "\nLearnPacketIntervalSec=%dms", def->bond_params.learn_interval);
 
     if (params->len)
         g_string_append_printf(s, "\n[Bond]%s\n", params->str);
diff --git a/tests/generate.py b/tests/generate.py
index 95abae2..572b978 100755
--- a/tests/generate.py
+++ b/tests/generate.py
@@ -981,10 +981,10 @@ unmanaged-devices+=interface-name:eth42,interface-name:eth43,interface-name:mybr
       dhcp4: true''')
 
         self.assert_networkd({'br0.netdev': '[NetDev]\nName=br0\nKind=bridge\n\n'
-                                            '[Bridge]\nAgeingTimeSec=50\n'
-                                            'ForwardDelaySec=12\n'
-                                            'HelloTimeSec=6\n'
-                                            'MaxAgeSec=24\n'
+                                            '[Bridge]\nAgeingTimeSec=50ms\n'
+                                            'ForwardDelaySec=12ms\n'
+                                            'HelloTimeSec=6ms\n'
+                                            'MaxAgeSec=24ms\n'
                                             'STP=true\n',
                               'br0.network': ND_DHCP4 % 'br0',
                               'eno1.network': '[Match]\nName=eno1\n\n'
@@ -1087,23 +1087,23 @@ unmanaged-devices+=interface-name:bn0,''')
                                             '[Bond]\n'
                                             'Mode=802.1ad\n'
                                             'LACPTransmitRate=10\n'
-                                            'MIIMonitorSec=10\n'
+                                            'MIIMonitorSec=10ms\n'
                                             'MinLinks=10\n'
                                             'TransmitHashPolicy=none\n'
                                             'AdSelect=none\n'
                                             'AllSlavesActive=1\n'
-                                            'ARPIntervalSec=10\n'
+                                            'ARPIntervalSec=10ms\n'
                                             'ARPIPTargets=10.10.10.10,20.20.20.20\n'
                                             'ARPValidate=all\n'
                                             'ARPAllTargets=all\n'
-                                            'UpDelaySec=10\n'
-                                            'DownDelaySec=10\n'
+                                            'UpDelaySec=10ms\n'
+                                            'DownDelaySec=10ms\n'
                                             'FailOverMACPolicy=none\n'
                                             'GratuitiousARP=10\n'
                                             'PacketsPerSlave=10\n'
                                             'PrimaryReselectPolicy=none\n'
                                             'ResendIGMP=10\n'
-                                            'LearnPacketIntervalSec=10\n',
+                                            'LearnPacketIntervalSec=10ms\n',
                               'bn0.network': ND_DHCP4 % 'bn0',
                               'eno1.network': '[Match]\nName=eno1\n\n'
                                               '[Network]\nBond=bn0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',