diff --git a/charmhelpers/__init__.py b/charmhelpers/__init__.py index 61ef90719..1f57ed2ad 100644 --- a/charmhelpers/__init__.py +++ b/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): diff --git a/charmhelpers/contrib/network/ovs/__init__.py b/charmhelpers/contrib/network/ovs/__init__.py index 87e6af611..acb503be7 100644 --- a/charmhelpers/contrib/network/ovs/__init__.py +++ b/charmhelpers/contrib/network/ovs/__init__.py @@ -14,10 +14,12 @@ ''' 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 @@ -25,6 +27,8 @@ 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 ' can't be replaced by + # 'ip link set 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'): diff --git a/tests/contrib/network/test_ovs.py b/tests/contrib/network/test_ovs.py index 9208499b8..221e87a19 100644 --- a/tests/contrib/network/test_ovs.py +++ b/tests/contrib/network/test_ovs.py @@ -165,30 +165,37 @@ 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() @@ -196,10 +203,14 @@ def test_add_ovsbridge_linuxbridge_already_direct_wired(self, @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)