Merge lp:~oddbloke/cloud-init/lp1375252 into lp:~cloud-init-dev/cloud-init/trunk

[Dan Watkins]

[Dan Watkins (oddbloke)]

Review appreciated, but probably shouldn't be merged until vivid is out the door.

=== modified file 'cloudinit/sources/DataSourceAzure.py'
--- cloudinit/sources/DataSourceAzure.py	2015-02-18 13:30:51 +0000
+++ cloudinit/sources/DataSourceAzure.py	2015-04-15 11:26:23 +0000
@@ -17,6 +17,7 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import base64
+import contextlib
 import crypt
 import fnmatch
 import os
@@ -66,6 +67,36 @@
 DEF_EPHEMERAL_LABEL = 'Temporary Storage'
 
 
+def get_hostname(hostname_command='hostname'):
+    return util.subp(hostname_command, capture=True)[0].strip()
+
+
+def set_hostname(hostname, hostname_command='hostname'):
+    util.subp([hostname_command, hostname])
+
+
[e-mail]
+def temporary_hostname(temp_hostname, cfg, hostname_command='hostname'):
+    """
+    Set a temporary hostname, restoring the previous hostname on exit.
+
+    Will have the value of the previous hostname when used as a context
+    manager, or None if the hostname was not changed.
+    """
+    policy = cfg['hostname_bounce']['policy']
+    previous_hostname = get_hostname(hostname_command)
+    if (not util.is_true(cfg.get('set_hostname'))
+            or util.is_false(policy)
+            or (previous_hostname == temp_hostname and policy != 'force')):
+        yield None
+        return
+    set_hostname(temp_hostname, hostname_command)
+    try:
+        yield previous_hostname
+    finally:
+        set_hostname(previous_hostname, hostname_command)
+
+
 class DataSourceAzureNet(sources.DataSource):
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
@@ -154,33 +185,40 @@
         # the directory to be protected.
         write_files(ddir, files, dirmode=0o700)
 
-        # handle the hostname 'publishing'
-        try:
-            handle_set_hostname(mycfg.get('set_hostname'),
-                                self.metadata.get('local-hostname'),
-                                mycfg['hostname_bounce'])
-        except Exception as e:
-            LOG.warn("Failed publishing hostname: %s", e)
-            util.logexc(LOG, "handling set_hostname failed")
-
-        try:
-            invoke_agent(mycfg['agent_command'])
-        except util.ProcessExecutionError:
-            # claim the datasource even if the command failed
-            util.logexc(LOG, "agent command '%s' failed.",
-                        mycfg['agent_command'])
-
-        shcfgxml = os.path.join(ddir, "SharedConfig.xml")
-        wait_for = [shcfgxml]
-
-        fp_files = []
-        for pk in self.cfg.get('_pubkeys', []):
-            bname = str(pk['fingerprint'] + ".crt")
-            fp_files += [os.path.join(ddir, bname)]
-
-        missing = util.log_time(logfunc=LOG.debug, msg="waiting for files",
-                                func=wait_for_files,
-                                args=(wait_for + fp_files,))
+        temp_hostname = self.metadata.get('local-hostname')
+        hostname_command = mycfg['hostname_bounce']['hostname_command']
+        with temporary_hostname(temp_hostname, mycfg,
+                                hostname_command=hostname_command) \
+                as previous_hostname:
+            if (previous_hostname is not None
+                    and util.is_true(mycfg.get('set_hostname'))):
+                cfg = mycfg['hostname_bounce']
+                try:
+                    perform_hostname_bounce(hostname=temp_hostname,
+                                            cfg=cfg,
+                                            prev_hostname=previous_hostname)
+                except Exception as e:
+                    LOG.warn("Failed publishing hostname: %s", e)
+                    util.logexc(LOG, "handling set_hostname failed")
+
+            try:
+                invoke_agent(mycfg['agent_command'])
+            except util.ProcessExecutionError:
+                # claim the datasource even if the command failed
+                util.logexc(LOG, "agent command '%s' failed.",
+                            mycfg['agent_command'])
+
+            shcfgxml = os.path.join(ddir, "SharedConfig.xml")
+            wait_for = [shcfgxml]
+
+            fp_files = []
+            for pk in self.cfg.get('_pubkeys', []):
+                bname = str(pk['fingerprint'] + ".crt")
+                fp_files += [os.path.join(ddir, bname)]
+
+            missing = util.log_time(logfunc=LOG.debug, msg="waiting for files",
+                                    func=wait_for_files,
+                                    args=(wait_for + fp_files,))
         if len(missing):
             LOG.warn("Did not find files, but going on: %s", missing)
 
