Merge lp:~hexta/curtin/fix-static-routes-with-multiple-interfaces into lp:~curtin-dev/curtin/trunk

[Artur Molchanov]

Currently, curtin creates /etc/network/interfaces with all static routes passed after last interface entry. As a result, all routes will be added only after the last interface turned on.

Example /etc/network/interfaces:
auto lo
iface lo inet loopback
    dns-search maas synesis
    dns-nameservers 10.1.254.254

auto boot
iface boot inet static
    gateway 10.1.255.254
    dns-nameservers 10.1.254.254
    address 10.1.2.255/16
    mtu 9000

auto private
iface private inet static
    address 10.7.0.201/16
    mtu 9000

auto public
iface public inet static
    dns-nameservers 10.1.254.254
    address 10.9.0.201/16
    mtu 9000

auto virt-inet
iface virt-inet inet static
    address 10.88.0.1/16
    mtu 1500
post-up route add -net 10.8.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
pre-down route del -net 10.8.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
post-up route add -net 192.168.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
pre-down route del -net 192.168.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
post-up route add -net 10.13.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
pre-down route del -net 10.13.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
post-up route add -net 10.4.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
pre-down route del -net 10.4.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
post-up route add -net 172.20.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true
pre-down route del -net 172.20.0.0 netmask 255.255.0.0 gw 10.9.255.254 metric 0 || true

