lxd: Delete default network and detach device if lxd-init created them.

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.

Similarly, we need to remove the device from the default profile
so that the attach code can work.

Also, add a _lxc method and use it to make sure we're getting the
--force-local flag everywhere.

LP: #1776958

2 files changed, 120 insertions, 24 deletions

diff --git a/cloudinit/config/cc_lxd.py b/cloudinit/config/cc_lxd.py
index 09374d2e..ac72ac4a 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
@@ -109,6 +114,7 @@ def handle(name, cfg, cloud, log, args):
     # Set up lxd-bridge if bridge config is given
     dconf_comm = "debconf-communicate"
     if bridge_cfg:
+        net_name = bridge_cfg.get("name", _DEFAULT_NETWORK_NAME)
         if os.path.exists("/etc/default/lxd-bridge") \
                 and util.which(dconf_comm):
             # Bridge configured through packaging
@@ -135,15 +141,18 @@ def handle(name, cfg, cloud, log, args):
         else:
             # Built-in LXD bridge support
             cmd_create, cmd_attach = bridge_to_cmd(bridge_cfg)
+            maybe_cleanup_default(
+                net_name=net_name, did_init=bool(init_cfg),
+                create=bool(cmd_create), attach=bool(cmd_attach))
             if cmd_create:
                 log.debug("Creating lxd bridge: %s" %
                           " ".join(cmd_create))
-                util.subp(cmd_create)
+                _lxc(cmd_create)
 
             if cmd_attach:
                 log.debug("Setting up default lxd bridge: %s" %
                           " ".join(cmd_create))
-                util.subp(cmd_attach)
+                _lxc(cmd_attach)
 
     elif bridge_cfg:
         raise RuntimeError(
@@ -204,10 +213,10 @@ 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"]
+    cmd_attach = ["network", "attach-profile", bridge_name,
+                  "default", "eth0"]
 
     if bridge_cfg.get("mode") == "existing":
         return None, cmd_attach
@@ -215,7 +224,7 @@ def bridge_to_cmd(bridge_cfg):
     if bridge_cfg.get("mode") != "new":
         raise Exception("invalid bridge mode \"%s\"" % bridge_cfg.get("mode"))
 
-    cmd_create = ["lxc", "network", "create", bridge_name]
+    cmd_create = ["network", "create", bridge_name]
 
     if bridge_cfg.get("ipv4_address") and bridge_cfg.get("ipv4_netmask"):
         cmd_create.append("ipv4.address=%s/%s" %
@@ -247,8 +256,47 @@ def bridge_to_cmd(bridge_cfg):
     if bridge_cfg.get("domain"):
         cmd_create.append("dns.domain=%s" % bridge_cfg.get("domain"))
 
-    cmd_create.append("--force-local")
-
     return cmd_create, cmd_attach
 
+
+def _lxc(cmd):
+    env = {'LC_ALL': 'C'}
+    util.subp(['lxc'] + list(cmd) + ["--force-local"], update_env=env)
+
+
+def maybe_cleanup_default(net_name, did_init, create, attach,
+                          profile="default", nic_name="eth0"):
+    """Newer versions of lxc (3.0.1+) create a lxdbr0 network when
+    'lxd init --auto' is run.  Older versions did not.
+
+    By removing ay that lxd-init created, we simply leave the add/attach
+    code in-tact.
+
+    https://github.com/lxc/lxd/issues/4649"""
+    if net_name != _DEFAULT_NETWORK_NAME or not did_init:
+        return
+
+    fail_assume_enoent = " failed. Assuming it did not exist."
+    succeeded = " succeeded."
+    if create:
+        msg = "Deletion of lxd network '%s'" % net_name
+        try:
+            _lxc(["network", "delete", net_name])
+            LOG.debug(msg + succeeded)
+        except util.ProcessExecutionError as e:
+            if e.exit_code != 1:
+                raise e
+            LOG.debug(msg + fail_assume_enoent)
+
+    if attach:
+        msg = "Removal of device '%s' from profile '%s'" % (nic_name, profile)
+        try:
+            _lxc(["profile", "device", "remove", profile, nic_name])
+            LOG.debug(msg + succeeded)
+        except util.ProcessExecutionError as e:
+            if e.exit_code != 1:
+                raise e
+            LOG.debug(msg + fail_assume_enoent)
+
+
 # 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 a2054980..4dd7e09f 100644
--- a/tests/unittests/test_handler/test_handler_lxd.py
+++ b/tests/unittests/test_handler/test_handler_lxd.py
@@ -33,12 +33,16 @@ class TestLxd(t_help.CiTestCase):
         cc = cloud.Cloud(ds, paths, {}, d, None)
         return cc
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_lxd_init(self, mock_util):
+    def test_lxd_init(self, mock_util, m_maybe_clean):
         cc = self._get_cloud('ubuntu')
         mock_util.which.return_value = True
+        m_maybe_clean.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_cleanup should not be called.
+        self.assertFalse(m_maybe_clean.called)
         init_call = mock_util.subp.call_args_list[0][0][0]
         self.assertEqual(init_call,
                          ['lxd', 'init', '--auto',
@@ -46,32 +50,39 @@ class TestLxd(t_help.CiTestCase):
                           '--storage-backend=zfs',
                           '--storage-pool=poolname'])
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default")
     @mock.patch("cloudinit.config.cc_lxd.util")
-    def test_lxd_install(self, mock_util):
+    def test_lxd_install(self, mock_util, m_maybe_clean):
         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_clean.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_cleanup_default")
     @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_clean):
         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_clean.called)
 
