sources/azure: consolidate DHCP variants to EphemeralDHCPv4WithReporting (#1190)

- Update EphemeralDHCPv4WithReporting to subclass EphemeralDHCPv4 for
  consistency (non-functional change).

- Replace all usage of EphemeralDHCPv4 with EphemeralDHCPv4WithReporting.

- Converging to one DHCP class exposed an issue with ExitStack patches
  being mixed with decorators.  Specifically, it appeared that tests
  that did not enable azure.EphemeralDHCPv4WithReporting mocks had it
  applied anyways from previous tests.

  Presumably ExitStack was overwriting the actual value with the
  mock provided by the decorator?  For now, remove some mock patches
  that trigger failures, but future work should move towards a
  consistent approach to prevent undetected effects.

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>

3 files changed, 41 insertions, 78 deletions

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index 280cdf50..22435284 100755
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -23,7 +23,6 @@ from cloudinit import log as logging
 from cloudinit import net, sources, ssh_util, subp, util
 from cloudinit.event import EventScope, EventType
 from cloudinit.net import device_driver
-from cloudinit.net.dhcp import EphemeralDHCPv4
 from cloudinit.reporting import events
 from cloudinit.sources.helpers import netlink
 from cloudinit.sources.helpers.azure import (
@@ -32,7 +31,6 @@ from cloudinit.sources.helpers.azure import (
     azure_ds_reporter,
     azure_ds_telemetry_reporter,
     build_minimal_ovf,
-    dhcp_log_cb,
     get_boot_telemetry,
     get_metadata_from_fabric,
     get_system_info,
@@ -946,20 +944,10 @@ class DataSourceAzure(sources.DataSource):
         # VM provisioning if there is any DHCP failure when trying to determine
         # the primary NIC.
         try:
-            with events.ReportEventStack(
-                name="obtain-dhcp-lease",
-                description=(
-                    "obtain dhcp lease for %s when attempting to "
-                    "determine primary NIC during reprovision of "
-                    "a pre-provisioned VM"
-                )
-                % ifname,
-                parent=azure_ds_reporter,
-            ):
-                dhcp_ctx = EphemeralDHCPv4(
-                    iface=ifname, dhcp_log_func=dhcp_log_cb
-                )
-                dhcp_ctx.obtain_lease()
+            dhcp_ctx = EphemeralDHCPv4WithReporting(
+                azure_ds_reporter, iface=ifname
+            )
+            dhcp_ctx.obtain_lease()
         except Exception as e:
             report_diagnostic_event(
                 "Giving up. Failed to obtain dhcp lease "
@@ -1234,15 +1222,10 @@ class DataSourceAzure(sources.DataSource):
                 if not is_ephemeral_ctx_present:
                     # Save our EphemeralDHCPv4 context to avoid repeated dhcp
                     # later when we report ready
-                    with events.ReportEventStack(
-                        name="obtain-dhcp-lease",
-                        description="obtain dhcp lease",
-                        parent=azure_ds_reporter,
-                    ):
-                        self._ephemeral_dhcp_ctx = EphemeralDHCPv4(
-                            dhcp_log_func=dhcp_log_cb
-                        )
-                        lease = self._ephemeral_dhcp_ctx.obtain_lease()
+                    self._ephemeral_dhcp_ctx = EphemeralDHCPv4WithReporting(
+                        azure_ds_reporter
+                    )
+                    lease = self._ephemeral_dhcp_ctx.obtain_lease()
 
                 if vnet_switched:
                     dhcp_attempts += 1
diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py
index 50058fe0..e0a1abb6 100755
--- a/cloudinit/sources/helpers/azure.py
+++ b/cloudinit/sources/helpers/azure.py
@@ -1243,11 +1243,12 @@ def dhcp_log_cb(out, err):
     )
 
 
-class EphemeralDHCPv4WithReporting:
-    def __init__(self, reporter, nic=None):
+class EphemeralDHCPv4WithReporting(EphemeralDHCPv4):
+    def __init__(self, reporter, iface=None):
         self.reporter = reporter
-        self.ephemeralDHCPv4 = EphemeralDHCPv4(
-            iface=nic, dhcp_log_func=dhcp_log_cb
+
+        super(EphemeralDHCPv4WithReporting, self).__init__(
+            iface=iface, dhcp_log_func=dhcp_log_cb
         )
 
     def __enter__(self):
@@ -1256,10 +1257,7 @@ class EphemeralDHCPv4WithReporting:
             description="obtain dhcp lease",
             parent=self.reporter,
         ):
-            return self.ephemeralDHCPv4.__enter__()
-
-    def __exit__(self, excp_type, excp_value, excp_traceback):
-        self.ephemeralDHCPv4.__exit__(excp_type, excp_value, excp_traceback)
+            return super(EphemeralDHCPv4WithReporting, self).__enter__()
 
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/sources/test_azure.py b/tests/unittests/sources/test_azure.py
index ff7f6479..df1c289b 100644
--- a/tests/unittests/sources/test_azure.py
+++ b/tests/unittests/sources/test_azure.py
@@ -465,7 +465,7 @@ class TestGetMetadataFromIMDS(HttprettyTestCase):
         )
 
     @mock.patch(MOCKPATH + "readurl")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4", autospec=True)
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "net.is_up", autospec=True)
     def test_get_metadata_does_not_dhcp_if_network_is_up(
         self, m_net_is_up, m_dhcp, m_readurl
@@ -487,7 +487,7 @@ class TestGetMetadataFromIMDS(HttprettyTestCase):
         )
 
     @mock.patch(MOCKPATH + "readurl", autospec=True)
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "net.is_up")
     def test_get_metadata_uses_instance_url(
         self, m_net_is_up, m_dhcp, m_readurl
@@ -512,7 +512,7 @@ class TestGetMetadataFromIMDS(HttprettyTestCase):
         )
 
     @mock.patch(MOCKPATH + "readurl", autospec=True)
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "net.is_up")
     def test_get_network_metadata_uses_network_url(
         self, m_net_is_up, m_dhcp, m_readurl
@@ -538,7 +538,7 @@ class TestGetMetadataFromIMDS(HttprettyTestCase):
         )
 
     @mock.patch(MOCKPATH + "readurl", autospec=True)
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "net.is_up")
     def test_get_default_metadata_uses_instance_url(
         self, m_net_is_up, m_dhcp, m_readurl
@@ -561,7 +561,7 @@ class TestGetMetadataFromIMDS(HttprettyTestCase):
         )
 
     @mock.patch(MOCKPATH + "readurl", autospec=True)
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "net.is_up")
     def test_get_metadata_uses_extended_url(
         self, m_net_is_up, m_dhcp, m_readurl
@@ -698,6 +698,15 @@ class TestAzureDataSource(CiTestCase):
         self.patches.enter_context(
             mock.patch.object(dsaz, "_get_random_seed", return_value="wild")
         )
+
+        self.m_dhcp = self.patches.enter_context(
+            mock.patch.object(
+                dsaz,
+                "EphemeralDHCPv4WithReporting",
+                autospec=True,
+            )
+        )
+
         self.m_get_metadata_from_imds = self.patches.enter_context(
             mock.patch.object(
                 dsaz,
@@ -799,8 +808,6 @@ scbus-1 on xpt0 bus 0
             return_value={"public-keys": []}
         )
         self.m_report_failure_to_fabric = mock.MagicMock(autospec=True)
-        self.m_ephemeral_dhcpv4 = mock.MagicMock()
-        self.m_ephemeral_dhcpv4_with_reporting = mock.MagicMock()
         self.m_list_possible_azure_ds = mock.MagicMock(
             side_effect=_load_possible_azure_ds
         )
@@ -834,12 +841,6 @@ scbus-1 on xpt0 bus 0
                     "report_failure_to_fabric",
                     self.m_report_failure_to_fabric,
                 ),
-                (dsaz, "EphemeralDHCPv4", self.m_ephemeral_dhcpv4),
-                (
-                    dsaz,
-                    "EphemeralDHCPv4WithReporting",
-                    self.m_ephemeral_dhcpv4_with_reporting,
-                ),
                 (dsaz, "get_boot_telemetry", mock.MagicMock()),
                 (dsaz, "get_system_info", mock.MagicMock()),
                 (dsaz.subp, "which", lambda x: True),
@@ -1138,16 +1139,13 @@ scbus-1 on xpt0 bus 0
         dsrc.crawl_metadata()
         self.assertEqual(1, self.m_get_metadata_from_imds.call_count)
 
-    @mock.patch(
-        "cloudinit.sources.DataSourceAzure.EphemeralDHCPv4WithReporting"
-    )
     @mock.patch("cloudinit.sources.DataSourceAzure.util.write_file")
     @mock.patch(
         "cloudinit.sources.DataSourceAzure.DataSourceAzure._report_ready"
     )
     @mock.patch("cloudinit.sources.DataSourceAzure.DataSourceAzure._poll_imds")
     def test_crawl_metadata_call_imds_twice_with_reprovision(
-        self, poll_imds_func, m_report_ready, m_write, m_dhcp
+        self, poll_imds_func, m_report_ready, m_write
     ):
         """If reprovisioning, imds metadata will be fetched twice"""
         ovfenv = construct_valid_ovf_env(
@@ -1160,16 +1158,13 @@ scbus-1 on xpt0 bus 0
         dsrc.crawl_metadata()
         self.assertEqual(2, self.m_get_metadata_from_imds.call_count)
 
-    @mock.patch(
-        "cloudinit.sources.DataSourceAzure.EphemeralDHCPv4WithReporting"
-    )
     @mock.patch("cloudinit.sources.DataSourceAzure.util.write_file")
     @mock.patch(
         "cloudinit.sources.DataSourceAzure.DataSourceAzure._report_ready"
     )
     @mock.patch("cloudinit.sources.DataSourceAzure.DataSourceAzure._poll_imds")
     def test_crawl_metadata_on_reprovision_reports_ready(
-        self, poll_imds_func, m_report_ready, m_write, m_dhcp
+        self, poll_imds_func, m_report_ready, m_write
     ):
         """If reprovisioning, report ready at the end"""
         ovfenv = construct_valid_ovf_env(
@@ -1182,9 +1177,6 @@ scbus-1 on xpt0 bus 0
         dsrc.crawl_metadata()
         self.assertEqual(1, m_report_ready.call_count)
 
-    @mock.patch(
-        "cloudinit.sources.DataSourceAzure.EphemeralDHCPv4WithReporting"
-    )
     @mock.patch("cloudinit.sources.DataSourceAzure.util.write_file")
     @mock.patch(
         "cloudinit.sources.DataSourceAzure.DataSourceAzure._report_ready"
@@ -1195,7 +1187,7 @@ scbus-1 on xpt0 bus 0
         "_wait_for_all_nics_ready"
     )
     def test_crawl_metadata_waits_for_nic_on_savable_vms(
-        self, detect_nics, poll_imds_func, report_ready_func, m_write, m_dhcp
+        self, detect_nics, poll_imds_func, report_ready_func, m_write
     ):
         """If reprovisioning, report ready at the end"""
         ovfenv = construct_valid_ovf_env(
@@ -1212,9 +1204,6 @@ scbus-1 on xpt0 bus 0
         self.assertEqual(1, report_ready_func.call_count)
         self.assertEqual(1, detect_nics.call_count)
 
-    @mock.patch(
-        "cloudinit.sources.DataSourceAzure.EphemeralDHCPv4WithReporting"
-    )
     @mock.patch("cloudinit.sources.DataSourceAzure.util.write_file")
     @mock.patch(
         "cloudinit.sources.DataSourceAzure.DataSourceAzure._report_ready"
@@ -1232,7 +1221,6 @@ scbus-1 on xpt0 bus 0
         poll_imds_func,
         report_ready_func,
         m_write,
-        m_dhcp,
     ):
         """If reprovisioning, wait for nic attach if marker present"""
 
@@ -1286,9 +1274,7 @@ scbus-1 on xpt0 bus 0
                 "subnet-mask": "255.255.255.0",
                 "unknown-245": "624c3620",
             }
-            self.m_ephemeral_dhcpv4_with_reporting.return_value.__enter__.return_value = (  # noqa: E501
-                lease
-            )
+            self.m_dhcp.return_value.__enter__.return_value = lease
             m_media_switch.return_value = None
 
             reprovision_ovfenv = construct_valid_ovf_env()
@@ -1793,9 +1779,7 @@ scbus-1 on xpt0 bus 0
             # setup ephemeral dhcp lease discovery mock
             test_lease_dhcp_option_245 = "test_lease_dhcp_option_245"
             test_lease = {"unknown-245": test_lease_dhcp_option_245}
-            self.m_ephemeral_dhcpv4_with_reporting.return_value.__enter__.return_value = (  # noqa: E501
-                test_lease
-            )
+            self.m_dhcp.return_value.__enter__.return_value = test_lease
 
             self.assertTrue(dsrc._report_failure())
 
@@ -1820,9 +1804,7 @@ scbus-1 on xpt0 bus 0
             m_dsrc_distro_networking_is_up.return_value = False
             # ephemeral dhcp discovery failure,
             # so cannot use a new ephemeral dhcp
-            self.m_ephemeral_dhcpv4_with_reporting.return_value.__enter__.side_effect = (  # noqa: E501
-                Exception
-            )
+            self.m_dhcp.return_value.__enter__.side_effect = Exception
 
             self.assertTrue(dsrc._report_failure())
 
@@ -2867,7 +2849,7 @@ class TestPreprovisioningHotAttachNics(CiTestCase):
 
     @mock.patch(MOCKPATH + "util.write_file", autospec=True)
     @mock.patch(MOCKPATH + "DataSourceAzure.fallback_interface")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "DataSourceAzure._report_ready")
     @mock.patch(MOCKPATH + "DataSourceAzure._wait_for_nic_detach")
     def test_detect_nic_attach_reports_ready_and_waits_for_detach(
@@ -2887,7 +2869,7 @@ class TestPreprovisioningHotAttachNics(CiTestCase):
 
     @mock.patch("os.path.isfile")
     @mock.patch(MOCKPATH + "DataSourceAzure.fallback_interface")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "DataSourceAzure._report_ready")
     @mock.patch(MOCKPATH + "DataSourceAzure._wait_for_nic_detach")
     def test_detect_nic_attach_skips_report_ready_when_marker_present(
@@ -2928,7 +2910,7 @@ class TestPreprovisioningHotAttachNics(CiTestCase):
     @mock.patch("cloudinit.sources.helpers.netlink.wait_for_nic_attach_event")
     @mock.patch("cloudinit.sources.net.find_fallback_nic")
     @mock.patch(MOCKPATH + "get_metadata_from_imds")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "DataSourceAzure._wait_for_nic_detach")
     @mock.patch("os.path.isfile")
     def test_wait_for_nic_attach_if_no_fallback_interface(
@@ -2972,7 +2954,7 @@ class TestPreprovisioningHotAttachNics(CiTestCase):
     @mock.patch("cloudinit.sources.helpers.netlink.wait_for_nic_attach_event")
     @mock.patch("cloudinit.sources.net.find_fallback_nic")
     @mock.patch(MOCKPATH + "DataSourceAzure.get_imds_data_with_api_fallback")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     @mock.patch(MOCKPATH + "DataSourceAzure._wait_for_nic_detach")
     @mock.patch("os.path.isfile")
     def test_wait_for_nic_attach_multinic_attach(
@@ -3036,7 +3018,7 @@ class TestPreprovisioningHotAttachNics(CiTestCase):
         self.assertEqual(2, m_link_up.call_count)
 
     @mock.patch(MOCKPATH + "DataSourceAzure.get_imds_data_with_api_fallback")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     def test_check_if_nic_is_primary_retries_on_failures(
         self, m_dhcpv4, m_imds
     ):
@@ -3218,7 +3200,7 @@ class TestPreprovisioningPollIMDS(CiTestCase):
         dsaz.BUILTIN_DS_CONFIG["data_dir"] = self.waagent_d
 
     @mock.patch("time.sleep", mock.MagicMock())
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     def test_poll_imds_re_dhcp_on_timeout(
         self,
         m_dhcpv4,
@@ -3293,7 +3275,7 @@ class TestPreprovisioningPollIMDS(CiTestCase):
         self.assertEqual(0, m_media_switch.call_count)
 
     @mock.patch("os.path.isfile")
-    @mock.patch(MOCKPATH + "EphemeralDHCPv4")
+    @mock.patch(MOCKPATH + "EphemeralDHCPv4WithReporting", autospec=True)
     def test_poll_imds_does_dhcp_on_retries_if_ctx_present(
         self,
         m_ephemeral_dhcpv4,