Repository: Azure/WALinuxAgent
Pull request #1194 (2018-05-29 21:58:58)

prefer HostGA for status updates

boumenot: > CI is expected to fail.  I am checking the approach before fixing all of the unit test breakage.

@hglkrijger - does this match the approach we discussed?

Patch:
diff --git a/azurelinuxagent/common/protocol/wire.py b/azurelinuxagent/common/protocol/wire.py
index 265b1f6f..5e5604a3 100644
--- a/azurelinuxagent/common/protocol/wire.py
+++ b/azurelinuxagent/common/protocol/wire.py
@@ -38,6 +38,7 @@ from azurelinuxagent.common.utils.archive import StateFlusher
 from azurelinuxagent.common.utils.cryptutil import CryptUtil
 from azurelinuxagent.common.utils.textutil import parse_doc, findall, find, \
     findtext, getattrib, gettext, remove_bom, get_bytes_from_pem, parse_json
+from azurelinuxagent.common.version import AGENT_NAME
 
 VERSION_INFO_URI = "http://{0}/?comp=versions"
 GOAL_STATE_URI = "http://{0}/machine/?comp=goalstate"
@@ -952,25 +953,44 @@ class WireClient(object):
                             "Exception creating status blob: {0}", ustr(e))
                         return
 
-                    if not HostPluginProtocol.is_default_channel():
-                        try:
-                            if self.status_blob.upload(blob_uri):
-                                return
-                        except HttpError as e:
-                            pass
+                    # Swap the order of use for the HostPlugin vs. the "direct" route.
+                    # Prefer the use of HostPlugin. If HostPlugin fails fall back to the
+                    # direct route.
+                    #
+                    # The code previously preferred the "direct" route always, and only fell back
+                    # to the HostPlugin *if* there was an error.  We would like to move to
+                    # the HostPlugin for all traffic, but this is a big change.  We would like
+                    # to see how this behaves at scale, and have a fallback should things go
+                    # wrong.  This is why we try HostPlugin then direct.
+                    try:
+                        host = self.get_host_plugin()
+                        host.put_vm_status(self.status_blob,
+                                           ext_conf.status_upload_blob,
+                                           ext_conf.status_upload_blob_type)
+                        return
+                    except HttpError:
+                        pass
+                    except Exception as e:
+                        if not isinstance(e, ResourceGoneError):
+                            self.report_status_event("Exception uploading status blob (HostPlugin): {0}", ustr(e))
 
-                    host = self.get_host_plugin()
-                    host.put_vm_status(self.status_blob,
-                                    ext_conf.status_upload_blob,
-                                    ext_conf.status_upload_blob_type)
-                    HostPluginProtocol.set_default_channel(True)
-                    return
+                    try:
+                        if self.status_blob.upload(blob_uri):
+                            from azurelinuxagent.common.event import add_event, WALAEventOperation
+                            add_event(
+                                name=AGENT_NAME,
+                                version=CURRENT_VERSION,
+                                op=WALAEventOperation.ReportStatus,
+                                is_success=True,
+                                message="direct",
+                                log_event=False)
+                            return
+                    except HttpError:
+                        pass
 
+                    return
             except Exception as e:
-                # If the HostPlugin rejects the request,
-                # let the error continue, but set to use the HostPlugin
                 if isinstance(e, ResourceGoneError):
