Repository: Azure/WALinuxAgent
Pull request #310 (2016-07-08 23:15:35)

Prevent cycling agent
This change blacklists an agent the restarts too often in a five minute window.
Signed-off-by: Brendan Dixon <brendand@microsoft.com>

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/utils/flexible_version.py b/azurelinuxagent/common/utils/flexible_version.py
index 047acc45..2fce88dc 100644
--- a/azurelinuxagent/common/utils/flexible_version.py
+++ b/azurelinuxagent/common/utils/flexible_version.py
@@ -133,6 +133,9 @@ class FlexibleVersion(version.Version):
 
         return this_index < that_index
 
+    def __ne__(self, that):
+        return not self.__eq__(that)
+
     def __eq__(self, that):
         this_version, that_version = self._ensure_compatible(that)
 
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index 8ed2f65f..1e6ca438 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -514,7 +514,7 @@ class ExtHandlerInstance(object):
         self.set_operation(WALAEventOperation.Enable)
 
         man = self.load_manifest()
-        self.launch_command(man.get_enable_command(), timeout=300)
+        self.launch_command(man.get_enable_command())
         self.set_handler_state(ExtHandlerState.Enabled)
         self.set_handler_status(status="Ready", message="Plugin enabled")
 
@@ -667,17 +667,15 @@ class ExtHandlerInstance(object):
         base_dir = self.get_base_dir()
         try:
             devnull = open(os.devnull, 'w')
-            child = subprocess.Popen(base_dir + "/" + cmd,
-                                     shell=True,
-                                     cwd=base_dir,
-                                     stdout=devnull)
+            child = subprocess.Popen(base_dir + "/" + cmd, shell=True,
+                                     cwd=base_dir, stdout=devnull)
         except Exception as e:
             #TODO do not catch all exception
             raise ExtensionError("Failed to launch: {0}, {1}".format(cmd, e))
 
-        retry = timeout
-        while retry > 0 and child.poll() is None:
-            time.sleep(1)
+        retry = timeout / 5
+        while retry > 0 and child.poll == None:
+            time.sleep(5)
             retry -= 1
         if retry == 0:
             os.kill(child.pid, 9)
diff --git a/azurelinuxagent/ga/update.py b/azurelinuxagent/ga/update.py
index 1c7d13a9..ba4859f5 100644
--- a/azurelinuxagent/ga/update.py
+++ b/azurelinuxagent/ga/update.py
@@ -52,6 +52,9 @@ from azurelinuxagent.ga.exthandlers import HandlerManifest
 AGENT_ERROR_FILE = "error.json" # File name for agent error record
 AGENT_MANIFEST_FILE = "HandlerManifest.json"
 
+CHILD_LAUNCH_INTERVAL = 5 * 60
+CHILD_LAUNCH_RESTART_MAX = 3
+
 MAX_FAILURE = 3 # Max failure allowed for agent before blacklisted
 
 GOAL_STATE_INTERVAL = 25
@@ -80,6 +83,9 @@ class UpdateHandler(object):
 
         self.agents = []
 
+        self.child_agent = None
+        self.child_launch_time = None
+        self.child_launch_attempts = 0
         self.child_process = None
         self.signal_handler = None
         return
@@ -117,6 +123,8 @@ class UpdateHandler(object):
                 cmds[0] = get_python_cmd()
                 agent_cmd = " ".join(cmds)
 