@@ -299,39 +337,15 @@
     return mod_list
 
 
-def handle_set_hostname(enabled, hostname, cfg):
-    if not util.is_true(enabled):
-        return
-
-    if not hostname:
-        LOG.warn("set_hostname was true but no local-hostname")
-        return
-
-    apply_hostname_bounce(hostname=hostname, policy=cfg['policy'],
-                          interface=cfg['interface'],
-                          command=cfg['command'],
-                          hostname_command=cfg['hostname_command'])
-
-
-def apply_hostname_bounce(hostname, policy, interface, command,
-                          hostname_command="hostname"):
+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
-    prev_hostname = util.subp(hostname_command, capture=True)[0].strip()
-
-    util.subp([hostname_command, hostname])
-
-    msg = ("phostname=%s hostname=%s policy=%s interface=%s" %
-           (prev_hostname, hostname, policy, interface))
-
-    if util.is_false(policy):
-        LOG.debug("pubhname: policy false, skipping [%s]", msg)
-        return
-
-    if prev_hostname == hostname and policy != "force":
-        LOG.debug("pubhname: no change, policy != force. skipping. [%s]", msg)
-        return
-
+    command = cfg['command']
+    interface = cfg['interface']
+    policy = cfg['policy']
+
+    msg = ("hostname=%s policy=%s interface=%s" %
+           (hostname, policy, interface))
     env = os.environ.copy()
     env['interface'] = interface
     env['hostname'] = hostname
@@ -344,9 +358,9 @@
     shell = not isinstance(command, (list, tuple))
     # capture=False, see comments in bug 1202758 and bug 1206164.
     util.log_time(logfunc=LOG.debug, msg="publishing hostname",
-        get_uptime=True, func=util.subp,
-        kwargs={'args': command, 'shell': shell, 'capture': False,
-                'env': env})
+                  get_uptime=True, func=util.subp,
+                  kwargs={'args': command, 'shell': shell, 'capture': False,
+                          'env': env})
 
 
 def crtfile_to_pubkey(fname):

=== modified file 'tests/unittests/test_datasource/test_azure.py'
--- tests/unittests/test_datasource/test_azure.py	2015-02-24 16:58:22 +0000
+++ tests/unittests/test_datasource/test_azure.py	2015-04-15 11:26:23 +0000
@@ -116,9 +116,6 @@
             data['iid_from_shared_cfg'] = path
             return 'i-my-azure-id'
 
-        def _apply_hostname_bounce(**kwargs):
-            data['apply_hostname_bounce'] = kwargs
-
         if data.get('ovfcontent') is not None:
             populate_dir(os.path.join(self.paths.seed_dir, "azure"),
                          {'ovf-env.xml': data['ovfcontent']})
@@ -132,7 +129,9 @@
             (mod, 'wait_for_files', _wait_for_files),
             (mod, 'pubkeys_from_crt_files', _pubkeys_from_crt_files),
             (mod, 'iid_from_shared_config', _iid_from_shared_config),
