Merge ~chad.smith/cloud-init:azure-no-ifupdown into cloud-init:master

[Chad Smith]

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

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.

LP: #1722668

[Chad Smith (chad.smith)]

One inline comment about a potential alternative that may be more palatable/specific.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:74f546422a0382e5b5782762b0db8be8339d48e3
https://jenkins.ubuntu.com/server/job/cloud-init-ci/648/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/648/rebuild

review: Needs Fixing (continuous-integration)

c092c12... by Chad Smith on 2017-12-20

flakes

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:c092c12f606dd5a92e0f80db35a1cb4b8b5c1dcb
https://jenkins.ubuntu.com/server/job/cloud-init-ci/649/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    FAILED: Ubuntu LTS: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/649/rebuild

review: Needs Fixing (continuous-integration)

fdf3923... by Chad Smith on 2017-12-20

per smoser's review, make the ifup/down check simpler and just don't bounce at all if we are missing ifup util. This will continue to do the right thing if ifup is present (xenial).

74fb5bb... by Chad Smith on 2017-12-20

drop unused is_bounce_command_missing_dependency

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:74fb5bb7fdd46d7067e371833499131dfde14b30
https://jenkins.ubuntu.com/server/job/cloud-init-ci/650/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    FAILED: Ubuntu LTS: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/650/rebuild

review: Needs Fixing (continuous-integration)

6d567fe... by Chad Smith on 2017-12-20

properly mock util.which for non-ifupdown test platforms

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6d567fe2e563f27412b4c6527eb12f57c81aeeea
https://jenkins.ubuntu.com/server/job/cloud-init-ci/651/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/651/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

looks good, thanks.

review: Approve

[Scott Moser (smoser)]

Thank you for your merge proposal.

Your branch has been set to 'Work in progress'.
Please set the branch back to 'Needs Review' after resolving the issues below.

Thanks again,
Your friendly neighborhood cloud-init robot.

Please fix the following issues:
------------------------------
Commit message lints:
 - Expected empty line on line 2 of the commit message
------------------------------

For more information, see commit message guidelines at
https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-for-each-feature-or-bug

review: Needs Fixing

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index e73b57b..d1d0975 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 5ab4889..6341e1e 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(