-                    HostPluginProtocol.set_default_channel(True)
                     continue
 
                 self.report_status_event(


Test patch:
diff --git a/tests/common/test_event.py b/tests/common/test_event.py
index 3917b37c..1acd4bcc 100644
--- a/tests/common/test_event.py
+++ b/tests/common/test_event.py
@@ -26,6 +26,8 @@ from azurelinuxagent.common.version import CURRENT_VERSION
 
 from tests.tools import *
 
+import azurelinuxagent.common.event as event
+
 
 class TestEvent(AgentTestCase):
     def test_event_status_event_marked(self):
diff --git a/tests/protocol/test_hostplugin.py b/tests/protocol/test_hostplugin.py
index 5eed8522..32729d54 100644
--- a/tests/protocol/test_hostplugin.py
+++ b/tests/protocol/test_hostplugin.py
@@ -114,8 +114,9 @@ class TestHostPlugin(AgentTestCase):
 
     def test_fallback(self):
         """
-        Validate fallback to upload status using HostGAPlugin is happening when
-        status reporting via default method is unsuccessful
+        Status now defaults to HostPlugin. Validate that any errors on the public
+        channel are ignored.  Validate that the default channel is never changed
+        as part of status upload.
         """
         test_goal_state = wire.GoalState(WireProtocolData(DATA_FILE).goal_state)
         status = restapi.VMStatus(status="Ready", message="Guest Agent is running")
@@ -132,16 +133,16 @@ class TestHostPlugin(AgentTestCase):
                     wire_protocol_client.ext_conf.status_upload_blob_type = page_blob_type
                     wire_protocol_client.status_blob.set_vm_status(status)
                     wire_protocol_client.upload_status_blob()
-                    self.assertEqual(patch_upload.call_count, 1)
+                    self.assertEqual(patch_upload.call_count, 0)
                     self.assertTrue(patch_put.call_count == 1,
                                     "Fallback was not engaged")
                     self.assertTrue(patch_put.call_args[0][0] == sas_url)
-                    self.assertTrue(wire.HostPluginProtocol.is_default_channel())
-                    wire.HostPluginProtocol.set_default_channel(False)
+                    self.assertFalse(wire.HostPluginProtocol.is_default_channel())
 
     def test_fallback_failure(self):
         """
-        Validate that when host plugin fails, the default channel is reset
+        Validate that if host plugin fails and the direct connection fails the
+        default channel remains unchanged.
         """
         test_goal_state = wire.GoalState(WireProtocolData(DATA_FILE).goal_state)
         status = restapi.VMStatus(status="Ready",
@@ -152,10 +153,11 @@ class TestHostPlugin(AgentTestCase):
                           return_value=True):
             with patch.object(wire.StatusBlob,
                               "upload",
-                              return_value=False):
+                              return_value=False) as patch_upload:
                 with patch.object(wire.HostPluginProtocol,
                                   "_put_page_blob_status",
                                   side_effect=wire.HttpError("put failure")) as patch_put:
+                    self.assertFalse(wire.HostPluginProtocol.is_default_channel())
                     client = wire.WireProtocol(wireserver_url).client
                     client.get_goal_state = Mock(return_value=test_goal_state)
                     client.ext_conf = wire.ExtensionsConfig(None)
@@ -163,8 +165,8 @@ class TestHostPlugin(AgentTestCase):
                     client.ext_conf.status_upload_blob_type = page_blob_type
                     client.status_blob.set_vm_status(status)
                     client.upload_status_blob()
-                    self.assertTrue(patch_put.call_count == 1,
-                                    "Fallback was not engaged")
+                    self.assertEqual(1, patch_put.call_count)
+                    self.assertEqual(1, patch_upload.call_count)
                     self.assertFalse(wire.HostPluginProtocol.is_default_channel())
 
     def test_put_status_error_reporting(self):
@@ -192,9 +194,16 @@ class TestHostPlugin(AgentTestCase):
                     with patch.object(wire.HostPluginProtocol,
                                       "ensure_initialized", return_value=True):
                         wire_protocol_client.upload_status_blob()
-                        self.assertFalse(wire.HostPluginProtocol.is_default_channel())
-                        self.assertTrue(patch_add_event.call_count == 1)
 
+                        # The agent tries to upload via HostPlugin and that fails due to
+                        # http_put having a side effect of "put_error"
+                        #
+                        # The agent tries to upload using a direct connection, and that succeeds.
+                        self.assertEqual(1, wire_protocol_client.status_blob.upload.call_count)
+                        # The agent never touches the default protocol is this code path, so no change.
+                        self.assertFalse(wire.HostPluginProtocol.is_default_channel())
+                        # The agent never logs a telemetry event for a bad HTTP call
+                        self.assertEqual(patch_add_event.call_count, 0)
 
     def test_validate_http_request(self):
         """Validate correct set of data is sent to HostGAPlugin when reporting VM status"""
