Repository: Azure/WALinuxAgent

Issue:
`systemctl stop waagent` hangs
Reproes on Ubuntu 16.04 as well as CentOS 7.

`systemctl stop walinuxagent` command hangs for a long time, although it eventually succeeds like after maybe 30-40 seconds. It used to be instant. I think it's related to the signal handling behavior that has been recently changed.

Discussion:
msftclas: Hi __@brendandixon__, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
    <span>You've already signed the contribution license agreement. Thanks!</span>
        <p>The agreement was validated by Microsoft and real humans are currently evaluating your PR.</p>

TTYL, MSBOT;


Patch:
diff --git a/azurelinuxagent/common/rdma.py b/azurelinuxagent/common/rdma.py
index 3ba332d3..c9451a21 100644
--- a/azurelinuxagent/common/rdma.py
+++ b/azurelinuxagent/common/rdma.py
@@ -21,15 +21,56 @@ Handle packages and modules to enable RDMA for IB networking
 
 import os
 import re
-import threading
 import time
+import threading
+
 import azurelinuxagent.common.logger as logger
 import azurelinuxagent.common.utils.fileutil as fileutil
 import azurelinuxagent.common.utils.shellutil as shellutil
 
-dapl_config_paths = ['/etc/dat.conf', '/etc/rdma/dat.conf',
-                     '/usr/local/etc/dat.conf']
+from azurelinuxagent.common.protocol.wire import SHARED_CONF_FILE_NAME
 
+dapl_config_paths = [
+    '/etc/dat.conf',
+    '/etc/rdma/dat.conf',
+    '/usr/local/etc/dat.conf'
+]
+
+def setup_rdma_device(self):
+    logger.verbose("Parsing SharedConfig XML contents for RDMA details")
+    xml_doc = parse_doc(
+        fileutil.read_file(os.path.join(conf.get_lib_dir(), SHARED_CONF_FILE_NAME)))
+    if xml_doc is None:
+        logger.error("Could not parse SharedConfig XML document")
+        return
+    instance_elem = find(xml_doc, "Instance")
+    if not instance_elem:
+        logger.error("Could not find <Instance> in SharedConfig document")
+        return
+
+    rdma_ipv4_addr = getattrib(instance_elem, "rdmaIPv4Address")
+    if not rdma_ipv4_addr:
+        logger.error(
+            "Could not find rdmaIPv4Address attribute on Instance element of SharedConfig.xml document")
+        return
+
+    rdma_mac_addr = getattrib(instance_elem, "rdmaMacAddress")
+    if not rdma_mac_addr:
+        logger.error(
+            "Could not find rdmaMacAddress attribute on Instance element of SharedConfig.xml document")
+        return
+
+    # add colons to the MAC address (e.g. 00155D33FF1D ->
+    # 00:15:5D:33:FF:1D)
+    rdma_mac_addr = ':'.join([rdma_mac_addr[i:i+2]
+                              for i in range(0, len(rdma_mac_addr), 2)])
+    logger.info("Found RDMA details. IPv4={0} MAC={1}".format(
+        rdma_ipv4_addr, rdma_mac_addr))
+
+    # Set up the RDMA device with collected informatino
+    RDMADeviceHandler(rdma_ipv4_addr, rdma_mac_addr).start()
+    logger.info("RDMA: device is set up")
+    return
 
 class RDMAHandler(object):
 
diff --git a/azurelinuxagent/daemon/main.py b/azurelinuxagent/daemon/main.py
index e1173ccb..d3185a1d 100644
--- a/azurelinuxagent/daemon/main.py
+++ b/azurelinuxagent/daemon/main.py
@@ -18,32 +18,31 @@
 #
 
 import os
-import time
 import sys
+import time
 import traceback
+
 import azurelinuxagent.common.conf as conf
+import azurelinuxagent.common.event as event
+import azurelinuxagent.common.utils.fileutil as fileutil
 import azurelinuxagent.common.logger as logger
+
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.event import add_event, WALAEventOperation
 from azurelinuxagent.common.exception import ProtocolError
