Merge ~chad.smith/cloud-init:fix-ec2-fallback-nic into cloud-init:master

[Chad Smith]

EC2: Fix bug using fallback_nic and metadata when restoring from cache.

If user upgraded to new cloud-init and attempted to run 'cloud-init init'
without rebooting, cloud-init restores the datasource object from pickle.
The older version pickled datasource object had no value for
_network_config or fallback_nic. This caused the Ec2 datasource to attempt
to reconfigure networking with a None fallback_nic. The pickled object
also cached an older version of ec2 metadata which didn't contain network
information.

This branch does two things:
 - Add a fallback_interface property to DatasourceEC2 to support reading the
   old .fallback_nic attribute if it was set. New versions will
   call net.find_fallback_nic() if there has not been one found.
 - Re-crawl metadata if we are on Ec2 and don't have a 'network' key in
   metadata

LP: #1732917

EC2: Fix bug using fallback_nic and metadata when restoring from cache.

If user upgraded to new cloud-init and attempted to run 'cloud-init init'
without rebooting, cloud-init restores the datasource object from pickle.
The older version pickled datasource object had no value for
_network_config or fallback_nic. This caused the Ec2 datasource to attempt
to reconfigure networking with a None fallback_nic. The pickled object
also cached an older version of ec2 metadata which didn't contain network
information.

This branch does two things:
 - Add a fallback_interface property to DatasourceEC2 to support reading the
   old .fallback_nic attribute if it was set. New versions will
   call net.find_fallback_nic() if there has not been one found.
 - Re-crawl metadata if we are on Ec2 and don't have a 'network' key in
   metadata

LP: #1732917

[Ryan Harper (raharper)]

I'm some what leery of recrawling merely because someone ran 'cloud-init init' when there is a cached object for the current instance-id. Wouldn't it be better to *not* re-read metadata?

Now, thinking about a reboot; due to Ec2 datasource not providing a check_instance_id() method, we always throw out the previous boot's Datasource cached object. Prior to local datasource Ec2, we always re-read metadata when acquiring an EC2 datasource; every boot we always read metadata.

I think that reasonably convinces (at least myself) that any time we instantiate an Ec2 datasource object (whether from unpickle or reboot); it's going to read the metadata.

[Chad Smith (chad.smith)]

Just tested on Ec2

ubuntu@ip-172-31-16-165:~$ sudo dpkg -i cloud-init_0.7.9-233-ge586fe35-0ubuntu1~16.04.2_all.deb

sudo rm -rf /var/log/cloud-init* /var/lib/cloud;
sudo cloud-init init

ubuntu@ip-172-31-16-165:~$ sudo dpkg -i cloud-init_17.1-44-gc8864aca-1~bddeb_all.deb
(Reading database ... 51123 files and directories currently installed.)
Preparing to unpack cloud-init_17.1-44-gc8864aca-1~bddeb_all.deb ...
Unpacking cloud-init (17.1-44-gc8864aca-1~bddeb) over (0.7.9-233-ge586fe35-0ubuntu1~16.04.2) ...
Setting up cloud-init (17.1-44-gc8864aca-1~bddeb) ...
Installing new version of config file /etc/cloud/cloud.cfg ...
Installing new version of config file /etc/cloud/templates/hosts.suse.tmpl ...
Installing new version of config file /etc/cloud/templates/ntp.conf.sles.tmpl ...
Installing new version of config file /etc/cloud/templates/sources.list.debian.tmpl ...
ubuntu@ip-172-31-16-165:~$ sudo cloud-init init
Cloud-init v. 17.1 running 'init' at Sat, 18 Nov 2017 03:01:33 +0000. Up 17185.03 seconds.
ci-info: ++++++++++++++++++++++++++++++++++++++Net device info++++++++++++++++++++++++++++++++++++++
ci-info: +--------+------+-----------------------------+---------------+-------+-------------------+
ci-info: | Device | Up | Address | Mask | Scope | Hw-Address |
ci-info: +--------+------+-----------------------------+---------------+-------+-------------------+
ci-info: | eth0 | True | 172.31.16.165 | 255.255.240.0 | . | 06:23:bc:a9:a4:fa |
ci-info: | eth0 | True | fe80::423:bcff:fea9:a4fa/64 | . | link | 06:23:bc:a9:a4:fa |
ci-info: | lo | True | 127.0.0.1 | 255.0.0.0 | . | . |
ci-info: | lo | True | ::1/128 | . | host | . |
ci-info: +--------+------+-----------------------------+---------------+-------+-------------------+
ci-info: +++++++++++++++++++++++++++++Route IPv4 info+++++++++++++++++++++++++++++
ci-info: +-------+-------------+-------------+---------------+-----------+-------+
ci-info: | Route | Destination | Gateway | Genmask | Interface | Flags |
ci-info: +-------+-------------+-------------+---------------+-----------+-------+
ci-info: | 0 | 0.0.0.0 | 172.31.16.1 | 0.0.0.0 | eth0 | UG |
ci-info: | 1 | 172.31.16.0 | 0.0.0.0 | 255.255.240.0 | eth0 | U |
ci-info: +-------+-------------+-------------+---------------+-----------+-------+
ubuntu@ip-172-31-16-165:~$ systemctl status cloud-init.service
● cloud-init.service - Initial cloud-init job (metadata service crawler)
   Loaded: loaded (/lib/systemd/system/cloud-init.service; enabled; vendor preset: enabled)
   Active: active (exited) since Fri 2017-11-17 23:01:14 UTC; 4h 0min ago
 Main PID: 3781 (code=exited, status=0/SUCCESS)

