Clean-up around adding linux bridges to OVS bridges (juju#449)

* Fix typo

* Mark OVS ports as managed by us

Before we used to mark only OVS interfaces

* add_ovsbridge_linuxbridge fixes

1. Fix missing 'ifup' on bionic.
2. Mark the OVS port also when upgrading the charm.

* Fix typos

* Use deprecate decorator

* Don't install ifupdown automatically

* Make dependency explicit

* Deprecate adding a linux bridge to an openvswitch bridge

* Minor improvements

* Make linter happy

* Raise on focal+

charmhelpers/__init__.py

@@ -49,7 +49,8 @@
 
 def deprecate(warning, date=None, log=None):
     """Add a deprecation warning the first time the function is used.
-    The date, which is a string in semi-ISO8660 format indicate the year-month
+
+    The date which is a string in semi-ISO8660 format indicates the year-month
     that the function is officially going to be removed.
 
     usage:
@@ -62,10 +63,11 @@ def contributed_add_source_thing(...):
     The reason for passing the logging function (log) is so that hookenv.log
     can be used for a charm if needed.
 
-    :param warning:  String to indicat where it has moved ot.
-    :param date: optional sting, in YYYY-MM format to indicate when the
+    :param warning: String to indicate what is to be used instead.
+    :param date: Optional string in YYYY-MM format to indicate when the
                  function will definitely (probably) be removed.
-    :param log: The log function to call to log.  If not, logs to stdout
+    :param log: The log function to call in order to log. If None, logs to
+                stdout
     """
     def wrap(f):
 

charmhelpers/contrib/network/ovs/__init__.py

@@ -14,17 +14,21 @@
 
 ''' Helpers for interacting with OpenvSwitch '''
 import hashlib
-import subprocess
 import os
+import re
 import six
+import subprocess
 
+from charmhelpers import deprecate
 from charmhelpers.fetch import apt_install
 
 
 from charmhelpers.core.hookenv import (
     log, WARNING, INFO, DEBUG
 )
 from charmhelpers.core.host import (
+    CompareHostReleases,
+    lsb_release,
     service
 )
 
@@ -318,7 +322,8 @@ def add_bridge_bond(bridge, port, interfaces, portdata=None, ifdatamap=None,
     subprocess.check_call(cmd)
 
 
-def add_ovsbridge_linuxbridge(name, bridge, ifdata=None):
+@deprecate('see lp:1877594', '2021-01', log=log)
+def add_ovsbridge_linuxbridge(name, bridge, ifdata=None, portdata=None):
     """Add linux bridge to the named openvswitch bridge
 
     :param name: Name of ovs bridge to be added to Linux bridge
@@ -346,6 +351,17 @@ def add_ovsbridge_linuxbridge(name, bridge, ifdata=None):
                20dac08fdcce4b7fda1d07add3b346aa9751cfbc/
                    lib/db-ctl-base.c#L189-L215
     :type ifdata: Optional[Dict[str,Union[str,Dict[str,str]]]]
+    :param portdata: Additional data to attach to port. Similar to ifdata.
+    :type portdata: Optional[Dict[str,Union[str,Dict[str,str]]]]
+
+    WARNINGS:
+    * The `ifup` command (NetworkManager) must be available on the system for
+      this to work. Before bionic this was shipped by default. On bionic and
+      newer you need to install the package `ifupdown`. This might however cause
+      issues when deploying to LXD, see lp:1877594, which is why this function
+      isn't supported anymore.
+    * On focal and newer this function won't even try to run `ifup` and raise
+      directly.
     """
     try:
         import netifaces
@@ -380,25 +396,42 @@ def add_ovsbridge_linuxbridge(name, bridge, ifdata=None):
         ovsbridge_port = "cvo{}".format(base)
         linuxbridge_port = "cvb{}".format(base)
 
+    network_interface_already_exists = False
     interfaces = netifaces.interfaces()
     for interface in interfaces:
         if interface == ovsbridge_port or interface == linuxbridge_port:
             log('Interface {} already exists'.format(interface), level=INFO)
-            return
+            network_interface_already_exists = True
+            break
 
     log('Adding linuxbridge {} to ovsbridge {}'.format(bridge, name),
         level=INFO)
 
-    check_for_eni_source()
-
-    with open('/etc/network/interfaces.d/{}.cfg'.format(
-            linuxbridge_port), 'w') as config:
-        config.write(BRIDGE_TEMPLATE.format(linuxbridge_port=linuxbridge_port,
-                                            ovsbridge_port=ovsbridge_port,
-                                            bridge=bridge))
-
-    subprocess.check_call(["ifup", linuxbridge_port])
-    add_bridge_port(name, linuxbridge_port, ifdata=ifdata)
+    if not network_interface_already_exists:
+        setup_eni()  # will raise on focal+
+
+        with open('/etc/network/interfaces.d/{}.cfg'.format(
+                linuxbridge_port), 'w') as config:
+            config.write(BRIDGE_TEMPLATE.format(
+                linuxbridge_port=linuxbridge_port,
+                ovsbridge_port=ovsbridge_port, bridge=bridge))
+
+        try:
+            # NOTE(lourot): 'ifup <name>' can't be replaced by
+            # 'ip link set <name> up' as the latter won't parse
+            # /etc/network/interfaces*
+            subprocess.check_call(['ifup', linuxbridge_port])
+        except FileNotFoundError:
+            # NOTE(lourot): on bionic and newer, 'ifup' isn't installed by
+            # default. It has been replaced by netplan.io but we can't use it
+            # yet because of lp:1876730. For the time being, charms using this
+            # have to install 'ifupdown' on bionic and newer. This will however
+            # cause issues when deploying to LXD, see lp:1877594.
+            raise RuntimeError('ifup: command not found. Did this charm forget '
+                               'to install ifupdown?')
+
+    add_bridge_port(name, linuxbridge_port, ifdata=ifdata, exclusive=False,
+                    portdata=portdata)
 
 
 def is_linuxbridge_interface(port):
@@ -458,18 +491,35 @@ def get_certificate():
         return None
 
 
-def check_for_eni_source():
-    ''' Juju removes the source line when setting up interfaces,
-    replace if missing '''
+@deprecate('see lp:1877594', '2021-01', log=log)
+def setup_eni():
+    """Makes sure /etc/network/interfaces.d/ exists and will be parsed.
+
+    When setting up interfaces, Juju removes from
+    /etc/network/interfaces the line sourcing interfaces.d/
+
+    WARNING: Not supported on focal and newer anymore. Will raise.
+    """
+    release = CompareHostReleases(lsb_release()['DISTRIB_CODENAME'])
+    if release >= 'focal':
+        raise RuntimeError("NetworkManager isn't supported anymore")
 
+    if not os.path.exists('/etc/network/interfaces.d'):
+        os.makedirs('/etc/network/interfaces.d', mode=0o755)
     with open('/etc/network/interfaces', 'r') as eni:
         for line in eni:
-            if line == 'source /etc/network/interfaces.d/*':
+            if re.search(r'^\s*source\s+/etc/network/interfaces.d/\*\s*$',
+                         line):
                 return
     with open('/etc/network/interfaces', 'a') as eni:
         eni.write('\nsource /etc/network/interfaces.d/*')
 
 
+@deprecate('use setup_eni() instead', '2021-01', log=log)
+def check_for_eni_source():
+    setup_eni()
+
+
 def full_restart():
     ''' Full restart and reload of openvswitch '''
     if os.path.exists('/etc/init/openvswitch-force-reload-kmod.conf'):

tests/contrib/network/test_ovs.py

@@ -165,41 +165,52 @@ def test_del_bridge(self, check_call):
 
     @patch.object(ovs, 'port_to_br')
     @patch.object(ovs, 'add_bridge_port')
+    @patch.object(ovs, 'lsb_release')
+    @patch('os.path.exists')
     @patch('subprocess.check_call')
-    def test_add_ovsbridge_linuxbridge(self, check_call,
+    def test_add_ovsbridge_linuxbridge(self, check_call, exists, lsb_release,
                                        add_bridge_port,
                                        port_to_br):
+        exists.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'}
         port_to_br.return_value = None
+        if_and_port_data = {
+            'external-ids': {'mycharm': 'br-ex'}
+        }
         with patch_open() as (mock_open, mock_file):
-            ovs.add_ovsbridge_linuxbridge('br-ex', 'br-eno1', ifdata={
-                'external-ids': {'mycharm': 'br-ex'}
-            })
+            ovs.add_ovsbridge_linuxbridge(
+                'br-ex', 'br-eno1', ifdata=if_and_port_data,
+                portdata=if_and_port_data)
 
         check_call.assert_called_with(['ifup', 'veth-br-eno1'])
         add_bridge_port.assert_called_with(
-            'br-ex', 'veth-br-eno1', ifdata={
-                'external-ids': {'mycharm': 'br-ex'}
-            }
-        )
+            'br-ex', 'veth-br-eno1', ifdata=if_and_port_data, exclusive=False,
+            portdata=if_and_port_data)
 
     @patch.object(ovs, 'port_to_br')
     @patch.object(ovs, 'add_bridge_port')
+    @patch.object(ovs, 'lsb_release')
+    @patch('os.path.exists')
     @patch('subprocess.check_call')
-    def test_add_ovsbridge_linuxbridge_already_direct_wired(self,
-                                                            check_call,
-                                                            add_bridge_port,
-                                                            port_to_br):
+    def test_add_ovsbridge_linuxbridge_already_direct_wired(
+            self, check_call, exists, lsb_release, add_bridge_port, port_to_br):
+        exists.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'}
         port_to_br.return_value = 'br-ex'
         ovs.add_ovsbridge_linuxbridge('br-ex', 'br-eno1')
         check_call.assert_not_called()
         add_bridge_port.assert_not_called()
 
     @patch.object(ovs, 'port_to_br')
     @patch.object(ovs, 'add_bridge_port')
+    @patch.object(ovs, 'lsb_release')
+    @patch('os.path.exists')
     @patch('subprocess.check_call')
-    def test_add_ovsbridge_linuxbridge_longname(self, check_call,
-                                                add_bridge_port,
+    def test_add_ovsbridge_linuxbridge_longname(self, check_call, exists,
+                                                lsb_release, add_bridge_port,
                                                 port_to_br):
+        exists.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'}
         port_to_br.return_value = None
         mock_hasher = MagicMock()
         mock_hasher.hexdigest.return_value = '12345678901234578910'
@@ -209,8 +220,8 @@ def test_add_ovsbridge_linuxbridge_longname(self, check_call,
 
         check_call.assert_called_with(['ifup', 'cvb12345678-10'])
         add_bridge_port.assert_called_with(
-            'br-ex', 'cvb12345678-10', ifdata=None
-        )
+            'br-ex', 'cvb12345678-10', ifdata=None, exclusive=False,
+            portdata=None)
 
     @patch('os.path.exists')
     def test_is_linuxbridge_interface_false(self, exists):
@@ -320,3 +331,38 @@ def test_disable_ipfix(self, check_call):
         check_call.assert_called_once_with(
             ['ovs-vsctl', 'clear', 'Bridge', 'br-int', 'ipfix']
         )
+
+    @patch.object(ovs, 'lsb_release')
+    @patch('os.path.exists')
+    def test_setup_eni_sources_eni_folder(self, exists, lsb_release):
+        exists.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'}
+        with patch_open() as (_, mock_file):
+            # Mocked initial /etc/network/interfaces file content:
+            mock_file.__iter__.return_value = [
+                'some line',
+                'some other line']
+
+            ovs.setup_eni()
+            mock_file.write.assert_called_once_with(
+                '\nsource /etc/network/interfaces.d/*')
+
+    @patch.object(ovs, 'lsb_release')
+    @patch('os.path.exists')
+    def test_setup_eni_wont_source_eni_folder_twice(self, exists, lsb_release):
+        exists.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'}
+        with patch_open() as (_, mock_file):
+            # Mocked initial /etc/network/interfaces file content:
+            mock_file.__iter__.return_value = [
+                'some line',
+                '  source    /etc/network/interfaces.d/*   ',
+                'some other line']
+
+            ovs.setup_eni()
+            self.assertFalse(mock_file.write.called)
+
+    @patch.object(ovs, 'lsb_release')
+    def test_setup_eni_raises_on_focal(self, lsb_release):
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'focal'}
+        self.assertRaises(RuntimeError, ovs.setup_eni)