source /etc/network/interfaces.d/*.cfg

All static routes applied to 'virt-inet' interface section. Restarting interface 'public' will not trigger adding routes.

[lp:~hexta/curtin/fix-static-routes-with-multiple-interfaces]

472. By Artur Molchanov <email address hidden> on 2017-03-02

Remove unused 'pprint' module

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:472
https://jenkins.ubuntu.com/server/job/curtin-ci/402/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/402
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/402
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/402
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/402
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/402

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/402/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:472
https://jenkins.ubuntu.com/server/job/curtin-ci/403/
Executed test runs:
    ABORTED: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/403/console
    ABORTED: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/403/console
    ABORTED: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/403/console
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/403
    ABORTED: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/403/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/403/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:472
https://jenkins.ubuntu.com/server/job/curtin-ci/410/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/410
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/410
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/410
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/410
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/410

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/410/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Hi, can you share your curtin network configuration?

maas <session> machine get-curtin-config <system-id>

[Artur Molchanov (hexta)]

> Hi, can you share your curtin network configuration?
>
> maas <session> machine get-curtin-config <system-id>

network:
  config:
  - id: boot
    mac_address: 0c:c4:7a:53:4d:6c
    mtu: 9000
    name: boot
    subnets:
    - address: 10.1.2.255/16
      dns_nameservers:
      - 10.1.254.254
      gateway: 10.1.255.254
      type: static
    type: physical
  - id: private
    mac_address: a0:36:9f:c6:37:c0
    mtu: 9000
    name: private
    subnets:
    - address: 10.7.0.201/16
      dns_nameservers: []
      type: static
    type: physical
  - id: public
    mac_address: a0:36:9f:c6:37:c2
    mtu: 9000
    name: public
    subnets:
    - address: 10.9.0.201/16
      dns_nameservers:
      - 10.1.254.254
      type: static
    type: physical
  - id: virt-inet
    mac_address: 0c:c4:7a:53:4d:6d
    mtu: 1500
    name: virt-inet
    subnets:
    - address: 10.88.0.1/16
      dns_nameservers: []
      type: static
    type: physical
  - destination: 10.8.0.0/16
    gateway: 10.9.255.254
    id: 8
    metric: 0
    type: route
  - destination: 172.20.0.0/16
    gateway: 10.9.255.254
    id: 9
    metric: 0
    type: route
  - destination: 192.168.0.0/16
    gateway: 10.9.255.254
    id: 11
    metric: 0
    type: route
  - destination: 10.13.0.0/16
    gateway: 10.9.255.254
    id: 12
    metric: 0
    type: route
  - destination: 10.4.0.0/16
    gateway: 10.9.255.254
    id: 21
    metric: 0
    type: route
  - address:
    - 10.1.254.254
    search:
    - maas
    - BY
    - SNS
    - synesis
    type: nameserver
  version: 1

[Artur Molchanov (hexta)]

ping

[Scott Moser (smoser)]

Hi,
sorry for being so slow to respond.
Youv'e done a good job of explaining the bug in the current implementation and explaining your fix.

Ryan and I have talked about this before... there isn't really a great way to solve it, and your solution may well be as good as any.

Another option would be to look through each of these "global" routes and find which subnet would have a route to the gateway and put it there.

Another complexity is that right now we basically have ENI rendering bits of code (cloud-init and curtin), with a goal of getting rid of that in curtin... at some point curtin will be able to just "pass through" the configuration to cloud-init and let it render it.

So your branch hasn't been completely ignored. Sorry for the crickets.

[Scott Moser (smoser)]

Hi Arthur,
I'm sorry that this has never really gotten looked at.
If you still are having a problem with it, could you re-submit via git?

I'm going to mark this 'Rejected' as no other state really makes sense since we've mvoed from bzr.

[Scott Moser (smoser)]

decided to move it to work in progress.

Unmerged revisions

472. By Artur Molchanov <email address hidden> on 2017-03-02

Remove unused 'pprint' module

471. By Artur Molchanov <email address hidden> on 2017-03-02

Fix static routes in case of multiple interfaces.

Currently, curtin creates /etc/network/interfaces with all static routes passed
after last interface entry. As a result, all routes will be added only after the
last interface turned on.

Changes:
- Create script /etc/network/if-up.d/routes.
  This script will invoke 'route' command with
  appropriate arguments after any interface turned on.
- Get rid of adding static routes inside /etc/network/interfaces.

=== modified file 'curtin/net/__init__.py'
--- curtin/net/__init__.py	2017-02-06 20:58:37 +0000
+++ curtin/net/__init__.py	2017-03-02 16:03:09 +0000
@@ -377,47 +377,41 @@
     return content
 
 
-def render_route(route, indent=""):
-    """When rendering routes for an iface, in some cases applying a route
-    may result in the route command returning non-zero which produces
-    some confusing output for users manually using ifup/ifdown[1].  To
-    that end, we will optionally include an '|| true' postfix to each
-    route line allowing users to work with ifup/ifdown without using
-    --force option.
-
-    We may at somepoint not want to emit this additional postfix, and
-    add a 'strict' flag to this function.  When called with strict=True,
-    then we will not append the postfix.
-
-    1. http://askubuntu.com/questions/168033/
-             how-to-set-static-routes-in-ubuntu-server
-    """
-    content = []
-    up = indent + "post-up route add"
-    down = indent + "pre-down route del"
-    or_true = " || true"
-    mapping = {
-        'network': '-net',
-        'netmask': 'netmask',
-        'gateway': 'gw',
-        'metric': 'metric',
-    }
-    if route['network'] == '0.0.0.0' and route['netmask'] == '0.0.0.0':
-        default_gw = " default gw %s" % route['gateway']
-        content.append(up + default_gw + or_true)
-        content.append(down + default_gw + or_true)
-    elif route['network'] == '::' and route['netmask'] == 0:
-        # ipv6!
-        default_gw = " -A inet6 default gw %s" % route['gateway']
-        content.append(up + default_gw + or_true)
-        content.append(down + default_gw + or_true)
-    else:
-        route_line = ""
-        for k in ['network', 'netmask', 'gateway', 'metric']:
-            if k in route:
-                route_line += " %s %s" % (mapping[k], route[k])
-        content.append(up + route_line + or_true)
-        content.append(down + route_line + or_true)
+def render_routes(network_state, indent=""):
+    content = ['#!/bin/sh', '']
+
+    def render_route(route):
+        up = indent + "route add"
+        mapping = {
+            'network': '-net',
+            'netmask': 'netmask',
+            'gateway': 'gw',
+            'metric': 'metric',
+        }
+        if route['network'] == '0.0.0.0' and route['netmask'] == '0.0.0.0':
+            default_gw = " default gw %s" % route['gateway']
+            content.append(up + default_gw)
+        elif route['network'] == '::' and route['netmask'] == 0:
+            # ipv6!
+            default_gw = " -A inet6 default gw %s" % route['gateway']
+            content.append(up + default_gw)
+        else:
+            route_line = ""
+            for k in ['network', 'netmask', 'gateway', 'metric']:
+                if k in route:
+                    route_line += " %s %s" % (mapping[k], route[k])
+            content.append(up + route_line)
+
+    for route in network_state.get('routes'):
+        render_route(route)
+
+    for iface in network_state.get('interfaces', {}).values():
+        for subnet in iface.get('subnets', []):
+            for route in subnet.get('routes', []):
+                render_route(route)
+
+    content.append('exit 0')
+
     return "\n".join(content) + "\n"
 
 
@@ -498,9 +492,6 @@
                 content += iface_add_subnet(iface, subnet)
                 content += iface_add_attrs(iface, index)
 
-                for route in subnet.get('routes', []):
-                    content += render_route(route, indent="    ") + '\n'
-
         else:
             # ifenslave docs say to auto the slave devices
             if 'bond-master' in iface or 'bond-slaves' in iface:
@@ -508,9 +499,6 @@
             content += "iface {name} {inet} {mode}\n".format(**iface)
             content += iface_add_attrs(iface, 0)
 
-    for route in network_state.get('routes'):
-        content += render_route(route)
-
     # global replacements until v2 format
     content = content.replace('mac_address', 'hwaddress ether')
 
@@ -524,6 +512,7 @@
     eni = 'etc/network/interfaces'
     netrules = 'etc/udev/rules.d/70-persistent-net.rules'
     cc = 'etc/cloud/cloud.cfg.d/curtin-disable-cloudinit-networking.cfg'
+    routes_file = 'etc/network/if-up.d/routes'
 
     eni = os.path.sep.join((target, eni,))
     LOG.info('Writing ' + eni)
@@ -537,6 +526,10 @@
     LOG.info('Writing ' + cc_disable)
     util.write_file(cc_disable, content='network: {config: disabled}\n')
 
+    routes_file = os.path.sep.join((target, routes_file,))
+    LOG.info('Writing ' + routes_file)
+    util.write_file(routes_file, content=render_routes(network_state), mode=0o755)
+
 
 def get_interface_mac(ifname):
     """Returns the string value of an interface's MAC Address"""

=== modified file 'tests/unittests/test_net.py'
--- tests/unittests/test_net.py	2017-02-09 16:58:23 +0000
+++ tests/unittests/test_net.py	2017-03-02 16:03:09 +0000
@@ -680,18 +680,7 @@
             'iface eth0 inet static',
             '    address 172.23.31.42/26',
             '    gateway 172.23.31.2',
-            ('post-up route add -net 10.0.0.0 netmask 255.240.0.0 gw '
-             '172.23.31.1 metric 0 || true'),
-            ('pre-down route del -net 10.0.0.0 netmask 255.240.0.0 gw '
-             '172.23.31.1 metric 0 || true'),
-            ('post-up route add -net 192.168.2.0 netmask 255.255.255.0 gw '
-             '172.23.31.1 metric 0 || true'),
-            ('pre-down route del -net 192.168.2.0 netmask 255.255.255.0 gw '
-             '172.23.31.1 metric 0 || true'),
-            ('post-up route add -net 10.0.200.0 netmask 255.255.0.0 gw '
-             '172.23.31.1 metric 1 || true'),
-            ('pre-down route del -net 10.0.200.0 netmask 255.255.0.0 gw '
-             '172.23.31.1 metric 1 || true'),
+            '',
             'source /etc/network/interfaces.d/*.cfg',
         ]
 
@@ -702,6 +691,23 @@
             sorted([line for line in expected if line]),
             sorted([line for line in found if line]))
 
+        expected = [
+            '#!/bin/sh',
+            '',
+            ('route add -net 10.0.0.0 netmask 255.240.0.0 gw '
+             '172.23.31.1 metric 0'),
+            ('route add -net 192.168.2.0 netmask 255.255.255.0 gw '
+             '172.23.31.1 metric 0'),
+            ('route add -net 10.0.200.0 netmask 255.255.0.0 gw '
+             '172.23.31.1 metric 1'),
+            'exit 0',
+        ]
+
+        found = net.render_routes(ns.network_state).split('\n')
+        self.assertEqual(
+            sorted([line for line in expected if line]),
+            sorted([line for line in found if line]))
+
     def test_render_interfaces_bridges(self):
         bridge_config = open(
             'examples/tests/bridging_network.yaml', 'r').read()