From 2e511097dc43bdcccb0ca30d1d67b4e88233e418 Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Thu, 13 Jan 2022 21:59:33 -0500 Subject: [PATCH] sources/azure: consolidate DHCP variants to EphemeralDHCPv4WithReporting - 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 --- cloudinit/sources/DataSourceAzure.py | 33 +++--------- cloudinit/sources/helpers/azure.py | 14 +++--- tests/unittests/sources/test_azure.py | 72 ++++++++++----------------- 3 files changed, 41 insertions(+), 78 deletions(-) diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 280cdf50c5b..22435284081 100755 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -23,7 +23,6 @@ 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 @@ 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 @@ def _check_if_nic_is_primary(self, ifname): # 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 @@ def exc_cb(msg, exception): 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 50058fe048c..e0a1abb69ec 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 @@ def __enter__(self): 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 ff7f64798be..df1c289b298 100644 --- a/tests/unittests/sources/test_azure.py +++ b/tests/unittests/sources/test_azure.py @@ -465,7 +465,7 @@ def setUp(self): ) @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 @@ def test_get_metadata_does_not_dhcp_if_network_is_up( ) @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 @@ def test_get_metadata_uses_instance_url( ) @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 @@ def test_get_network_metadata_uses_network_url( ) @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 @@ def test_get_default_metadata_uses_instance_url( ) @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 @@ def setUp(self): 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 @@ def _load_possible_azure_ds(seed_dir, cache_dir): 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 @@ def _dmi_mocks(key): "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 @@ def test_crawl_metadata_call_imds_once_no_reprovision(self): 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 @@ def test_crawl_metadata_call_imds_twice_with_reprovision( 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 @@ def test_crawl_metadata_on_reprovision_reports_ready( 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 @@ def test_crawl_metadata_on_reprovision_reports_ready( "_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 @@ def test_crawl_metadata_waits_for_nic_on_savable_vms( 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 @@ def test_detect_nics_when_marker_present( poll_imds_func, report_ready_func, m_write, - m_dhcp, ): """If reprovisioning, wait for nic attach if marker present""" @@ -1286,9 +1274,7 @@ def test_crawl_metadata_on_reprovision_reports_ready_using_lease( "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 @@ def test_dsaz_report_failure_no_net_uses_new_ephemeral_dhcp_lease(self): # 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 @@ def test_dsaz_report_failure_no_net_and_no_dhcp_uses_fallback_lease(self): 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 @@ def test_nic_detach_writes_marker(self, m_writefile, m_detach): @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 @@ def test_detect_nic_attach_reports_ready_and_waits_for_detach( @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 @@ def test_detect_nic_attach_skips_nic_detach_when_marker_present( @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 @@ def test_wait_for_nic_attach_if_no_fallback_interface( @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 @@ def network_metadata_ret(ifname, retries, type, exc_cb, infinite): 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 @@ def setUp(self): 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 @@ def test_poll_imds_skips_dhcp_if_ctx_present( 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,