Repository: Azure/WALinuxAgent
Pull request #1188 (2018-05-23 20:43:14)

Clean up signal handling and sentinel file
- in a child process, when handling a `SIGTERM` from the default handler we should call `shutdown()`
- we should not emit a sentinel on normal startup because `SIGKILL` cannot be caught, and we consider this to be a clean shutdown; instead only emit a sentinel on an unhandled error
- fix naming of sentinel everywhere
- update tests


Patch:
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index 04647721..a802e192 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -988,7 +988,10 @@ class ExtHandlerInstance(object):
         try:
             # This should be .run(), but due to the wide variety
             # of Python versions we must support we must use .communicate().
-            process = subprocess.Popen(os.path.join(base_dir, cmd),
+            # Some extensions erroneously begin cmd with a slash; don't interpret those
+            # as root-relative. (Issue #1170)
+            full_path = os.path.join(base_dir, cmd.lstrip(os.sep))
+            process = subprocess.Popen(full_path,
                                   shell=True,
                                   cwd=base_dir,
                                   stdout=subprocess.PIPE,
@@ -996,7 +999,7 @@ class ExtHandlerInstance(object):
                                   env=os.environ,
                                   preexec_fn=os.setsid)
         except OSError as e:
-            raise ExtensionError("Failed to launch '{0}': {1}".format(cmd, e.strerror))
+            raise ExtensionError("Failed to launch '{0}': {1}".format(full_path, e.strerror))
 
         msg = capture_from_process(process, cmd, timeout)
 
diff --git a/azurelinuxagent/ga/update.py b/azurelinuxagent/ga/update.py
index e0e01e4e..ec9329f4 100644
--- a/azurelinuxagent/ga/update.py
+++ b/azurelinuxagent/ga/update.py
@@ -75,7 +75,7 @@ GOAL_STATE_INTERVAL = 3
 
 ORPHAN_WAIT_INTERVAL = 15 * 60
 
-AGENT_SENTINAL_FILE = "current_version"
+AGENT_SENTINEL_FILE = "current_version"
 
 READONLY_FILE_GLOBS = [
     "*.crt",
@@ -313,7 +313,7 @@ class UpdateHandler(object):
         except Exception as e:
             msg = u"Agent {0} failed with exception: {1}".format(
                 CURRENT_AGENT, ustr(e))
-            self._set_sentinal(msg=msg)
+            self._set_sentinel(msg=msg)
             logger.warn(msg)
             logger.warn(traceback.format_exc())
             sys.exit(1)
@@ -324,12 +324,7 @@ class UpdateHandler(object):
         sys.exit(0)
 
     def forward_signal(self, signum, frame):
-        # Note:
-        #  - At present, the handler is registered only for SIGTERM.
-        #    However, clean shutdown is both SIGTERM and SIGKILL.
-        #    A SIGKILL handler is not being registered at this time to
-        #    minimize perturbing the code.
-        if signum in (signal.SIGTERM, signal.SIGKILL):
+        if signum == signal.SIGTERM:
             self._shutdown()
 
         if self.child_process is None:
@@ -340,13 +335,14 @@ class UpdateHandler(object):
             CURRENT_AGENT,
             signum,
             self.child_agent.name if self.child_agent is not None else CURRENT_AGENT)
+
         self.child_process.send_signal(signum)
 
         if self.signal_handler not 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:
-                # TODO: This should set self.running to False vs. just exiting
+                self._shutdown()
                 sys.exit(0)
         return
 
@@ -371,7 +367,7 @@ class UpdateHandler(object):
         try:
             if not self._is_clean_start:
                 msg = u"Agent did not terminate cleanly: {0}".format(
-                            fileutil.read_file(self._sentinal_file_path()))
+                            fileutil.read_file(self._sentinel_file_path()))
                 logger.info(msg)
                 add_event(
                     AGENT_NAME,
@@ -382,7 +378,6 @@ class UpdateHandler(object):
         except Exception:
             pass
 
-        self._set_sentinal(msg="Starting")
         return
 
     def _ensure_no_orphans(self, orphan_wait_interval=ORPHAN_WAIT_INTERVAL):
@@ -500,7 +495,7 @@ class UpdateHandler(object):
 
     @property
     def _is_clean_start(self):
-        return not os.path.isfile(self._sentinal_file_path())
+        return not os.path.isfile(self._sentinel_file_path())
 
     @property
     def _is_orphaned(self):
@@ -570,33 +565,33 @@ class UpdateHandler(object):
         self.agents.sort(key=lambda agent: agent.version, reverse=True)
         return
 
-    def _set_sentinal(self, agent=CURRENT_AGENT, msg="Unknown cause"):
+    def _set_sentinel(self, agent=CURRENT_AGENT, msg="Unknown cause"):
         try:
             fileutil.write_file(
-                self._sentinal_file_path(),
+                self._sentinel_file_path(),
                 "[{0}] [{1}]".format(agent, msg))
         except Exception as e:
             logger.warn(
-                u"Exception writing sentinal file {0}: {1}",
-                self._sentinal_file_path(),
+                u"Exception writing sentinel file {0}: {1}",
+                self._sentinel_file_path(),
                 str(e))
         return
 
-    def _sentinal_file_path(self):
-        return os.path.join(conf.get_lib_dir(), AGENT_SENTINAL_FILE)
+    def _sentinel_file_path(self):
+        return os.path.join(conf.get_lib_dir(), AGENT_SENTINEL_FILE)
 
     def _shutdown(self):
         self.running = False
 
-        if not os.path.isfile(self._sentinal_file_path()):
+        if not os.path.isfile(self._sentinel_file_path()):
             return
 
         try:
-            os.remove(self._sentinal_file_path())
+            os.remove(self._sentinel_file_path())
         except Exception as e:
             logger.warn(
-                u"Exception removing sentinal file {0}: {1}",
-                self._sentinal_file_path(),
+                u"Exception removing sentinel file {0}: {1}",
+                self._sentinel_file_path(),
                 str(e))
         return
 


Test patch:
diff --git a/tests/ga/test_update.py b/tests/ga/test_update.py
index 81d85f11..0a267965 100644
--- a/tests/ga/test_update.py
+++ b/tests/ga/test_update.py
@@ -669,15 +669,10 @@ class TestUpdate(UpdateTestCase):
 
         self.assertEqual(None, self.update_handler.signal_handler)
 
-    def test_emit_restart_event_writes_sentinal_file(self):
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
-        self.update_handler._emit_restart_event()
-        self.assertTrue(os.path.isfile(self.update_handler._sentinal_file_path()))
-
     def test_emit_restart_event_emits_event_if_not_clean_start(self):
         try:
             mock_event = self.event_patch.start()
-            self.update_handler._set_sentinal()
+            self.update_handler._set_sentinel()
             self.update_handler._emit_restart_event()
             self.assertEqual(1, mock_event.call_count)
         except Exception as e:
@@ -941,16 +936,16 @@ class TestUpdate(UpdateTestCase):
         for p in pid_files:
             self.assertTrue(pid_re.match(os.path.basename(p)))
 
-    def test_is_clean_start_returns_true_when_no_sentinal(self):
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
+    def test_is_clean_start_returns_true_when_no_sentinel(self):
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
         self.assertTrue(self.update_handler._is_clean_start)
 
-    def test_is_clean_start_returns_false_when_sentinal_exists(self):
-        self.update_handler._set_sentinal(agent=CURRENT_AGENT)
+    def test_is_clean_start_returns_false_when_sentinel_exists(self):
+        self.update_handler._set_sentinel(agent=CURRENT_AGENT)
         self.assertFalse(self.update_handler._is_clean_start)
 
     def test_is_clean_start_returns_false_for_exceptions(self):
-        self.update_handler._set_sentinal()
+        self.update_handler._set_sentinel()
         with patch("azurelinuxagent.common.utils.fileutil.read_file", side_effect=Exception):
             self.assertFalse(self.update_handler._is_clean_start)
 
@@ -1248,14 +1243,14 @@ class TestUpdate(UpdateTestCase):
         with patch('os.getppid', return_value=1):
             self._test_run(invocations=0, calls=[], enable_updates=True)
 
-    def test_run_clears_sentinal_on_successful_exit(self):
+    def test_run_clears_sentinel_on_successful_exit(self):
         self._test_run()
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
 
-    def test_run_leaves_sentinal_on_unsuccessful_exit(self):
+    def test_run_leaves_sentinel_on_unsuccessful_exit(self):
         self.update_handler._upgrade_available = Mock(side_effect=Exception)
         self._test_run(invocations=0, calls=[], enable_updates=True)
-        self.assertTrue(os.path.isfile(self.update_handler._sentinal_file_path()))
+        self.assertTrue(os.path.isfile(self.update_handler._sentinel_file_path()))
 
     def test_run_emits_restart_event(self):
         self.update_handler._emit_restart_event = Mock()
@@ -1279,31 +1274,31 @@ class TestUpdate(UpdateTestCase):
             self.assertTrue(v > a.version)
             v = a.version
 
-    def test_set_sentinal(self):
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
-        self.update_handler._set_sentinal()
-        self.assertTrue(os.path.isfile(self.update_handler._sentinal_file_path()))
+    def test_set_sentinel(self):
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
+        self.update_handler._set_sentinel()
+        self.assertTrue(os.path.isfile(self.update_handler._sentinel_file_path()))
 
-    def test_set_sentinal_writes_current_agent(self):
-        self.update_handler._set_sentinal()
+    def test_set_sentinel_writes_current_agent(self):
+        self.update_handler._set_sentinel()
         self.assertTrue(
-            fileutil.read_file(self.update_handler._sentinal_file_path()),
+            fileutil.read_file(self.update_handler._sentinel_file_path()),
             CURRENT_AGENT)
 
     def test_shutdown(self):
-        self.update_handler._set_sentinal()
+        self.update_handler._set_sentinel()
         self.update_handler._shutdown()
         self.assertFalse(self.update_handler.running)
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
 
-    def test_shutdown_ignores_missing_sentinal_file(self):
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
+    def test_shutdown_ignores_missing_sentinel_file(self):
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
         self.update_handler._shutdown()
         self.assertFalse(self.update_handler.running)
-        self.assertFalse(os.path.isfile(self.update_handler._sentinal_file_path()))
+        self.assertFalse(os.path.isfile(self.update_handler._sentinel_file_path()))
 
     def test_shutdown_ignores_exceptions(self):
-        self.update_handler._set_sentinal()
+        self.update_handler._set_sentinel()
 
         try:
             with patch("os.remove", side_effect=Exception):