cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:fix/lxd-only-create-network-if-noexist into cloud-init:master.

Commit message:
lxd: Delete lxdbr0 network if it exists and is to be created.

Newer versions (3.0.1+) of lxd create the 'lxdbr0' network when
'lxd init --auto' is invoked.

When cloud-init is given a network configuration to pass on to
lxc and that config had no name specified or 'lxdbr0', then cloud-init
would fail to create the network as it already exists.

The change here is to check if it exists and delete it if it does.

LP: #1776958

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1776958 in cloud-init: "error creating lxdbr0."
  https://bugs.launchpad.net/cloud-init/+bug/1776958

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/348005

see commit message
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/lxd-only-create-network-if-noexist into cloud-init:master.

diff --git a/cloudinit/config/cc_lxd.py b/cloudinit/config/cc_lxd.py
index 09374d2..b6ee756 100644
--- a/cloudinit/config/cc_lxd.py
+++ b/cloudinit/config/cc_lxd.py
@@ -47,11 +47,16 @@ lxd-bridge will be configured accordingly.
             domain: <domain>
 """
 
+from cloudinit import log as logging
 from cloudinit import util
 import os
 
 distros = ['ubuntu']
 
+LOG = logging.getLogger(__name__)
+
+_DEFAULT_NETWORK_NAME = "lxdbr0"
+
 
 def handle(name, cfg, cloud, log, args):
     # Get config
@@ -134,6 +139,7 @@ def handle(name, cfg, cloud, log, args):
                        '--frontend=noninteractive'])
         else:
             # Built-in LXD bridge support
+            maybe_delete_network(bridge_cfg)
             cmd_create, cmd_attach = bridge_to_cmd(bridge_cfg)
             if cmd_create:
                 log.debug("Creating lxd bridge: %s" %
@@ -204,7 +210,7 @@ def bridge_to_cmd(bridge_cfg):
     if bridge_cfg.get("mode") == "none":
         return None, None
 
-    bridge_name = bridge_cfg.get("name", "lxdbr0")
+    bridge_name = bridge_cfg.get("name", _DEFAULT_NETWORK_NAME)
     cmd_create = []
     cmd_attach = ["lxc", "network", "attach-profile", bridge_name,
                   "default", "eth0", "--force-local"]
@@ -251,4 +257,47 @@ def bridge_to_cmd(bridge_cfg):
 
     return cmd_create, cmd_attach
 
+
+def _network_exists(name):
+    """Return boolean indicating if network exists."""
+    try:
+        _out, _err = util.subp(["lxc", "network", "show", name],
+                               update_env={'LC_ALL': 'C'})
+        return True
+    except util.ProcessExecutionError as e:
+        if e.exit_code != 1 or "not found" not in e.stderr.lower():
+            raise e
+        return False
+
+
+def _network_delete(name):
+    util.subp(["lxc", "network", "delete", name])
+
+
+def maybe_delete_network(cfg, default=None):
+    """Some versions of lxd init automatically create a lxdbr0.
+    If cfg specified to create that name, then we need to delete it
+    so that the creation will work.  If cfg specified a name other
+    than default, or mode == existing, then do not do anything.
+
+    https://github.com/lxc/lxd/issues/4649
+    """
+
+    if default is None:
+        default = _DEFAULT_NETWORK_NAME
+
+    if cfg.get("mode", "new") == "existing":
+        return
+
+    name = cfg.get("name", default)
+    if name != default:
+        return
+
+    if not _network_exists(name):
+        return
+
+    LOG.debug("Removing lxd network '%s'", name)
+    _network_delete(name)
+
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_handler/test_handler_lxd.py b/tests/unittests/test_handler/test_handler_lxd.py
index a205498..02b0c8d 100644
--- a/tests/unittests/test_handler/test_handler_lxd.py
+++ b/tests/unittests/test_handler/test_handler_lxd.py
@@ -4,6 +4,7 @@ from cloudinit.config import cc_lxd
 from cloudinit.sources import DataSourceNoCloud
 from cloudinit import (distros, helpers, cloud)
 from cloudinit.tests import helpers as t_help
+from cloudinit.util import ProcessExecutionError
 
 try:
     from unittest import mock
@@ -33,12 +34,16 @@ class TestLxd(t_help.CiTestCase):
         cc = cloud.Cloud(ds, paths, {}, d, None)
         return cc
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_delete_network")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_lxd_init(self, mock_util):
+    def test_lxd_init(self, mock_util, m_maybe_del):
         cc = self._get_cloud('ubuntu')
         mock_util.which.return_value = True
+        m_maybe_del.return_value = None
         cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, [])
         self.assertTrue(mock_util.which.called)
+        # no bridge config, so maybe_delete should not be called.
+        self.assertFalse(m_maybe_del.called)
         init_call = mock_util.subp.call_args_list[0][0][0]
         self.assertEqual(init_call,
                          ['lxd', 'init', '--auto',
@@ -46,32 +51,39 @@ class TestLxd(t_help.CiTestCase):
                           '--storage-backend=zfs',
                           '--storage-pool=poolname'])
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_delete_network")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_lxd_install(self, mock_util):
+    def test_lxd_install(self, mock_util, m_maybe_del):
         cc = self._get_cloud('ubuntu')
         cc.distro = mock.MagicMock()
         mock_util.which.return_value = None
         cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, [])
         self.assertNotIn('WARN', self.logs.getvalue())
         self.assertTrue(cc.distro.install_packages.called)
+        cc_lxd.handle('cc_lxd', self.lxd_cfg, cc, self.logger, [])
+        self.assertFalse(m_maybe_del.called)
         install_pkg = cc.distro.install_packages.call_args_list[0][0][0]
         self.assertEqual(sorted(install_pkg), ['lxd', 'zfs'])
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_delete_network")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_no_init_does_nothing(self, mock_util):
+    def test_no_init_does_nothing(self, mock_util, m_maybe_del):
         cc = self._get_cloud('ubuntu')
         cc.distro = mock.MagicMock()
         cc_lxd.handle('cc_lxd', {'lxd': {}}, cc, self.logger, [])
         self.assertFalse(cc.distro.install_packages.called)
         self.assertFalse(mock_util.subp.called)
+        self.assertFalse(m_maybe_del.called)
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_delete_network")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_no_lxd_does_nothing(self, mock_util):
+    def test_no_lxd_does_nothing(self, mock_util, m_maybe_del):
         cc = self._get_cloud('ubuntu')
         cc.distro = mock.MagicMock()
         cc_lxd.handle('cc_lxd', {'package_update': True}, cc, self.logger, [])
         self.assertFalse(cc.distro.install_packages.called)
         self.assertFalse(mock_util.subp.called)
+        self.assertFalse(m_maybe_del.called)
 
     def test_lxd_debconf_new_full(self):
         data = {"mode": "new",
@@ -183,4 +195,81 @@ class TestLxd(t_help.CiTestCase):
             cc_lxd.bridge_to_cmd(data),
             (None, None))
 
+
+class TestLxdMaybeDeleteNetwork(t_help.CiTestCase):
+    """Test the implementation of maybe_delete_network."""
+
+    defnet = cc_lxd._DEFAULT_NETWORK_NAME
+
+    @mock.patch("cloudinit.config.cc_lxd._network_exists", return_value=True)
+    @mock.patch("cloudinit.config.cc_lxd._network_delete")
+    def test_network_other_than_default_not_deleted(self, m_delete, m_exists):
+        """deletion should only occur if bridge is default."""
+        cc_lxd.maybe_delete_network({"name": "lxdbr1"})
+        # do not make assertions on m_exists as that call is optimized out.
+        m_delete.assert_not_called()
+
+    @mock.patch("cloudinit.config.cc_lxd._network_exists", return_value=True)
+    @mock.patch("cloudinit.config.cc_lxd._network_delete")
+    def test_mode_existing_should_not_delete(self, m_delete, m_exists):
+        """deletion should not occur if mode == existing."""
+        cc_lxd.maybe_delete_network({"name": "lxdbr1", "mode": "existing"})
+        # do not make assertions on m_exists as that call is optimized out.
+        m_delete.assert_not_called()
+
+    @mock.patch("cloudinit.config.cc_lxd._network_exists", return_value=True)
+    @mock.patch("cloudinit.config.cc_lxd._network_delete")
+    def test_delete_if_default_exists(self, m_delete, m_exists):
+        """deletion should occur if network existed."""
+        cc_lxd.maybe_delete_network({"name": self.defnet})
+        m_exists.assert_called_once_with(self.defnet)
+        m_delete.assert_called_once_with(self.defnet)
+
+    @mock.patch("cloudinit.config.cc_lxd._network_exists", return_value=False)
+    @mock.patch("cloudinit.config.cc_lxd._network_delete")
+    def test_delete_only_if_default_exists(self, m_delete, m_exists):
+        """deletion should not occur if network did not exist."""
+        cc_lxd.maybe_delete_network({"name": self.defnet})
+        m_exists.assert_called_once_with(self.defnet)
+        m_delete.assert_not_called()
+
+    @mock.patch("cloudinit.config.cc_lxd.util.subp")
+    def test_network_delete_calls_lxc_correctly(self, name, m_subp):
+        """Test _network_delete function operates correctly."""
+        m_subp.return_value = (b'', b'')
+        netname = "foo"
+        cc_lxd._network_delete(netname)
+        m_subp.assert_called_once_with(["lxc", "network", "delete", netname])
+
+    @mock.patch("cloudinit.config.cc_lxd.util.subp")
+    def test_network_exists_when_network_exists(self, m_subp):
+        """Test _network_exists exists path."""
+        m_subp.return_value = ('not important\nwould be yaml\n', '')
+        ret = cc_lxd._network_exists("thisexists")
+        self.assertTrue(m_subp.called)
+        self.assertTrue(ret)
+
+    @mock.patch("cloudinit.config.cc_lxd.util.subp")
+    def test_network_exists_when_network_does_not_exist(self, m_subp):
+        """Test _network_exists when network does not exist."""
+        # lxc network info foo is expected to return 1 and
+        # write 'Error: not found' to stderr.
+        m_subp.side_effect = ProcessExecutionError(
+            stdout="", stderr="Error: not found", exit_code=1)
+        ret = cc_lxd._network_exists("foonet")
+        self.assertTrue(m_subp.called)
+        self.assertFalse(ret)
+
+    @mock.patch("cloudinit.config.cc_lxd.util.subp")
+    def test_network_exists_error_path(self, m_subp):
+        """Test _network_exists with unexpected subp error raises."""
+        myexc = ProcessExecutionError(
+            stdout="", stderr="Error: unexpected.", exit_code=2)
+        m_subp.side_effect = myexc
+        with self.assertRaises(ProcessExecutionError) as e:
+            cc_lxd._network_exists("foonet")
+        self.assertEqual(myexc, e.exception)
+        self.assertTrue(m_subp.called)
+
+
 # vi: ts=4 expandtab