Nov 17 23:01:14 ip-172-31-16-165 cloud-init[3781]: ci-info: | lo | True | ::1/128 | . | host | . |
Nov 17 23:01:14 ip-172-31-16-165 cloud-init[3781]: ci-info: +--------+------+-----------------------------+---------------+-------+-------------------+
Nov 17 23:01:14 ip-172-31-16-165 ...

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

FAILED: Continuous integration, rev:c8864aca05cdbc6a8a35c944f0f75d6d687e92fb
https://jenkins.ubuntu.com/server/job/cloud-init-ci/513/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    FAILED: MAAS Compatability Testing

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

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

one comment inline.

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

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/516/
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/516/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

review: Approve

diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py
index 0cba703..1a561be 100644
--- a/cloudinit/net/dhcp.py
+++ b/cloudinit/net/dhcp.py
@@ -41,8 +41,7 @@ def maybe_perform_dhcp_discovery(nic=None):
     if nic is None:
         nic = find_fallback_nic()
         if nic is None:
-            LOG.debug(
-                'Skip dhcp_discovery: Unable to find fallback nic.')
+            LOG.debug('Skip dhcp_discovery: Unable to find fallback nic.')
             return {}
     elif nic not in get_devicelist():
         LOG.debug(
diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 0ef2217..7bbbfb6 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -65,7 +65,7 @@ class DataSourceEc2(sources.DataSource):
     get_network_metadata = False
 
     # Track the discovered fallback nic for use in configuration generation.
-    fallback_nic = None
+    _fallback_interface = None
 
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
@@ -92,18 +92,17 @@ class DataSourceEc2(sources.DataSource):
         elif self.cloud_platform == Platforms.NO_EC2_METADATA:
             return False
 
-        self.fallback_nic = net.find_fallback_nic()
         if self.get_network_metadata:  # Setup networking in init-local stage.
             if util.is_FreeBSD():
                 LOG.debug("FreeBSD doesn't support running dhclient with -sf")
                 return False
-            dhcp_leases = dhcp.maybe_perform_dhcp_discovery(self.fallback_nic)
+            dhcp_leases = dhcp.maybe_perform_dhcp_discovery(
+                self.fallback_interface)
             if not dhcp_leases:
                 # DataSourceEc2Local failed in init-local stage. DataSourceEc2
                 # will still run in init-network stage.
                 return False
             dhcp_opts = dhcp_leases[-1]
-            self.fallback_nic = dhcp_opts.get('interface')
             net_params = {'interface': dhcp_opts.get('interface'),
                           'ip': dhcp_opts.get('fixed-address'),
                           'prefix_or_mask': dhcp_opts.get('subnet-mask'),
@@ -301,21 +300,44 @@ class DataSourceEc2(sources.DataSource):
             return None
 
         result = None
-        net_md = self.metadata.get('network')
+        no_network_metadata_on_aws = bool(
+            'network' not in self.metadata and
+            self.cloud_platform == Platforms.AWS)
+        if no_network_metadata_on_aws:
+            LOG.debug("Metadata 'network' not present:"
+                      " Refreshing stale metadata from prior to upgrade.")
+            util.log_time(
+                logfunc=LOG.debug, msg='Re-crawl of metadata service',
+                func=self._crawl_metadata)
+
         # Limit network configuration to only the primary/fallback nic
-        macs_to_nics = {
-            net.get_interface_mac(self.fallback_nic): self.fallback_nic}
+        iface = self.fallback_interface
+        macs_to_nics = {net.get_interface_mac(iface): iface}
+        net_md = self.metadata.get('network')
         if isinstance(net_md, dict):
             result = convert_ec2_metadata_network_config(
-                net_md, macs_to_nics=macs_to_nics,
-                fallback_nic=self.fallback_nic)
+                net_md, macs_to_nics=macs_to_nics, fallback_nic=iface)
         else:
-            LOG.warning("unexpected metadata 'network' key not valid: %s",
-                        net_md)
+            LOG.warning("Metadata 'network' key not valid: %s.", net_md)
         self._network_config = result
 
         return self._network_config
 
+    @property
+    def fallback_interface(self):
+        if self._fallback_interface is None:
+            # fallback_nic was used at one point, so restored objects may
+            # have an attribute there. respect that if found.
+            _legacy_fbnic = getattr(self, 'fallback_nic', None)
+            if _legacy_fbnic:
+                self._fallback_interface = _legacy_fbnic
+                self.fallback_nic = None
+            else:
+                self._fallback_interface = net.find_fallback_nic()
+                if self._fallback_interface is None:
+                    LOG.warning("Did not find a fallback interface on EC2.")
+        return self._fallback_interface
+
     def _crawl_metadata(self):
         """Crawl metadata service when available.
 
diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py
index 6af699a..ba328ee 100644
--- a/tests/unittests/test_datasource/test_ec2.py
+++ b/tests/unittests/test_datasource/test_ec2.py
@@ -307,6 +307,39 @@ class TestEc2(test_helpers.HttprettyTestCase):
 
     @httpretty.activate
     @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery')
+    def test_network_config_cached_property_refreshed_on_upgrade(self, m_dhcp):
+        """Refresh the network_config Ec2 cache if network key is absent.
+
+        This catches an upgrade issue where obj.pkl contained stale metadata
+        which lacked newly required network key.
+        """
+        old_metadata = copy.deepcopy(DEFAULT_METADATA)
+        old_metadata.pop('network')
+        ds = self._setup_ds(
+            platform_data=self.valid_platform_data,
+            sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
+            md=old_metadata)
+        self.assertTrue(ds.get_data())
+        # Provide new revision of metadata that contains network data
+        register_mock_metaserver(
+            'http://169.254.169.254/2009-04-04/meta-data/', DEFAULT_METADATA)
+        mac1 = '06:17:04:d7:26:09'  # Defined in DEFAULT_METADATA
+        get_interface_mac_path = (
+            'cloudinit.sources.DataSourceEc2.net.get_interface_mac')
+        ds.fallback_nic = 'eth9'
+        with mock.patch(get_interface_mac_path) as m_get_interface_mac:
+            m_get_interface_mac.return_value = mac1
+            ds.network_config  # Will re-crawl network metadata
+        self.assertIn('Re-crawl of metadata service', self.logs.getvalue())
+        expected = {'version': 1, 'config': [
+            {'mac_address': '06:17:04:d7:26:09',
+             'name': 'eth9',
+             'subnets': [{'type': 'dhcp4'}, {'type': 'dhcp6'}],
+             'type': 'physical'}]}
+        self.assertEqual(expected, ds.network_config)
+
+    @httpretty.activate
+    @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery')
     def test_valid_platform_with_strict_true(self, m_dhcp):
         """Valid platform data should return true with strict_id true."""
         ds = self._setup_ds(