net: detect unstable network names and trigger a settle if needed

The cloud-init-local.service expects that any network device name changes
have already been completed by the kernel or udev daemon.

In some situations we've found that the renaming of interfaces from kernel
names (eth0, eth1, etc) to their persistent names (eno1, ens3, enp0s1,
etc) may happen after cloud-init-local has started where it reads values
from sysfs about what network devices are present, and which device to use
as a fallback nic.

Subsequently, cloud-init-local would write out network configuration for a
kernel device name which would no longer be present by the time that
networking services start to bring up the devices. The result is that the
instance does not get networking configured. Prior to use of
systemd-networkd, the Ubuntu 'networking.service' unit included a call to
udevadm settle which is why this race is not seen on a Xenial system.

This change adds the ability to detect if an interface has a stable name,
if if we find one without stable names and stable names have not been
disabled (net.ifnames=0 in /proc/cmdline), then cloud-init will invoke
udevadm settle.

LP: #1766287

7 files changed, 165 insertions, 16 deletions

diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py
index c3e8c484..943089e0 100644
--- a/cloudinit/config/cc_disk_setup.py
+++ b/cloudinit/config/cc_disk_setup.py
@@ -680,13 +680,13 @@ def read_parttbl(device):
     reliable way to probe the partition table.
     """
     blkdev_cmd = [BLKDEV_CMD, '--rereadpt', device]
-    udevadm_settle()
+    util.udevadm_settle()
     try:
         util.subp(blkdev_cmd)
     except Exception as e:
         util.logexc(LOG, "Failed reading the partition table %s" % e)
 
-    udevadm_settle()
+    util.udevadm_settle()
 
 
 def exec_mkpart_mbr(device, layout):
@@ -737,14 +737,10 @@ def exec_mkpart(table_type, device, layout):
     return get_dyn_func("exec_mkpart_%s", table_type, device, layout)
 
 
-def udevadm_settle():
-    util.subp(['udevadm', 'settle'])
-
-
 def assert_and_settle_device(device):
     """Assert that device exists and settle so it is fully recognized."""
     if not os.path.exists(device):
-        udevadm_settle()
+        util.udevadm_settle()
         if not os.path.exists(device):
             raise RuntimeError("Device %s did not exist and was not created "
                                "with a udevamd settle." % device)
@@ -752,7 +748,7 @@ def assert_and_settle_device(device):
     # Whether or not the device existed above, it is possible that udev
     # events that would populate udev database (for reading by lsdname) have
     # not yet finished. So settle again.
-    udevadm_settle()
+    util.udevadm_settle()
 
 
 def mkpart(device, definition):
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 80054546..43226bd0 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -107,6 +107,21 @@ def is_bond(devname):
     return os.path.exists(sys_dev_path(devname, "bonding"))
 
 
+def is_renamed(devname):
+    """
+    /* interface name assignment types (sysfs name_assign_type attribute) */
+    #define NET_NAME_UNKNOWN	0	/* unknown origin (not exposed to user) */
+    #define NET_NAME_ENUM		1	/* enumerated by kernel */
+    #define NET_NAME_PREDICTABLE	2	/* predictably named by the kernel */
+    #define NET_NAME_USER		3	/* provided by user-space */
+    #define NET_NAME_RENAMED	4	/* renamed by user-space */
+    """
+    name_assign_type = read_sys_net_safe(devname, 'name_assign_type')
+    if name_assign_type and name_assign_type in ['3', '4']:
+        return True
+    return False
+
+
 def is_vlan(devname):
     uevent = str(read_sys_net_safe(devname, "uevent"))
     return 'DEVTYPE=vlan' in uevent.splitlines()
@@ -180,6 +195,17 @@ def find_fallback_nic(blacklist_drivers=None):
     if not blacklist_drivers:
         blacklist_drivers = []
 
+    if 'net.ifnames=0' in util.get_cmdline():
+        LOG.debug('Stable ifnames disabled by net.ifnames=0 in /proc/cmdline')
+    else:
+        unstable = [device for device in get_devicelist()
+                    if device != 'lo' and not is_renamed(device)]
+        if len(unstable):
+            LOG.debug('Found unstable nic names: %s; calling udevadm settle',
+                      unstable)
+            msg = 'Waiting for udev events to settle'
+            util.log_time(LOG.debug, msg, func=util.udevadm_settle)
+
     # get list of interfaces that could have connections
     invalid_interfaces = set(['lo'])
     potential_interfaces = set([device for device in get_devicelist()
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index 276556ee..5c017d15 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -199,6 +199,7 @@ class TestGenerateFallbackConfig(CiTestCase):
         self.sysdir = self.tmp_dir() + '/'
         self.m_sys_path.return_value = self.sysdir
         self.addCleanup(sys_mock.stop)
+        self.add_patch('cloudinit.net.util.udevadm_settle', 'm_settle')
 
     def test_generate_fallback_finds_connected_eth_with_mac(self):
         """generate_fallback_config finds any connected device with a mac."""
diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py
index e1d0055b..f6e86f34 100644
--- a/cloudinit/sources/DataSourceAltCloud.py
+++ b/cloudinit/sources/DataSourceAltCloud.py
@@ -29,7 +29,6 @@ CLOUD_INFO_FILE = '/etc/sysconfig/cloud-info'
 
 # Shell command lists
 CMD_PROBE_FLOPPY = ['modprobe', 'floppy']
-CMD_UDEVADM_SETTLE = ['udevadm', 'settle', '--timeout=5']
 
 META_DATA_NOT_SUPPORTED = {
     'block-device-mapping': {},
@@ -196,9 +195,7 @@ class DataSourceAltCloud(sources.DataSource):
 
         # udevadm settle for floppy device
         try:
-            cmd = CMD_UDEVADM_SETTLE
-            cmd.append('--exit-if-exists=' + floppy_dev)
-            (cmd_out, _err) = util.subp(cmd)
+            (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5)
             LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
         except ProcessExecutionError as _err:
             util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), _err)
diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
index 76eed076..3c05a437 100644
--- a/cloudinit/tests/test_util.py
+++ b/cloudinit/tests/test_util.py
@@ -212,4 +212,53 @@ class TestBlkid(CiTestCase):
                                   capture=True, decode="replace")
 
 
+@mock.patch('cloudinit.util.subp')
+class TestUdevadmSettle(CiTestCase):
+    def test_with_no_params(self, m_subp):
+        """called with no parameters."""
+        util.udevadm_settle()
+        m_subp.called_once_with(mock.call(['udevadm', 'settle']))
+
+    def test_with_exists_and_not_exists(self, m_subp):
+        """with exists=file where file does not exist should invoke subp."""
+        mydev = self.tmp_path("mydev")
+        util.udevadm_settle(exists=mydev)
+        m_subp.called_once_with(
+            ['udevadm', 'settle', '--exit-if-exists=%s' % mydev])
+
+    def test_with_exists_and_file_exists(self, m_subp):
+        """with exists=file where file does exist should not invoke subp."""
+        mydev = self.tmp_path("mydev")
+        util.write_file(mydev, "foo\n")
+        util.udevadm_settle(exists=mydev)
+        self.assertIsNone(m_subp.call_args)
+
+    def test_with_timeout_int(self, m_subp):
+        """timeout can be an integer."""
+        timeout = 9
+        util.udevadm_settle(timeout=timeout)
+        m_subp.called_once_with(
+            ['udevadm', 'settle', '--timeout=%s' % timeout])
+
+    def test_with_timeout_string(self, m_subp):
+        """timeout can be a string."""
+        timeout = "555"
+        util.udevadm_settle(timeout=timeout)
+        m_subp.assert_called_once_with(
+            ['udevadm', 'settle', '--timeout=%s' % timeout])
+
+    def test_with_exists_and_timeout(self, m_subp):
+        """test call with both exists and timeout."""
+        mydev = self.tmp_path("mydev")
+        timeout = "3"
+        util.udevadm_settle(exists=mydev)
+        m_subp.called_once_with(
+            ['udevadm', 'settle', '--exit-if-exists=%s' % mydev,
+             '--timeout=%s' % timeout])
+
+    def test_subp_exception_raises_to_caller(self, m_subp):
+        m_subp.side_effect = util.ProcessExecutionError("BOOM")
+        self.assertRaises(util.ProcessExecutionError, util.udevadm_settle)
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 310758dd..2828ca38 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2727,4 +2727,19 @@ def mount_is_read_write(mount_point):
     mount_opts = result[-1].split(',')
     return mount_opts[0] == 'rw'
 
+
+def udevadm_settle(exists=None, timeout=None):
+    """Invoke udevadm settle with optional exists and timeout parameters"""
+    settle_cmd = ["udevadm", "settle"]
+    if exists:
+        # skip the settle if the requested path already exists
+        if os.path.exists(exists):
+            return
+        settle_cmd.extend(['--exit-if-exists=%s' % exists])
+    if timeout:
+        settle_cmd.extend(['--timeout=%s' % timeout])
+
+    return subp(settle_cmd)
+
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 84839fd7..fac82678 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1442,6 +1442,7 @@ DEFAULT_DEV_ATTRS = {
         "address": "07-1C-C6-75-A4-BE",
         "device/driver": None,
         "device/device": None,
+        "name_assign_type": "4",
     }
 }
 
@@ -1489,11 +1490,14 @@ class TestGenerateFallbackConfig(CiTestCase):
             'eth0': {
                 'bridge': False, 'carrier': False, 'dormant': False,
                 'operstate': 'down', 'address': '00:11:22:33:44:55',
-                'device/driver': 'hv_netsvc', 'device/device': '0x3'},
+                'device/driver': 'hv_netsvc', 'device/device': '0x3',
+                'name_assign_type': '4'},
             'eth1': {
                 'bridge': False, 'carrier': False, 'dormant': False,
                 'operstate': 'down', 'address': '00:11:22:33:44:55',
-                'device/driver': 'mlx4_core', 'device/device': '0x7'},
+                'device/driver': 'mlx4_core', 'device/device': '0x7',
+                'name_assign_type': '4'},
+
         }
 
         tmp_dir = self.tmp_dir()
@@ -1549,11 +1553,13 @@ iface eth0 inet dhcp
             'eth1': {
                 'bridge': False, 'carrier': False, 'dormant': False,
                 'operstate': 'down', 'address': '00:11:22:33:44:55',
-                'device/driver': 'hv_netsvc', 'device/device': '0x3'},
+                'device/driver': 'hv_netsvc', 'device/device': '0x3',
+                'name_assign_type': '4'},
             'eth0': {
                 'bridge': False, 'carrier': False, 'dormant': False,
                 'operstate': 'down', 'address': '00:11:22:33:44:55',
-                'device/driver': 'mlx4_core', 'device/device': '0x7'},
+                'device/driver': 'mlx4_core', 'device/device': '0x7',
+                'name_assign_type': '4'},
         }
 
         tmp_dir = self.tmp_dir()
@@ -1602,6 +1608,65 @@ iface eth1 inet dhcp
         ]
         self.assertEqual(", ".join(expected_rule) + '\n', contents.lstrip())
 
+    @mock.patch("cloudinit.util.udevadm_settle")
+    @mock.patch("cloudinit.net.sys_dev_path")
+    @mock.patch("cloudinit.net.read_sys_net")
+    @mock.patch("cloudinit.net.get_devicelist")
+    def test_unstable_names(self, mock_get_devicelist, mock_read_sys_net,
+                            mock_sys_dev_path, mock_settle):
+        """verify that udevadm settle is called when we find unstable names"""
+        devices = {
+            'eth0': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'hv_netsvc', 'device/device': '0x3',
+                'name_assign_type': False},
+            'ens4': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'mlx4_core', 'device/device': '0x7',
+                'name_assign_type': '4'},
+
+        }
+
+        tmp_dir = self.tmp_dir()
+        _setup_test(tmp_dir, mock_get_devicelist,
+                    mock_read_sys_net, mock_sys_dev_path,
+                    dev_attrs=devices)
+        net.generate_fallback_config(config_driver=True)
+        self.assertEqual(1, mock_settle.call_count)
+
+    @mock.patch("cloudinit.util.get_cmdline")
+    @mock.patch("cloudinit.util.udevadm_settle")
+    @mock.patch("cloudinit.net.sys_dev_path")
+    @mock.patch("cloudinit.net.read_sys_net")
+    @mock.patch("cloudinit.net.get_devicelist")
+    def test_unstable_names_disabled(self, mock_get_devicelist,
+                                     mock_read_sys_net, mock_sys_dev_path,
+                                     mock_settle, m_get_cmdline):
+        """verify udevadm settle not called when cmdline has net.ifnames=0"""
+        devices = {
+            'eth0': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'hv_netsvc', 'device/device': '0x3',
+                'name_assign_type': False},
+            'ens4': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'mlx4_core', 'device/device': '0x7',
+                'name_assign_type': '4'},
+
+        }
+
+        m_get_cmdline.return_value = 'net.ifnames=0'
+        tmp_dir = self.tmp_dir()
+        _setup_test(tmp_dir, mock_get_devicelist,
+                    mock_read_sys_net, mock_sys_dev_path,
+                    dev_attrs=devices)
+        net.generate_fallback_config(config_driver=True)
+        self.assertEqual(0, mock_settle.call_count)
+
 
 class TestSysConfigRendering(CiTestCase):