-            (mod, 'apply_hostname_bounce', _apply_hostname_bounce),
+            (mod, 'perform_hostname_bounce', mock.MagicMock()),
+            (mod, 'get_hostname', mock.MagicMock()),
+            (mod, 'set_hostname', mock.MagicMock()),
             ])
 
         dsrc = mod.DataSourceAzureNet(
@@ -272,47 +271,6 @@
         for mypk in mypklist:
             self.assertIn(mypk, dsrc.cfg['_pubkeys'])
 
-    def test_disabled_bounce(self):
-        pass
-
-    def test_apply_bounce_call_1(self):
-        # hostname needs to get through to apply_hostname_bounce
-        odata = {'HostName': 'my-random-hostname'}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-
-        self._get_ds(data).get_data()
-        self.assertIn('hostname', data['apply_hostname_bounce'])
-        self.assertEqual(data['apply_hostname_bounce']['hostname'],
-                         odata['HostName'])
-
-    def test_apply_bounce_call_configurable(self):
-        # hostname_bounce should be configurable in datasource cfg
-        cfg = {'hostname_bounce': {'interface': 'eth1', 'policy': 'off',
-                                   'command': 'my-bounce-command',
-                                   'hostname_command': 'my-hostname-command'}}
-        odata = {'HostName': "xhost",
-                'dscfg': {'text': b64e(yaml.dump(cfg)),
-                          'encoding': 'base64'}}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-        self._get_ds(data).get_data()
-
-        for k in cfg['hostname_bounce']:
-            self.assertIn(k, data['apply_hostname_bounce'])
-
-        for k, v in cfg['hostname_bounce'].items():
-            self.assertEqual(data['apply_hostname_bounce'][k], v)
-
-    def test_set_hostname_disabled(self):
-        # config specifying set_hostname off should not bounce
-        cfg = {'set_hostname': False}
-        odata = {'HostName': "xhost",
-                'dscfg': {'text': b64e(yaml.dump(cfg)),
-                          'encoding': 'base64'}}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-        self._get_ds(data).get_data()
-
-        self.assertEqual(data.get('apply_hostname_bounce', "N/A"), "N/A")
-
     def test_default_ephemeral(self):
         # make sure the ephemeral device works
         odata = {}
@@ -425,6 +383,175 @@
             load_file(os.path.join(self.waagent_d, 'ovf-env.xml')))
 
 