+from azurelinuxagent.common.osutil import get_osutil
+from azurelinuxagent.common.protocol import get_protocol_util
+from azurelinuxagent.common.rdma import RDMADeviceHandler, setup_rdma_device
+from azurelinuxagent.common.utils.textutil import parse_doc, find, getattrib
 from azurelinuxagent.common.version import AGENT_LONG_NAME, AGENT_VERSION, \
                                      DISTRO_NAME, DISTRO_VERSION, \
                                      DISTRO_FULL_NAME, PY_VERSION_MAJOR, \
                                      PY_VERSION_MINOR, PY_VERSION_MICRO
-from azurelinuxagent.common.protocol.wire import SHARED_CONF_FILE_NAME
-import azurelinuxagent.common.event as event
-import azurelinuxagent.common.utils.fileutil as fileutil
-from azurelinuxagent.common.utils.textutil import parse_doc, find, getattrib
-from azurelinuxagent.common.osutil import get_osutil
-from azurelinuxagent.common.protocol import get_protocol_util
-from azurelinuxagent.daemon.scvmm import get_scvmm_handler
 from azurelinuxagent.daemon.resourcedisk import get_resourcedisk_handler
-from azurelinuxagent.daemon.monitor import get_monitor_handler
-from azurelinuxagent.daemon.env import get_env_handler
+from azurelinuxagent.daemon.scvmm import get_scvmm_handler
 from azurelinuxagent.pa.provision import get_provision_handler
 from azurelinuxagent.pa.rdma import get_rdma_handler
 from azurelinuxagent.ga.update import get_update_handler
-from azurelinuxagent.common.rdma import RDMADeviceHandler
 
 def get_daemon_handler():
     return DaemonHandler()
@@ -95,12 +94,10 @@ class DaemonHandler(object):
         self.scvmm_handler = get_scvmm_handler()
         self.resourcedisk_handler = get_resourcedisk_handler()
         self.rdma_handler = get_rdma_handler()
-        self.monitor_handler = get_monitor_handler()
-        self.env_handler = get_env_handler()
         self.provision_handler = get_provision_handler()
         self.update_handler = get_update_handler()
 
-        #Create lib dir
+        # Create lib dir
         if not os.path.isdir(conf.get_lib_dir()):
             fileutil.mkdir(conf.get_lib_dir(), mode=0o700)
             os.chdir(conf.get_lib_dir())
@@ -110,63 +107,24 @@ class DaemonHandler(object):
 
         if conf.get_resourcedisk_format():
             self.resourcedisk_handler.run()
-        
+
+        # Always redetermine the protocol start (e.g., wireserver vs.
+        # on-premise) since a VHD can move between environments        
         self.protocol_util.clear_protocol()
 
         self.provision_handler.run()
 
+        # Enable RDMA, continue in errors
         if conf.enable_rdma():
             self.rdma_handler.install_driver()
 
-        self.monitor_handler.run()
-
-        self.env_handler.run()
-
-        # Enable RDMA, continue in errors
-        if conf.enable_rdma():
-                logger.info("RDMA capabilities are enabled in configuration")
-                try:
-                    self.setup_rdma_device()
-                except Exception as e:
-                    logger.error("Error setting up rdma device: %s" % e)
+            logger.info("RDMA capabilities are enabled in configuration")
+            try:
+                setup_rdma_device()
+            except Exception as e:
+                logger.error("Error setting up rdma device: %s" % e)
         else:
             logger.info("RDMA capabilities are not enabled, skipping")
         
         while self.running:
             self.update_handler.run_latest()
