Azure: Only bounce network when necessary.

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
cloud images. 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.

LP: #1722668

2 files changed, 40 insertions, 13 deletions

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index e73b57b9..d1d09757 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -26,10 +26,16 @@ DS_NAME = 'Azure'
 DEFAULT_METADATA = {"instance-id": "iid-AZURE-NODE"}
 AGENT_START = ['service', 'walinuxagent', 'start']
 AGENT_START_BUILTIN = "__builtin__"
-BOUNCE_COMMAND = [
+BOUNCE_COMMAND_IFUP = [
     'sh', '-xc',
     "i=$interface; x=0; ifdown $i || x=$?; ifup $i || x=$?; exit $x"
 ]
+BOUNCE_COMMAND_FREEBSD = [
+    'sh', '-xc',
+    ("i=$interface; x=0; ifconfig down $i || x=$?; "
+     "ifconfig up $i || x=$?; exit $x")
+]
+
 # azure systems will always have a resource disk, and 66-azure-ephemeral.rules
 # ensures that it gets linked to this path.
 RESOURCE_DISK_PATH = '/dev/disk/cloud/azure_resource'
@@ -177,11 +183,6 @@ if util.is_FreeBSD():
         RESOURCE_DISK_PATH = "/dev/" + res_disk
     else:
         LOG.debug("resource disk is None")
-    BOUNCE_COMMAND = [
-        'sh', '-xc',
-        ("i=$interface; x=0; ifconfig down $i || x=$?; "
-         "ifconfig up $i || x=$?; exit $x")
-    ]
 
 BUILTIN_DS_CONFIG = {
     'agent_command': AGENT_START_BUILTIN,
@@ -190,7 +191,7 @@ BUILTIN_DS_CONFIG = {
     'hostname_bounce': {
         'interface': DEFAULT_PRIMARY_NIC,
         'policy': True,
-        'command': BOUNCE_COMMAND,
+        'command': 'builtin',
         'hostname_command': 'hostname',
     },
     'disk_aliases': {'ephemeral0': RESOURCE_DISK_PATH},
@@ -606,8 +607,14 @@ def perform_hostname_bounce(hostname, cfg, prev_hostname):
     env['old_hostname'] = prev_hostname
 
     if command == "builtin":
-        command = BOUNCE_COMMAND
-
+        if util.is_FreeBSD():
+            command = BOUNCE_COMMAND_FREEBSD
+        elif util.which('ifup'):
+            command = BOUNCE_COMMAND_IFUP
+        else:
+            LOG.debug(
+                "Skipping network bounce: ifupdown utils aren't present.")
+            return  # Don't bounce as networkd handles hostname DDNS updates
     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 5ab48897..6341e1e8 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'))
@@ -653,6 +656,8 @@ class TestAzureBounce(CiTestCase):
         self.patches.enter_context(
             mock.patch.object(dsaz, 'get_metadata_from_fabric',
                               mock.MagicMock(return_value={})))
+        self.patches.enter_context(
+            mock.patch.object(dsaz.util, 'which', lambda x: True))
 
         def _dmi_mocks(key):
             if key == 'system-uuid':
@@ -753,6 +758,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('ifup')], 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'
@@ -817,9 +838,7 @@ class TestAzureBounce(CiTestCase):
         self.assertEqual(hostname, bounce_env['hostname'])
         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
+    def test_default_bounce_command_ifup_used_by_default(self):
         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 +846,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.BOUNCE_COMMAND_IFUP, bounce_args)
 
     @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
     def test_set_hostname_option_can_disable_bounce(