Repository: Azure/WALinuxAgent
Pull request #2152 (2021-01-29 21:40:38)

Fix for unit test and refactor monitor thread
A bad assert has been hiding failures in this test. I added comments to the PR pointing out the issues.

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=h1) Report
> Merging [#2152](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=desc) (cb2a996) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/54b57565e4b3d7a455f333cbf64ae693414e15b5?el=desc) (54b5756) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152/graphs/tree.svg?width=650&height=150&src=pr&token=ae90E2KYCw)](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #2152      +/-   ##
===========================================
- Coverage    70.52%   70.49%   -0.04%     
===========================================
  Files           95       95              
  Lines        13064    13064              
  Branches      1859     1859              
===========================================
- Hits          9214     9210       -4     
- Misses        3437     3438       +1     
- Partials       413      416       +3     
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/ga/collect\_telemetry\_events.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL2NvbGxlY3RfdGVsZW1ldHJ5X2V2ZW50cy5weQ==) | `89.65% <0.00%> (-1.73%)` | :arrow_down: |
| [azurelinuxagent/ga/monitor.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL21vbml0b3IucHk=) | `76.77% <0.00%> (-1.30%)` | :arrow_down: |
| [azurelinuxagent/common/event.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9ldmVudC5weQ==) | `87.35% <0.00%> (+0.67%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=footer). Last update [54b5756...cb2a996](https://codecov.io/gh/Azure/WALinuxAgent/pull/2152?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/azurelinuxagent/ga/monitor.py b/azurelinuxagent/ga/monitor.py
index 8b9edc5b..087bb6f4 100644
--- a/azurelinuxagent/ga/monitor.py
+++ b/azurelinuxagent/ga/monitor.py
@@ -17,7 +17,6 @@
 
 import datetime
 import threading
-import uuid
 
 import azurelinuxagent.common.logger as logger
 import azurelinuxagent.common.utils.networkutil as networkutil
@@ -100,7 +99,6 @@ class ReportNetworkConfigurationChangesOperation(PeriodicOperation):
     """
     Periodic operation to check and log changes in network configuration.
     """
-
     def __init__(self):
         super(ReportNetworkConfigurationChangesOperation, self).__init__(
             name="report network configuration changes",
@@ -125,16 +123,102 @@ class ReportNetworkConfigurationChangesOperation(PeriodicOperation):
             self.last_nic_state = nic_state
 
 
-class MonitorHandler(ThreadHandlerInterface):
-    # telemetry
-    EVENT_COLLECTION_PERIOD = datetime.timedelta(minutes=1)
-    # host plugin
-    HOST_PLUGIN_HEARTBEAT_PERIOD = datetime.timedelta(minutes=1)
-    HOST_PLUGIN_HEALTH_PERIOD = datetime.timedelta(minutes=5)
-    # imds
-    IMDS_HEARTBEAT_PERIOD = datetime.timedelta(minutes=1)
-    IMDS_HEALTH_PERIOD = datetime.timedelta(minutes=3)
+class SendHostPluginHeartbeatOperation(PeriodicOperation):
+    """
+    Periodic operation for reporting the HostGAPlugin's health. The signal is 'Healthy' when we have been able to communicate with
+    plugin at least once in the last _HOST_PLUGIN_HEALTH_PERIOD.
+    """
+    def __init__(self, protocol, health_service):
+        super(SendHostPluginHeartbeatOperation, self).__init__(
+            name="send_host_plugin_heartbeat",
+            operation=self._operation_impl,
+            period=SendHostPluginHeartbeatOperation._HOST_PLUGIN_HEARTBEAT_PERIOD)
+        self.protocol = protocol
+        self.health_service = health_service
+        self.host_plugin_error_state = ErrorState(min_timedelta=SendHostPluginHeartbeatOperation._HOST_PLUGIN_HEALTH_PERIOD)
+
+    _HOST_PLUGIN_HEARTBEAT_PERIOD = datetime.timedelta(minutes=1)
+    _HOST_PLUGIN_HEALTH_PERIOD = datetime.timedelta(minutes=5)
+
+    def _operation_impl(self):
+        try:
+            host_plugin = self.protocol.client.get_host_plugin()
+            host_plugin.ensure_initialized()
+            is_currently_healthy = host_plugin.get_health()
+
+            if is_currently_healthy:
+                self.host_plugin_error_state.reset()
+            else:
+                self.host_plugin_error_state.incr()
+
+            is_healthy = self.host_plugin_error_state.is_triggered() is False
+            logger.verbose("HostGAPlugin health: {0}", is_healthy)
+
+            self.health_service.report_host_plugin_heartbeat(is_healthy)
+
+            if not is_healthy:
+                add_event(
+                    name=AGENT_NAME,
+                    version=CURRENT_VERSION,
+                    op=WALAEventOperation.HostPluginHeartbeatExtended,
+                    is_success=False,
+                    message='{0} since successful heartbeat'.format(self.host_plugin_error_state.fail_time),
+                    log_event=False)
+
+        except Exception as e:
+            msg = "Exception sending host plugin heartbeat: {0}".format(ustr(e))
+            add_event(
+                name=AGENT_NAME,
+                version=CURRENT_VERSION,
+                op=WALAEventOperation.HostPluginHeartbeat,
+                is_success=False,
+                message=msg,
+                log_event=False)
+
+
+class SendImdsHeartbeatOperation(PeriodicOperation):
+    """
+    Periodic operation to report the IDMS's health. The signal is 'Healthy' when we have successfully called and validated
+    a response in the last _IMDS_HEALTH_PERIOD.
+    """
+    def __init__(self, protocol_util, health_service):
+        super(SendImdsHeartbeatOperation, self).__init__(
+            name="send_imds_heartbeat",
+            operation=self._operation_impl,
+            period=SendImdsHeartbeatOperation._IMDS_HEARTBEAT_PERIOD)
+        self.health_service = health_service
+        self.imds_client = get_imds_client(protocol_util.get_wireserver_endpoint())
+        self.imds_error_state = ErrorState(min_timedelta=SendImdsHeartbeatOperation._IMDS_HEALTH_PERIOD)
+
+    _IMDS_HEARTBEAT_PERIOD = datetime.timedelta(minutes=1)
+    _IMDS_HEALTH_PERIOD = datetime.timedelta(minutes=3)
+
+    def _operation_impl(self):
+        try:
+            is_currently_healthy, response = self.imds_client.validate()
+
+            if is_currently_healthy:
+                self.imds_error_state.reset()
+            else:
+                self.imds_error_state.incr()
+
+            is_healthy = self.imds_error_state.is_triggered() is False
+            logger.verbose("IMDS health: {0} [{1}]", is_healthy, response)
+
+            self.health_service.report_imds_status(is_healthy, response)
+
+        except Exception as e:
+            msg = "Exception sending imds heartbeat: {0}".format(ustr(e))
+            add_event(
+                name=AGENT_NAME,
+                version=CURRENT_VERSION,
+                op=WALAEventOperation.ImdsHeartbeat,
+                is_success=False,
+                message=msg,
+                log_event=False)
 
+
+class MonitorHandler(ThreadHandlerInterface):
     _THREAD_NAME = "MonitorHandler"
 
     @staticmethod
@@ -142,27 +226,8 @@ class MonitorHandler(ThreadHandlerInterface):
         return MonitorHandler._THREAD_NAME
 
     def __init__(self):
-        self.osutil = get_osutil()
-        self.imds_client = None
-
-        self.event_thread = None
-        self._periodic_operations = [
-            ResetPeriodicLogMessagesOperation(),
-            ReportNetworkErrorsOperation(),
-            PeriodicOperation("send_host_plugin_heartbeat", self.send_host_plugin_heartbeat, self.HOST_PLUGIN_HEARTBEAT_PERIOD),
-            PeriodicOperation("send_imds_heartbeat", self.send_imds_heartbeat, self.IMDS_HEARTBEAT_PERIOD),
-            ReportNetworkConfigurationChangesOperation(),
-            PollResourceUsageOperation()
-        ]
-
-        self.protocol = None
-        self.protocol_util = None
-        self.health_service = None
-
+        self.monitor_thread = None
         self.should_run = True
-        self.heartbeat_id = str(uuid.uuid4()).upper()
-        self.host_plugin_errorstate = ErrorState(min_timedelta=MonitorHandler.HOST_PLUGIN_HEALTH_PERIOD)
-        self.imds_errorstate = ErrorState(min_timedelta=MonitorHandler.IMDS_HEALTH_PERIOD)
 
     def run(self):
         self.start()
@@ -173,118 +238,48 @@ class MonitorHandler(ThreadHandlerInterface):
             self.join()
 
     def join(self):
-        self.event_thread.join()
+        self.monitor_thread.join()
 
     def stopped(self):
         return not self.should_run
 
-    def init_protocols(self):
-        # The initialization of ProtocolUtil for the Monitor thread should be done within the thread itself rather
-        # than initializing it in the ExtHandler thread. This is done to avoid any concurrency issues as each
-        # thread would now have its own ProtocolUtil object as per the SingletonPerThread model.
-        self.protocol_util = get_protocol_util()
-        self.protocol = self.protocol_util.get_protocol()
-        self.health_service = HealthService(self.protocol.get_endpoint())
-
-    def init_imds_client(self):
-        wireserver_endpoint = self.protocol_util.get_wireserver_endpoint()
-        self.imds_client = get_imds_client(wireserver_endpoint)
-
     def is_alive(self):
-        return self.event_thread is not None and self.event_thread.is_alive()
+        return self.monitor_thread is not None and self.monitor_thread.is_alive()
 
     def start(self):
-        self.event_thread = threading.Thread(target=self.daemon)
-        self.event_thread.setDaemon(True)
-        self.event_thread.setName(self.get_thread_name())
-        self.event_thread.start()
+        self.monitor_thread = threading.Thread(target=self.daemon)
+        self.monitor_thread.setDaemon(True)
+        self.monitor_thread.setName(self.get_thread_name())
+        self.monitor_thread.start()
 
     def daemon(self):
         try:
-            if self.protocol_util is None or self.protocol is None:
-                self.init_protocols()
-
-            if self.imds_client is None:
-                self.init_imds_client()
+            # The protocol needs to be instantiated in the monitor thread itself (to avoid concurrency issues with the protocol object each
+            # thread uses a different instance as per the SingletonPerThread model.
+            protocol_util = get_protocol_util()
+            protocol = protocol_util.get_protocol()
+            health_service = HealthService(protocol.get_endpoint())
+            periodic_operations = [
+                ResetPeriodicLogMessagesOperation(),
+                ReportNetworkErrorsOperation(),
+                ReportNetworkConfigurationChangesOperation(),
+                PollResourceUsageOperation(),
+                SendHostPluginHeartbeatOperation(protocol, health_service),
+                SendImdsHeartbeatOperation(protocol_util, health_service)
+            ]
 
             while not self.stopped():
                 try:
-                    self.protocol.update_host_plugin_from_goal_state()
+                    protocol.update_host_plugin_from_goal_state()
 
-                    for op in self._periodic_operations:
+                    for op in periodic_operations:
                         op.run()
 
                 except Exception as e:
                     logger.error("An error occurred in the monitor thread main loop; will skip the current iteration.\n{0}", ustr(e))
                 finally:
-                    PeriodicOperation.sleep_until_next_operation(self._periodic_operations)
+                    PeriodicOperation.sleep_until_next_operation(periodic_operations)
         except Exception as e:
             logger.error("An error occurred in the monitor thread; will exit the thread.\n{0}", ustr(e))
 
-    def send_imds_heartbeat(self):
-        """
-        Send a health signal every IMDS_HEARTBEAT_PERIOD. The signal is 'Healthy' when we have
-        successfully called and validated a response in the last IMDS_HEALTH_PERIOD.
-        """
-        try:
-            is_currently_healthy, response = self.imds_client.validate()
-
-            if is_currently_healthy:
-                self.imds_errorstate.reset()
-            else:
-                self.imds_errorstate.incr()
-
-            is_healthy = self.imds_errorstate.is_triggered() is False
-            logger.verbose("IMDS health: {0} [{1}]", is_healthy, response)
-
-            self.health_service.report_imds_status(is_healthy, response)
-
-        except Exception as e:
-            msg = "Exception sending imds heartbeat: {0}".format(ustr(e))
-            add_event(
-                name=AGENT_NAME,
-                version=CURRENT_VERSION,
-                op=WALAEventOperation.ImdsHeartbeat,
-                is_success=False,
-                message=msg,
-                log_event=False)
-
-    def send_host_plugin_heartbeat(self):
-        """
-        Send a health signal every HOST_PLUGIN_HEARTBEAT_PERIOD. The signal is 'Healthy' when we have been able to
-        communicate with HostGAPlugin at least once in the last HOST_PLUGIN_HEALTH_PERIOD.
-        """
-        try:
-            host_plugin = self.protocol.client.get_host_plugin()
-            host_plugin.ensure_initialized()
-            is_currently_healthy = host_plugin.get_health()
-
-            if is_currently_healthy:
-                self.host_plugin_errorstate.reset()
-            else:
-                self.host_plugin_errorstate.incr()
-
-            is_healthy = self.host_plugin_errorstate.is_triggered() is False
-            logger.verbose("HostGAPlugin health: {0}", is_healthy)
-
-            self.health_service.report_host_plugin_heartbeat(is_healthy)
-
-            if not is_healthy:
-                add_event(
-                    name=AGENT_NAME,
-                    version=CURRENT_VERSION,
-                    op=WALAEventOperation.HostPluginHeartbeatExtended,
-                    is_success=False,
-                    message='{0} since successful heartbeat'.format(self.host_plugin_errorstate.fail_time),
-                    log_event=False)
-
-        except Exception as e:
-            msg = "Exception sending host plugin heartbeat: {0}".format(ustr(e))
-            add_event(
-                name=AGENT_NAME,
-                version=CURRENT_VERSION,
-                op=WALAEventOperation.HostPluginHeartbeat,
-                is_success=False,
-                message=msg,
-                log_event=False)
 


Test patch:
diff --git a/tests/ga/test_monitor.py b/tests/ga/test_monitor.py
index cf7218bb..f0b9e72d 100644
--- a/tests/ga/test_monitor.py
+++ b/tests/ga/test_monitor.py
@@ -16,7 +16,6 @@
 # Requires Python 2.6+ and Openssl 1.0+
 #
 import contextlib
-import datetime
 import os
 import random
 import string
@@ -25,11 +24,11 @@ from azurelinuxagent.common import event, logger
 from azurelinuxagent.common.cgroup import CGroup, CpuCgroup, MemoryCgroup, MetricValue
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.event import EVENTS_DIRECTORY
-from azurelinuxagent.common.logger import Logger
+from azurelinuxagent.common.protocol.healthservice import HealthService
 from azurelinuxagent.common.protocol.util import ProtocolUtil
 from azurelinuxagent.common.protocol.wire import WireProtocol
-from azurelinuxagent.ga.monitor import get_monitor_handler, MonitorHandler, PeriodicOperation, \
-    ResetPeriodicLogMessagesOperation, PollResourceUsageOperation
+from azurelinuxagent.ga.monitor import get_monitor_handler, PeriodicOperation, \
+    ResetPeriodicLogMessagesOperation, SendHostPluginHeartbeatOperation, PollResourceUsageOperation
 from tests.protocol.mocks import mock_wire_protocol, HttpRequestPredicates, MockHttpResponse
 from tests.protocol.mockwiredata import DATA_FILE
 from tests.tools import Mock, MagicMock, patch, AgentTestCase, clear_singleton_instances
@@ -40,115 +39,129 @@ def random_generator(size=6, chars=string.ascii_uppercase + string.digits + stri
 
 
 @contextlib.contextmanager
-def _create_monitor_handler(enabled_operations=None, iterations=1):
-    """
-    Creates an instance of MonitorHandler that
-        * Uses a mock_wire_protocol for network requests,
-        * Executes only the operations given in the 'enabled_operations' parameter,
-        * Runs its main loop only the number of times given in the 'iterations' parameter, and
-        * Does not sleep at the end of each iteration
-
-    The returned MonitorHandler is augmented with 2 methods:
-        * get_mock_wire_protocol() - returns the mock protocol
-        * run_and_wait() - invokes run() and wait() on the MonitorHandler
-
-    """
-    if enabled_operations is None:
-        enabled_operations = []
-
-    def run(self):
-        if len(enabled_operations) == 0 or self._name in enabled_operations:  # pylint: disable=protected-access
-            run.original_definition(self)
-    run.original_definition = PeriodicOperation.run
+def _mock_wire_protocol():
+    # Since ProtocolUtil is a singleton per thread, we need to clear it to ensure that the test cases do not
+    # reuse a previous state
+    clear_singleton_instances(ProtocolUtil)
 
     with mock_wire_protocol(DATA_FILE) as protocol:
         protocol_util = MagicMock()
         protocol_util.get_protocol = Mock(return_value=protocol)
         with patch("azurelinuxagent.ga.monitor.get_protocol_util", return_value=protocol_util):
-            with patch.object(PeriodicOperation, "run", side_effect=run, autospec=True):
-                with patch("azurelinuxagent.ga.monitor.MonitorHandler.stopped", side_effect=[False] * iterations + [True]):
-                    with patch("time.sleep"):
-                        def run_and_wait():
-                            monitor_handler.run()
-                            monitor_handler.join()
+            yield protocol
 
-                        monitor_handler = get_monitor_handler()
-                        monitor_handler.get_mock_wire_protocol = lambda: protocol
-                        monitor_handler.run_and_wait = run_and_wait
-                        yield monitor_handler
-
-
-class TestMonitor(AgentTestCase, HttpRequestPredicates):
-    def setUp(self):
-        AgentTestCase.setUp(self)
-        prefix = "UnitTest"
-        logger.DEFAULT_LOGGER = Logger(prefix=prefix)
-
-        # Since ProtocolUtil is a singleton per thread, we need to clear it to ensure that the test cases do not
-        # reuse a previous state
-        clear_singleton_instances(ProtocolUtil)
-
-    def tearDown(self):
-        AgentTestCase.tearDown(self)
 
+class MonitorHandlerTestCase(AgentTestCase):
     def test_it_should_invoke_all_periodic_operations(self):
+        def periodic_operation_run(self):
+            invoked_operations.append(self._name)
         invoked_operations = []
 
-        with _create_monitor_handler() as monitor_handler:
-            def mock_run(self):
-                invoked_operations.append(self._name)
+        with _mock_wire_protocol():
+            with patch("azurelinuxagent.ga.monitor.MonitorHandler.stopped", side_effect=[False, True]):
+                with patch("time.sleep"):
+                    with patch.object(PeriodicOperation, "run", side_effect=periodic_operation_run, autospec=True):
+                        invoked_operations = []
 
-            with patch.object(PeriodicOperation, "run", side_effect=mock_run, spec=MonitorHandler.run):
-                monitor_handler.run_and_wait()
+                        monitor_handler = get_monitor_handler()
+                        monitor_handler.run()
+                        monitor_handler.join()
+
+                        expected_operations = [
+                            'poll resource usage',
+                            'report network configuration changes',
+                            'report network errors',
+                            'reset periodic log messages',
+                            'send_host_plugin_heartbeat',
+                            'send_imds_heartbeat'
+                        ]
+
+                        invoked_operations.sort()
+                        expected_operations.sort()
 
-                expected_operations = [
-                    "reset_loggers", "collect_and_send_events", "send_telemetry_heartbeat",
-                    "poll_telemetry_metrics usage", "send_telemetry_metrics usage", "send_host_plugin_heartbeat",
-                    "send_imds_heartbeat", "log_altered_network_configuration"
-                ]
+                        self.assertEqual(invoked_operations, expected_operations, "The monitor thread did not invoke the expected operations")
 
-                self.assertEqual(invoked_operations.sort(), expected_operations.sort(), "The monitor thread did not invoke the expected operations")
 
+class SendHostPluginHeartbeatOperationTestCase(AgentTestCase, HttpRequestPredicates):
     def test_it_should_report_host_ga_health(self):
-        with _create_monitor_handler(enabled_operations=["send_host_plugin_heartbeat"]) as monitor_handler:
+        with _mock_wire_protocol() as protocol:
             def http_post_handler(url, _, **__):
                 if self.is_health_service_request(url):
                     http_post_handler.health_service_posted = True
                     return MockHttpResponse(status=200)
                 return None
             http_post_handler.health_service_posted = False
+            protocol.set_http_handlers(http_post_handler=http_post_handler)
 
-            monitor_handler.get_mock_wire_protocol().set_http_handlers(http_post_handler=http_post_handler)
+            health_service = HealthService(protocol.get_endpoint())
 
-            monitor_handler.run_and_wait()
+            SendHostPluginHeartbeatOperation(protocol, health_service).run()
 
             self.assertTrue(http_post_handler.health_service_posted, "The monitor thread did not report host ga plugin health")
 
     def test_it_should_report_a_telemetry_event_when_host_plugin_is_not_healthy(self):
-        with _create_monitor_handler(enabled_operations=["send_host_plugin_heartbeat"]) as monitor_handler:
-            def http_get_handler(url, *_, **__):
-                if self.is_host_plugin_health_request(url):
-                    return MockHttpResponse(status=503)
-                return None
-
-            monitor_handler.get_mock_wire_protocol().set_http_handlers(http_get_handler=http_get_handler)
-
+        with _mock_wire_protocol() as protocol:
             # the error triggers only after ERROR_STATE_DELTA_DEFAULT
             with patch('azurelinuxagent.common.errorstate.ErrorState.is_triggered', return_value=True):
                 with patch('azurelinuxagent.common.event.EventLogger.add_event') as add_event_patcher:
-                    monitor_handler.run_and_wait()
+                    def http_get_handler(url, *_, **__):
+                        if self.is_host_plugin_health_request(url):
+                            return MockHttpResponse(status=503)
+                        return None
+                    protocol.set_http_handlers(http_get_handler=http_get_handler)
+
+                    health_service = HealthService(protocol.get_endpoint())
+
+                    SendHostPluginHeartbeatOperation(protocol, health_service).run()
 
                     heartbeat_events = [kwargs for _, kwargs in add_event_patcher.call_args_list if kwargs['op'] == 'HostPluginHeartbeatExtended']
                     self.assertTrue(len(heartbeat_events) == 1, "The monitor thread should have reported exactly 1 telemetry event for an unhealthy host ga plugin")
                     self.assertFalse(heartbeat_events[0]['is_success'], 'The reported event should indicate failure')
 
+    def test_it_should_not_send_a_health_signal_when_the_hearbeat_fails(self):
+        with _mock_wire_protocol() as protocol:
+            with patch('azurelinuxagent.common.event.EventLogger.add_event') as add_event_patcher:
+                health_service_post_requests = []
+
+                def http_get_handler(url, *_, **__):
+                    if self.is_host_plugin_health_request(url):
+                        del health_service_post_requests[:]  # clear the requests; after this error there should be no more POSTs
+                        raise IOError('A CLIENT ERROR')
+                    return None
+
+                def http_post_handler(url, _, **__):
+                    if self.is_health_service_request(url):
+                        health_service_post_requests.append(url)
+                        return MockHttpResponse(status=200)
+                    return None
+
+                protocol.set_http_handlers(http_get_handler=http_get_handler, http_post_handler=http_post_handler)
+
+                health_service = HealthService(protocol.get_endpoint())
+
+                SendHostPluginHeartbeatOperation(protocol, health_service).run()
+
+                self.assertEqual(0, len(health_service_post_requests), "No health signals should have been posted: {0}".format(health_service_post_requests))
+
+                heartbeat_events = [kwargs for _, kwargs in add_event_patcher.call_args_list if kwargs['op'] == 'HostPluginHeartbeat']
+                self.assertTrue(len(heartbeat_events) == 1, "The monitor thread should have reported exactly 1 telemetry event for an unhealthy host ga plugin")
+                self.assertFalse(heartbeat_events[0]['is_success'], 'The reported event should indicate failure')
+                self.assertIn('A CLIENT ERROR', heartbeat_events[0]['message'], 'The failure does not include the expected message')
+
+
+class ResetPeriodicLogMessagesOperationTestCase(AgentTestCase, HttpRequestPredicates):
     def test_it_should_clear_periodic_log_messages(self):
+        logger.reset_periodic()
+
         # Adding 100 different messages
-        for i in range(100):
+        expected = 100
+        for i in range(expected):
             logger.periodic_info(logger.EVERY_DAY, "Test {0}".format(i))
 
-        if len(logger.DEFAULT_LOGGER.periodic_messages) != 100:
-            raise Exception('Test setup error: the periodic messages were not added')
+        actual = len(logger.DEFAULT_LOGGER.periodic_messages)
+
+        if actual != expected:
+            raise Exception('Test setup error: the periodic messages were not added. Got: {0} Expected: {1}'.format(actual, expected))
 
         ResetPeriodicLogMessagesOperation().run()
 
@@ -273,33 +286,3 @@ class TestExtensionMetricsDataTelemetry(AgentTestCase):
                             patch_get_memory_max_usage.return_value = max_memory_usage_values[i]  # example 450 MB
                             CGroupsTelemetry.poll_all_tracked()
 
-
-@patch("azurelinuxagent.common.utils.restutil.http_post")
-@patch('azurelinuxagent.common.protocol.wire.WireClient.get_goal_state')
-@patch('azurelinuxagent.common.event.EventLogger.add_event')
-@patch("azurelinuxagent.common.utils.restutil.http_get")
-class TestMonitorFailure(AgentTestCase):
-
-    @patch("azurelinuxagent.common.protocol.healthservice.HealthService.report_host_plugin_heartbeat")
-    def test_error_heartbeat_creates_no_signal(self, patch_report_heartbeat, patch_http_get, patch_add_event, *args):  # pylint: disable=unused-argument
-
-        monitor_handler = get_monitor_handler()
-        protocol = WireProtocol('endpoint')
-        protocol.update_goal_state = MagicMock()
-        with patch('azurelinuxagent.common.protocol.util.ProtocolUtil.get_protocol', return_value=protocol):
-            monitor_handler.init_protocols()
-            monitor_handler.last_host_plugin_heartbeat = datetime.datetime.utcnow() - datetime.timedelta(hours=1)
-
-            patch_http_get.side_effect = IOError('client error')
-            monitor_handler.send_host_plugin_heartbeat()
-
-            # health report should not be made
-            self.assertEqual(0, patch_report_heartbeat.call_count)
-
-            # telemetry with failure details is sent
-            self.assertEqual(1, patch_add_event.call_count)
-            self.assertEqual('HostPluginHeartbeat', patch_add_event.call_args[1]['op'])
-            self.assertTrue('client error' in patch_add_event.call_args[1]['message'])
-
-            self.assertEqual(False, patch_add_event.call_args[1]['is_success'])
-            monitor_handler.stop()