+    @mock.patch("cloudinit.config.cc_lxd.maybe_cleanup_default")
     @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_clean):
         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_clean.called)
 
     def test_lxd_debconf_new_full(self):
         data = {"mode": "new",
@@ -147,14 +158,13 @@ class TestLxd(t_help.CiTestCase):
                 "domain": "lxd"}
         self.assertEqual(
             cc_lxd.bridge_to_cmd(data),
-            (["lxc", "network", "create", "testbr0",
+            (["network", "create", "testbr0",
               "ipv4.address=10.0.8.1/24", "ipv4.nat=true",
               "ipv4.dhcp.ranges=10.0.8.2-10.0.8.254",
               "ipv6.address=fd98:9e0:3744::1/64",
-              "ipv6.nat=true", "dns.domain=lxd",
-              "--force-local"],
-             ["lxc", "network", "attach-profile",
-              "testbr0", "default", "eth0", "--force-local"]))
+              "ipv6.nat=true", "dns.domain=lxd"],
+             ["network", "attach-profile",
+              "testbr0", "default", "eth0"]))
 
     def test_lxd_cmd_new_partial(self):
         data = {"mode": "new",
@@ -163,19 +173,18 @@ class TestLxd(t_help.CiTestCase):
                 "ipv6_nat": "true"}
         self.assertEqual(
             cc_lxd.bridge_to_cmd(data),
-            (["lxc", "network", "create", "lxdbr0", "ipv4.address=none",
-              "ipv6.address=fd98:9e0:3744::1/64", "ipv6.nat=true",
-              "--force-local"],
-             ["lxc", "network", "attach-profile",
-              "lxdbr0", "default", "eth0", "--force-local"]))
+            (["network", "create", "lxdbr0", "ipv4.address=none",
+              "ipv6.address=fd98:9e0:3744::1/64", "ipv6.nat=true"],
+             ["network", "attach-profile",
+              "lxdbr0", "default", "eth0"]))
 
     def test_lxd_cmd_existing(self):
         data = {"mode": "existing",
                 "name": "testbr0"}
         self.assertEqual(
             cc_lxd.bridge_to_cmd(data),
-            (None, ["lxc", "network", "attach-profile",
-                    "testbr0", "default", "eth0", "--force-local"]))
+            (None, ["network", "attach-profile",
+                    "testbr0", "default", "eth0"]))
 
     def test_lxd_cmd_none(self):
         data = {"mode": "none"}
@@ -183,4 +192,43 @@ class TestLxd(t_help.CiTestCase):
             cc_lxd.bridge_to_cmd(data),
             (None, None))
 
+
+class TestLxdMaybeCleanupDefault(t_help.CiTestCase):
+    """Test the implementation of maybe_cleanup_default."""
+
+    defnet = cc_lxd._DEFAULT_NETWORK_NAME
+
+    @mock.patch("cloudinit.config.cc_lxd._lxc")
+    def test_network_other_than_default_not_deleted(self, m_lxc):
+        """deletion or removal should only occur if bridge is default."""
+        cc_lxd.maybe_cleanup_default(
+            net_name="lxdbr1", did_init=True, create=True, attach=True)
+        m_lxc.assert_not_called()
+
+    @mock.patch("cloudinit.config.cc_lxd._lxc")
+    def test_did_init_false_does_not_delete(self, m_lxc):
+        """deletion or removal should only occur if did_init is True."""
+        cc_lxd.maybe_cleanup_default(
+            net_name=self.defnet, did_init=False, create=True, attach=True)
+        m_lxc.assert_not_called()
+
+    @mock.patch("cloudinit.config.cc_lxd._lxc")
+    def test_network_deleted_if_create_true(self, m_lxc):
+        """deletion of network should occur if create is True."""
+        cc_lxd.maybe_cleanup_default(
+            net_name=self.defnet, did_init=True, create=True, attach=False)
+        m_lxc.assert_called_once_with(["network", "delete", self.defnet])
+
+    @mock.patch("cloudinit.config.cc_lxd._lxc")
+    def test_device_removed_if_attach_true(self, m_lxc):
+        """deletion of network should occur if create is True."""
+        nic_name = "my_nic"
+        profile = "my_profile"
+        cc_lxd.maybe_cleanup_default(
+            net_name=self.defnet, did_init=True, create=False, attach=True,
+            profile=profile, nic_name=nic_name)
+        m_lxc.assert_called_once_with(
+            ["profile", "device", "remove", profile, nic_name])
+
+
 # vi: ts=4 expandtab