-
-
-    def setup_rdma_device(self):
-        logger.verbose("Parsing SharedConfig XML contents for RDMA details")
-        xml_doc = parse_doc(
-            fileutil.read_file(os.path.join(conf.get_lib_dir(), SHARED_CONF_FILE_NAME)))
-        if xml_doc is None:
-            logger.error("Could not parse SharedConfig XML document")
-            return
-        instance_elem = find(xml_doc, "Instance")
-        if not instance_elem:
-            logger.error("Could not find <Instance> in SharedConfig document")
-            return
-
-        rdma_ipv4_addr = getattrib(instance_elem, "rdmaIPv4Address")
-        if not rdma_ipv4_addr:
-            logger.error(
-                "Could not find rdmaIPv4Address attribute on Instance element of SharedConfig.xml document")
-            return
-
-        rdma_mac_addr = getattrib(instance_elem, "rdmaMacAddress")
-        if not rdma_mac_addr:
-            logger.error(
-                "Could not find rdmaMacAddress attribute on Instance element of SharedConfig.xml document")
-            return
-
-        # add colons to the MAC address (e.g. 00155D33FF1D ->
-        # 00:15:5D:33:FF:1D)
-        rdma_mac_addr = ':'.join([rdma_mac_addr[i:i+2]
-                                  for i in range(0, len(rdma_mac_addr), 2)])
-        logger.info("Found RDMA details. IPv4={0} MAC={1}".format(
-            rdma_ipv4_addr, rdma_mac_addr))
-
-        # Set up the RDMA device with collected informatino
-        RDMADeviceHandler(rdma_ipv4_addr, rdma_mac_addr).start()
-        logger.info("RDMA: device is set up")
diff --git a/azurelinuxagent/daemon/env.py b/azurelinuxagent/ga/env.py
similarity index 99%
rename from azurelinuxagent/daemon/env.py
rename to azurelinuxagent/ga/env.py
index 9d18026d..2d67d4bb 100644
--- a/azurelinuxagent/daemon/env.py
+++ b/azurelinuxagent/ga/env.py
@@ -19,12 +19,14 @@
 
 import os
 import socket
-import threading
 import time
-import azurelinuxagent.common.logger as logger
+import threading
+
 import azurelinuxagent.common.conf as conf
-from azurelinuxagent.common.osutil import get_osutil
+import azurelinuxagent.common.logger as logger
+
 from azurelinuxagent.common.dhcp import get_dhcp_handler
+from azurelinuxagent.common.osutil import get_osutil
 
 def get_env_handler():
     return EnvHandler()
diff --git a/azurelinuxagent/daemon/monitor.py b/azurelinuxagent/ga/monitor.py
similarity index 95%
rename from azurelinuxagent/daemon/monitor.py
rename to azurelinuxagent/ga/monitor.py
index 9e1e1eb7..0ac86d4a 100644
--- a/azurelinuxagent/daemon/monitor.py
+++ b/azurelinuxagent/ga/monitor.py
@@ -15,28 +15,28 @@
 # Requires Python 2.4+ and Openssl 1.0+
 #
 
-import os
+import datetime
 import json
+import os
+import platform
 import time
-import datetime
 import threading
-import platform
-import azurelinuxagent.common.logger as logger
+
 import azurelinuxagent.common.conf as conf
+import azurelinuxagent.common.logger as logger
+
 from azurelinuxagent.common.event import WALAEventOperation, add_event
-from azurelinuxagent.common.exception import EventError, ProtocolError, \
-    OSUtilError
+from azurelinuxagent.common.exception import EventError, ProtocolError, OSUtilError
 from azurelinuxagent.common.future import ustr
-from azurelinuxagent.common.utils.textutil import parse_doc, findall, find, \
-    getattrib
-from azurelinuxagent.common.protocol.restapi import TelemetryEventParam, \
-    TelemetryEventList, \
-    TelemetryEvent, \
-    set_properties
-from azurelinuxagent.common.version import DISTRO_NAME, DISTRO_VERSION, \
-    DISTRO_CODE_NAME, AGENT_LONG_VERSION
 from azurelinuxagent.common.osutil import get_osutil
 from azurelinuxagent.common.protocol import get_protocol_util
+from azurelinuxagent.common.protocol.restapi import TelemetryEventParam, \
+                                                    TelemetryEventList, \
+                                                    TelemetryEvent, \
+                                                    set_properties
+from azurelinuxagent.common.utils.textutil import parse_doc, findall, find, getattrib
+from azurelinuxagent.common.version import DISTRO_NAME, DISTRO_VERSION, \
+                                            DISTRO_CODE_NAME, AGENT_LONG_VERSION
 
 
 def parse_event(data_str):
diff --git a/azurelinuxagent/ga/update.py b/azurelinuxagent/ga/update.py
index 0d69f523..635f7e98 100644
--- a/azurelinuxagent/ga/update.py
+++ b/azurelinuxagent/ga/update.py
@@ -129,7 +129,7 @@ class UpdateHandler(object):
             ret = self.child_process.wait()
             if ret == None:
                 ret = 1
