cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Chad Smith has proposed merging ~chad.smith/cloud-init:azure-no-ifupdown into cloud-init:master.

Requested reviews:
  cloud-init commiters (cloud-init-dev)

For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/335470

azure: Don't bounce network with ifdown ifup when those tools don't exist

This fixes a traceback when attempting to bounce the network after
hostname resets.

In artful and bionic ifupdown package is no longer installed in default
cloudimages. As such, Azure can't use those tools to bounce the network
informing DDNS about hostname changes. This doesn't affect DDNS updates
though because systemd-networkd is now watching hostname deltas and with
default behavior to SendHostname=True over dhcp for all hostname updates
which publishes DDNS for us.

This branch also fixes two use-cases unhandled by the prerequisite branch:
  - In lxc's ip -o link list returns interface alias information such as
    eth0@if37 which needs to be truncated as we try to complete network
device information
  - In Azure, host target routes and gateways need to be handled

LP: #1722668
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:azure-no-ifupdown into cloud-init:master.

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index e73b57b..a56ced2 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -591,6 +591,18 @@ def address_ephemeral_resize(devpath=RESOURCE_DISK_PATH, maxwait=120,
     return
 
 
+def _is_bounce_command_missing_dependency(command):
+    '''Return True if bounce command is unsupported.'''
+    if isinstance(command, (list, tuple)):
+        for item in command:
+            if 'ifdown' in item and not util.which('ifdown'):
+                return True
+    else:
+        if 'ifdown' in command and not util.which('ifdown'):
+            return True
+    return False
+
+
 def perform_hostname_bounce(hostname, cfg, prev_hostname):
     # set the hostname to 'hostname' if it is not already set to that.
     # then, if policy is not off, bounce the interface using command
@@ -608,6 +620,10 @@ def perform_hostname_bounce(hostname, cfg, prev_hostname):
     if command == "builtin":
         command = BOUNCE_COMMAND
 
+    if _is_bounce_command_missing_dependency(command):
+        LOG.debug(
+            "Skipping network bounce: ifupdown utils aren't present.")
+        return
     LOG.debug("pubhname: publishing hostname [%s]", msg)
     shell = not isinstance(command, (list, tuple))
     # capture=False, see comments in bug 1202758 and bug 1206164.
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 5ab4889..3cfc4d2 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -174,6 +174,7 @@ scbus-1 on xpt0 bus 0
             (dsaz, 'get_hostname', mock.MagicMock()),
             (dsaz, 'set_hostname', mock.MagicMock()),
             (dsaz, 'get_metadata_from_fabric', self.get_metadata_from_fabric),
+            (dsaz.util, 'which', lambda x: True),
             (dsaz.util, 'read_dmi_data', mock.MagicMock(
                 side_effect=_dmi_mocks)),
             (dsaz.util, 'wait_for_files', mock.MagicMock(
@@ -642,6 +643,8 @@ fdescfs            /dev/fd          fdescfs rw              0 0
 
 class TestAzureBounce(CiTestCase):
 
+    with_logs = True
+
     def mock_out_azure_moving_parts(self):
         self.patches.enter_context(
             mock.patch.object(dsaz, 'invoke_agent'))
@@ -753,6 +756,22 @@ class TestAzureBounce(CiTestCase):
         self.assertTrue(ret)
         self.assertEqual(1, perform_hostname_bounce.call_count)
 
+    def test_bounce_skipped_on_ifupdown_absent(self):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'force'}}
+        dsrc = self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg),
+                            agent_command=['not', '__builtin__'])
+        patch_path = 'cloudinit.sources.DataSourceAzure.util.which'
+        with mock.patch(patch_path) as m_which:
+            m_which.return_value = None
+            ret = self._get_and_setup(dsrc)
+        self.assertEqual([mock.call('ifdown')],  m_which.call_args_list)
+        self.assertTrue(ret)
+        self.assertIn(
+            "Skipping network bounce: ifupdown utils aren't present.",
+            self.logs.getvalue())
+
     def test_different_hostnames_sets_hostname(self):
         expected_hostname = 'azure-expected-host-name'
         self.get_hostname.return_value = 'default-host-name'
@@ -818,8 +837,6 @@ class TestAzureBounce(CiTestCase):
         self.assertEqual(old_hostname, bounce_env['old_hostname'])
 
     def test_default_bounce_command_used_by_default(self):
-        cmd = 'default-bounce-command'
-        dsaz.BUILTIN_DS_CONFIG['hostname_bounce']['command'] = cmd
         cfg = {'hostname_bounce': {'policy': 'force'}}
         data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
         dsrc = self._get_ds(data, agent_command=['not', '__builtin__'])
@@ -827,7 +844,8 @@ class TestAzureBounce(CiTestCase):
         self.assertTrue(ret)
         self.assertEqual(1, self.subp.call_count)
         bounce_args = self.subp.call_args[1]['args']
-        self.assertEqual(cmd, bounce_args)
+        self.assertEqual(
+            dsaz.BUILTIN_DS_CONFIG['hostname_bounce']['command'], bounce_args)
 
     @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
     def test_set_hostname_option_can_disable_bounce(