+            self._evaluate_agent_health(latest_agent)
+
             self.child_process = subprocess.Popen(
                 cmds,
                 cwd=agent_dir,
@@ -336,6 +344,33 @@ class UpdateHandler(object):
         # Return True if agents more recent than the current are available
         return len(self.agents) > 0 and self.agents[0].version > base_version
 
+    def _evaluate_agent_health(self, latest_agent):
+        """
+        Evaluate the health of the selected agent: If it is restarting
+        too frequently, raise an Exception to force blacklisting.
+        """
+        if latest_agent is None:
+            return
+
+        if self.child_agent is None or latest_agent.version != self.child_agent.version:
+            self.child_agent = latest_agent
+            self.child_launch_time = None
+            self.child_launch_attempts = 0
+
+        if self.child_launch_time is None:
+            self.child_launch_time = time.time()
+
+        self.child_launch_attempts += 1
+
+        if (time.time() - self.child_launch_time) <= CHILD_LAUNCH_INTERVAL \
+            and self.child_launch_attempts >= CHILD_LAUNCH_RESTART_MAX:
+                msg = u"Agent {0} restarted more than {1} times in {2} seconds".format(
+                    self.child_agent.name,
+                    CHILD_LAUNCH_RESTART_MAX,
+                    CHILD_LAUNCH_INTERVAL)
+                raise Exception(msg)
+        return
+
     def _filter_blacklisted_agents(self):
         self.agents = [agent for agent in self.agents if not agent.is_blacklisted]
         return


Test patch:
diff --git a/tests/data/ext/sample_ext/sample.py b/tests/data/ext/sample_ext/sample.py
index 74bd839f..7107ac28 100755
--- a/tests/data/ext/sample_ext/sample.py
+++ b/tests/data/ext/sample_ext/sample.py
@@ -2,19 +2,18 @@
 
 import os
 
-
 def get_seq():
-    latest_seq = -1
+    latest_seq = -1;
     config_dir = os.path.join(os.getcwd(), "config")
     if os.path.isdir(config_dir):
         for item in os.listdir(config_dir):
             item_path = os.path.join(config_dir, item)
             if os.path.isfile(item_path):
-                separator = item.rfind(".")
-                if separator > 0 and item[separator + 1:] == "settings":
-                    sequence = int(item[0: separator])
-                    if sequence > latest_seq:
-                        latest_seq = sequence
+                seperator = item.rfind(".")
+                if seperator > 0 and item[seperator + 1:] == "settings":
+                    seq = int(item[0: seperator])
+                    if seq > latest_seq:
+                        latest_seq = seq
     return latest_seq
 
 
@@ -29,9 +28,6 @@ succeed_status = """
 if __name__ == "__main__":
     seq = get_seq()
     if seq >= 0:
-        status_path = os.path.join(os.getcwd(), "status")
-        if not os.path.exists(status_path):
-            os.makedirs(status_path)
-        status_file = os.path.join(status_path, "{0}.status".format(seq))
+        status_file = os.path.join(os.getcwd(), "status", "{0}.status".format(seq))
         with open(status_file, "w+") as status:
             status.write(succeed_status)
diff --git a/tests/ga/test_extension.py b/tests/ga/test_extension.py
index 71c219c5..1f748257 100644
--- a/tests/ga/test_extension.py
+++ b/tests/ga/test_extension.py
@@ -21,6 +21,7 @@ from azurelinuxagent.common.protocol import get_protocol_util
 from azurelinuxagent.ga.exthandlers import *
 from azurelinuxagent.common.protocol.wire import WireProtocol
 
+@patch("time.sleep")
 @patch("azurelinuxagent.common.protocol.wire.CryptUtil")
 @patch("azurelinuxagent.common.utils.restutil.http_get")
 class TestExtension(AgentTestCase):
@@ -46,14 +47,14 @@ class TestExtension(AgentTestCase):
         self.assertEquals(0, len(vm_status.vmAgent.extensionHandlers))
         return
 
-    def _create_mock(self, test_data, mock_http_get, MockCryptUtil):
-        """Test enable/disable/uninstall of an extension"""
+    def _create_mock(self, test_data, mock_http_get, MockCryptUtil, _):
+        """Test enable/disable/unistall of an extension"""
         handler = get_exthandlers_handler()
 
         #Mock protocol to return test data
         mock_http_get.side_effect = test_data.mock_http_get
         MockCryptUtil.side_effect = test_data.mock_crypt_util
-
+ 
         protocol = WireProtocol("foo.bar")
         protocol.detect()
         protocol.report_ext_status = MagicMock()
diff --git a/tests/ga/test_update.py b/tests/ga/test_update.py
index cfa537e7..36241b16 100644
--- a/tests/ga/test_update.py
+++ b/tests/ga/test_update.py
@@ -659,6 +659,51 @@ class TestUpdate(UpdateTestCase):
             v = a.version
         return
 
+    def _test_evaluate_agent_health(self, child_agent_index=0):
+        self.prepare_agents()
+
+        latest_agent = self.update_handler.get_latest_agent()
+        self.assertTrue(latest_agent.is_available)
+        self.assertFalse(latest_agent.is_blacklisted)
+        self.assertTrue(len(self.update_handler.agents) > 1)
+
+        child_agent = self.update_handler.agents[child_agent_index]
+        self.assertTrue(child_agent.is_available)
+        self.assertFalse(child_agent.is_blacklisted)
+        self.update_handler.child_agent = child_agent
+
+        self.update_handler._evaluate_agent_health(latest_agent)
+        return
+
+    def test_evaluate_agent_health_ignores_installed_agent(self):
+        self.update_handler._evaluate_agent_health(None)
+        return
+
+    def test_evaluate_agent_health_raises_exception_for_restarting_agent(self):
+        self.update_handler.child_launch_time = time.time() - (4 * 60)
+        self.update_handler.child_launch_attempts = CHILD_LAUNCH_RESTART_MAX - 1
+        self.assertRaises(Exception, self._test_evaluate_agent_health)
+        return
+
+    def test_evaluate_agent_health_will_not_raise_exception_for_long_restarts(self):
+        self.update_handler.child_launch_time = time.time() - 24 * 60
+        self.update_handler.child_launch_attempts = CHILD_LAUNCH_RESTART_MAX
+        self._test_evaluate_agent_health()
+        return
+
+    def test_evaluate_agent_health_will_not_raise_exception_too_few_restarts(self):
+        self.update_handler.child_launch_time = time.time()
+        self.update_handler.child_launch_attempts = CHILD_LAUNCH_RESTART_MAX - 2
+        self._test_evaluate_agent_health()
+        return
+
+    def test_evaluate_agent_health_resets_with_new_agent(self):
+        self.update_handler.child_launch_time = time.time() - (4 * 60)
+        self.update_handler.child_launch_attempts = CHILD_LAUNCH_RESTART_MAX - 1
+        self._test_evaluate_agent_health(child_agent_index=1)
+        self.assertEqual(1, self.update_handler.child_launch_attempts)
+        return
+
     def test_filter_blacklisted_agents(self):
         self.prepare_agents()
 
@@ -763,12 +808,12 @@ class TestUpdate(UpdateTestCase):
             self.assertTrue(os.path.exists(agent_path + ".zip"))
         return
 
-    def _test_run_latest(self, return_value=0, side_effect=None):
+    def _test_run_latest(self, return_value=0, side_effect=None, child_calls=1):
         mock_child = Mock()
         mock_child.wait = Mock(return_value=return_value, side_effect=side_effect)
         with patch('subprocess.Popen', return_value=mock_child) as mock_popen:
             self.update_handler.run_latest()
-            self.assertEqual(1, len(mock_popen.mock_calls))
+            self.assertEqual(child_calls, mock_popen.call_count)
 
             return mock_popen.call_args
 
diff --git a/tests/utils/test_flexible_version.py b/tests/utils/test_flexible_version.py
index 23d1d418..11620226 100644
--- a/tests/utils/test_flexible_version.py
+++ b/tests/utils/test_flexible_version.py
@@ -467,6 +467,9 @@ class TestFlexibleVersion(unittest.TestCase):
         self.assertTrue(FlexibleVersion("1.0") == FlexibleVersion("1.0"))
         self.assertTrue(FlexibleVersion("1.0") >= FlexibleVersion("1.0"))
         self.assertTrue(FlexibleVersion("1.0") <= FlexibleVersion("1.0"))
+
+        self.assertFalse(FlexibleVersion("1.0") != FlexibleVersion("1.0"))
+        self.assertTrue(FlexibleVersion("1.1") != FlexibleVersion("1.0"))
         return