-            if ret != 0:
+            if ret > 0:
                 msg = u"Agent {0} launched with command '{1}' failed with code: {2}".format(
                     agent_name,
                     agent_cmd,
@@ -144,9 +144,10 @@ class UpdateHandler(object):
                 if latest_agent is not None:
                     latest_agent.mark_failure()
             else:
-                msg = u"Agent {0} launched with command '{1}' returned 0".format(
+                msg = u"Agent {0} launched with command '{1}' returned {2}".format(
                     agent_name,
-                    agent_cmd)
+                    agent_cmd,
+                    ret)
                 logger.info(msg)
                 add_event(
                     AGENT_NAME,
@@ -184,14 +185,22 @@ class UpdateHandler(object):
         """
         This is the main loop which watches for agent and extension updates.
         """
-        from azurelinuxagent.ga.exthandlers import get_exthandlers_handler
-        exthandlers_handler = get_exthandlers_handler()
 
         msg = u"Agent {0} is running as the current agent".format(
             CURRENT_AGENT)
         logger.info(msg)
         add_event(AGENT_NAME, version=CURRENT_VERSION, is_success=True, message=msg)
 
+        # Launch monitoring threads
+        from azurelinuxagent.ga.monitor import get_monitor_handler
+        get_monitor_handler().run()
+
+        from azurelinuxagent.ga.env import get_env_handler
+        get_env_handler().run()
+
+        from azurelinuxagent.ga.exthandlers import get_exthandlers_handler
+        exthandlers_handler = get_exthandlers_handler()
+
         # TODO: Add means to stop running
         try:
             while self.running:
@@ -224,6 +233,7 @@ class UpdateHandler(object):
                 is_success=False,
                 message=msg)
             sys.exit(1)
+
         sys.exit(0)
         return
 
@@ -231,12 +241,13 @@ class UpdateHandler(object):
         if self.child_process is None:
             return
         
-        if signum is signal.SIGTERM:
-            self.child_process.send_signal(signal.SIGTERM)
+        self.child_process.send_signal(signum)
 
-        if self.signal_handler is not None:
-          if not self.signal_handler in (signal.SIG_IGN, signal.SIG_DFL):
+        if not self.signal_handler in (None, signal.SIG_IGN, signal.SIG_DFL):
             self.signal_handler(signum, frame)
+        elif self.signal_handler is signal.SIG_DFL:
+            if signum == signal.SIGTERM:
+                sys.exit(0)
         return
 
     def get_latest_agent(self):


Test patch:
diff --git a/tests/daemon/test_monitor.py b/tests/ga/test_monitor.py
similarity index 95%
rename from tests/daemon/test_monitor.py
rename to tests/ga/test_monitor.py
index e037dc0c..838d037a 100644
--- a/tests/daemon/test_monitor.py
+++ b/tests/ga/test_monitor.py
@@ -17,7 +17,7 @@
 
 from tests.tools import *
 from azurelinuxagent.common.exception import *
-from azurelinuxagent.daemon.monitor import *
+from azurelinuxagent.ga.monitor import *
 
 class TestMonitor(AgentTestCase):
     def test_parse_xml_event(self):
diff --git a/tests/ga/test_update.py b/tests/ga/test_update.py
index 1a29b3fd..3c81437c 100644
--- a/tests/ga/test_update.py
+++ b/tests/ga/test_update.py
@@ -862,18 +862,38 @@ class TestUpdate(UpdateTestCase):
         self.assertEqual(1, latest_agent.error.failure_count)
         return
 
-    def _test_run(self, invocations=1, enable_updates=False):
+    def _test_run(self, invocations=1, calls=[call.run()], enable_updates=False):
         conf.get_autoupdate_enabled = Mock(return_value=enable_updates)
+
+        # Note:
+        # - Python only allows mutations of objects to which a function has
+        #   a reference. Incrementing an integer directly changes the
+        #   reference. Incrementing an item of a list changes an item to
+        #   which the code has a reference.
+        #   See http://stackoverflow.com/questions/26408941/python-nested-functions-and-variable-scope
+        iterations = [0]
+        def iterator(*args, **kwargs):
+            iterations[0] += 1
+            if iterations[0] >= invocations:
+                self.update_handler.running = False
+            return
+
+        calls = calls * invocations
         
-        mock_sleep = _IterationMock(self.update_handler, invocations=invocations)
         with patch('azurelinuxagent.ga.exthandlers.get_exthandlers_handler') as mock_handler:
-            with  patch('time.sleep', new=mock_sleep):
-                try:
-                    self.update_handler.run()
-                except:
-                    pass
-                self.assertEqual(invocations + 1, len(mock_handler.mock_calls))
-                self.assertEqual(invocations, len(mock_sleep.mock_calls))
+            with patch('azurelinuxagent.ga.monitor.get_monitor_handler') as mock_monitor:
+                with patch('azurelinuxagent.ga.env.get_env_handler') as mock_env:
+                    with patch('time.sleep', side_effect=iterator) as mock_sleep:
+                        with patch('sys.exit') as mock_exit:
+
+                            self.update_handler.run()
+
+                            self.assertEqual(1, mock_handler.call_count)
+                            self.assertEqual(mock_handler.return_value.method_calls, calls)
+                            self.assertEqual(invocations, mock_sleep.call_count)
+                            self.assertEqual(1, mock_monitor.call_count)
+                            self.assertEqual(1, mock_env.call_count)
+                            self.assertEqual(1, mock_exit.call_count)
         return
 
     def test_run(self):
@@ -886,9 +906,7 @@ class TestUpdate(UpdateTestCase):
 
     def test_run_stops_if_update_available(self):
         self.update_handler._ensure_latest_agent = Mock(return_value=True)
-        with patch('sys.exit', side_effect=Exception("System Exit")) as mock_exit:
-            self._test_run(invocations=0, enable_updates=True)
-            self.assertEqual(1, mock_exit.call_count)
+        self._test_run(invocations=0, calls=[], enable_updates=True)
         return
 
     def test_set_agents(self):
@@ -904,20 +922,6 @@ class TestUpdate(UpdateTestCase):
         return
 
 
-class _IterationMock(object):
-    def __init__(self, update_handler, invocations=1):
-        self.update_handler = update_handler
-        self.invocations = invocations
-        self.mock_calls = []
-        return
-
-    def __call__(self, *args, **kwargs):
-        self.mock_calls.append((args, kwargs))
-        if len(self.mock_calls) >= self.invocations:
-            self.update_handler.running = False
-        return
-
-
 class ProtocolMock(object):
     def __init__(self, family="TestAgent", etag=42, versions=None):
         self.family = family
diff --git a/tests/test_import.py b/tests/test_import.py
index 04124118..39a48abd 100644
--- a/tests/test_import.py
+++ b/tests/test_import.py
@@ -7,9 +7,9 @@ import azurelinuxagent.pa.deprovision as deprovision
 import azurelinuxagent.daemon as daemon
 import azurelinuxagent.daemon.resourcedisk as resourcedisk
 import azurelinuxagent.daemon.scvmm as scvmm
-import azurelinuxagent.daemon.monitor as monitor
-import azurelinuxagent.ga.update as update
 import azurelinuxagent.ga.exthandlers as exthandlers
+import azurelinuxagent.ga.monitor as monitor
+import azurelinuxagent.ga.update as update
 
 class TestImportHandler(AgentTestCase):
     def test_get_handler(self):
diff --git a/tests/tools.py b/tests/tools.py
index 2d5d0316..8bf23ed5 100644
--- a/tests/tools.py
+++ b/tests/tools.py
@@ -36,9 +36,9 @@ from azurelinuxagent.common.version import PY_VERSION_MAJOR
 
 #Import mock module for Python2 and Python3
 try:
-    from unittest.mock import Mock, patch, MagicMock, DEFAULT
+    from unittest.mock import Mock, patch, MagicMock, DEFAULT, call
 except ImportError:
-    from mock import Mock, patch, MagicMock, DEFAULT
+    from mock import Mock, patch, MagicMock, DEFAULT, call
 
 test_dir = os.path.dirname(os.path.abspath(__file__))
 data_dir = os.path.join(test_dir, "data")