+class TestAzureBounce(TestCase):
+
+    def mock_out_azure_moving_parts(self):
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'invoke_agent'))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'wait_for_files'))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'iid_from_shared_config',
+                              mock.MagicMock(return_value='i-my-azure-id')))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'list_possible_azure_ds_devs',
+                              mock.MagicMock(return_value=[])))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'find_ephemeral_disk',
+                              mock.MagicMock(return_value=None)))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'find_ephemeral_part',
+                              mock.MagicMock(return_value=None)))
+
+    def setUp(self):
+        super(TestAzureBounce, self).setUp()
+        self.tmp = tempfile.mkdtemp()
+        self.waagent_d = os.path.join(self.tmp, 'var', 'lib', 'waagent')
+        self.paths = helpers.Paths({'cloud_dir': self.tmp})
+        self.addCleanup(shutil.rmtree, self.tmp)
+        DataSourceAzure.BUILTIN_DS_CONFIG['data_dir'] = self.waagent_d
+        self.patches = ExitStack()
+        self.mock_out_azure_moving_parts()
+        self.get_hostname = self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'get_hostname'))
+        self.set_hostname = self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'set_hostname'))
+        self.subp = self.patches.enter_context(
+            mock.patch('cloudinit.sources.DataSourceAzure.util.subp'))
+
+    def tearDown(self):
+        self.patches.close()
+
+    def _get_ds(self, ovfcontent=None):
+        if ovfcontent is not None:
+            populate_dir(os.path.join(self.paths.seed_dir, "azure"),
+                         {'ovf-env.xml': ovfcontent})
+        return DataSourceAzure.DataSourceAzureNet(
+            {}, distro=None, paths=self.paths)
+
+    def get_ovf_env_with_dscfg(self, hostname, cfg):
+        odata = {
+            'HostName': hostname,
+            'dscfg': {
+                'text': b64e(yaml.dump(cfg)),
+                'encoding': 'base64'
+            }
+        }
+        return construct_valid_ovf_env(data=odata)
+
+    def test_disabled_bounce_does_not_change_hostname(self):
+        cfg = {'hostname_bounce': {'policy': 'off'}}
+        self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg)).get_data()
+        self.assertEqual(0, self.set_hostname.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_disabled_bounce_does_not_perform_bounce(
+            self, perform_hostname_bounce):
+        cfg = {'hostname_bounce': {'policy': 'off'}}
+        self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg)).get_data()
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    def test_same_hostname_does_not_change_hostname(self):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'yes'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(0, self.set_hostname.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_unchanged_hostname_does_not_perform_bounce(
+            self, perform_hostname_bounce):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'yes'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_force_performs_bounce_regardless(self, perform_hostname_bounce):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'force'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(1, perform_hostname_bounce.call_count)
+
+    def test_different_hostnames_sets_hostname(self):
+        expected_hostname = 'azure-expected-host-name'
+        self.get_hostname.return_value = 'default-host-name'
+        self._get_ds(
+            self.get_ovf_env_with_dscfg(expected_hostname, {})).get_data()
+        self.assertEqual(expected_hostname,
+                         self.set_hostname.call_args_list[0][0][0])
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_different_hostnames_performs_bounce(
+            self, perform_hostname_bounce):
+        expected_hostname = 'azure-expected-host-name'
+        self.get_hostname.return_value = 'default-host-name'
+        self._get_ds(
+            self.get_ovf_env_with_dscfg(expected_hostname, {})).get_data()
+        self.assertEqual(1, perform_hostname_bounce.call_count)
+
+    def test_different_hostnames_sets_hostname_back(self):
+        initial_host_name = 'default-host-name'
+        self.get_hostname.return_value = initial_host_name
+        self._get_ds(
+            self.get_ovf_env_with_dscfg('some-host-name', {})).get_data()
+        self.assertEqual(initial_host_name,
+                         self.set_hostname.call_args_list[-1][0][0])
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_failure_in_bounce_still_resets_host_name(
+            self, perform_hostname_bounce):
+        perform_hostname_bounce.side_effect = Exception
+        initial_host_name = 'default-host-name'
+        self.get_hostname.return_value = initial_host_name
+        self._get_ds(
+            self.get_ovf_env_with_dscfg('some-host-name', {})).get_data()
+        self.assertEqual(initial_host_name,
+                         self.set_hostname.call_args_list[-1][0][0])
+
+    def test_environment_correct_for_bounce_command(self):
+        interface = 'int0'
+        hostname = 'my-new-host'
+        old_hostname = 'my-old-host'
+        self.get_hostname.return_value = old_hostname
+        cfg = {'hostname_bounce': {'interface': interface, 'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg(hostname, cfg)
+        self._get_ds(data).get_data()
+        self.assertEqual(1, self.subp.call_count)
+        bounce_env = self.subp.call_args[1]['env']
+        self.assertEqual(interface, bounce_env['interface'])
+        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'
+        DataSourceAzure.BUILTIN_DS_CONFIG['hostname_bounce']['command'] = cmd
+        cfg = {'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+        self.assertEqual(1, self.subp.call_count)
+        bounce_args = self.subp.call_args[1]['args']
+        self.assertEqual(cmd, bounce_args)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_set_hostname_option_can_disable_bounce(
+            self, perform_hostname_bounce):
+        cfg = {'set_hostname': False, 'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    def test_set_hostname_option_can_disable_hostname_set(self):
+        cfg = {'set_hostname': False, 'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+
+        self.assertEqual(0, self.set_hostname.call_count)
+
+
 class TestReadAzureOvf(TestCase):
     def test_invalid_xml_raises_non_azure_ds(self):
         invalid_xml = "<foo>" + construct_valid_ovf_env(data={})