diff --git a/tests/protocol/test_wire.py b/tests/protocol/test_wire.py
index 99dc5596..4a69d105 100644
--- a/tests/protocol/test_wire.py
+++ b/tests/protocol/test_wire.py
@@ -180,6 +180,9 @@ class TestWireProtocol(AgentTestCase):
                                      host_uri)
 
     def test_upload_status_blob_default(self, *args):
+        """
+        Default status blob method is HostPlugin.
+        """
         vmstatus = VMStatus(message="Ready", status="Ready")
         wire_protocol_client = WireProtocol(wireserver_url).client
         wire_protocol_client.ext_conf = ExtensionsConfig(None)
@@ -193,9 +196,12 @@ class TestWireProtocol(AgentTestCase):
                     HostPluginProtocol.set_default_channel(False)
                     wire_protocol_client.upload_status_blob()
 
-                    patch_default_upload.assert_called_once_with(testurl)
-                    patch_get_goal_state.assert_not_called()
-                    patch_host_ga_plugin_upload.assert_not_called()
+                    # do not call the direct method unless host plugin fails
+                    patch_default_upload.assert_not_called()
+                    # host plugin always fetches a goal state
+                    patch_get_goal_state.assert_called_once_with()
+                    # host plugin uploads the status blob
+                    patch_host_ga_plugin_upload.assert_called_once_with(ANY, testurl, 'BlockBlob')
 
     def test_upload_status_blob_host_ga_plugin(self, *args):
         vmstatus = VMStatus(message="Ready", status="Ready")
@@ -217,11 +223,10 @@ class TestWireProtocol(AgentTestCase):
                     HostPluginProtocol.set_default_channel(False)
                     wire_protocol_client.get_goal_state = Mock(return_value=goal_state)
                     wire_protocol_client.upload_status_blob()
-                    patch_default_upload.assert_called_once_with(testurl)
+                    patch_default_upload.assert_not_called()
                     self.assertEqual(1, wire_protocol_client.get_goal_state.call_count)
                     patch_http.assert_called_once_with(testurl, wire_protocol_client.status_blob)
-                    self.assertTrue(HostPluginProtocol.is_default_channel())
-                    HostPluginProtocol.set_default_channel(False)
+                    self.assertFalse(HostPluginProtocol.is_default_channel())
 
     def test_upload_status_blob_unknown_type_assumes_block(self, *args):
         vmstatus = VMStatus(message="Ready", status="Ready")
@@ -239,7 +244,7 @@ class TestWireProtocol(AgentTestCase):
 
                     patch_prepare.assert_called_once_with("BlockBlob")
                     patch_default_upload.assert_called_once_with(testurl)
-                    patch_get_goal_state.assert_not_called()
+                    patch_get_goal_state.assert_called_once_with()
 
     def test_upload_status_blob_reports_prepare_error(self, *args):
         vmstatus = VMStatus(message="Ready", status="Ready")
diff --git a/tests/tools.py b/tests/tools.py
index a41af3c3..4226c1f9 100644
--- a/tests/tools.py
+++ b/tests/tools.py
@@ -28,7 +28,7 @@ from functools import wraps
 
 import time
 
-import azurelinuxagent.common.event as event
+import azurelinuxagent.common.event
 import azurelinuxagent.common.conf as conf
 import azurelinuxagent.common.logger as logger
 from azurelinuxagent.common.utils import fileutil
@@ -80,8 +80,8 @@ class AgentTestCase(unittest.TestCase):
 
         conf.get_agent_pid_file_path = Mock(return_value=os.path.join(self.tmp_dir, "waagent.pid"))
 
-        event.init_event_status(self.tmp_dir)
-        event.init_event_logger(self.tmp_dir)
+        azurelinuxagent.common.event.init_event_status(self.tmp_dir)
+        azurelinuxagent.common.event.init_event_logger(self.tmp_dir)
 
     def tearDown(self):
         if not debug and self.tmp_dir is not None: