Implement FastTrack handshake with CRP #2514

narrieta · 2022-02-17T18:29:06Z

If the HostGAPlugin supports the VM settings API, the agent needs to add FastTrack to the supportedFeatures in the status report. When CRP detects this, it enables the feature and starts producing FastTrack goal states for that VM.

The PR includes some test fixes; I added comments in the PR for those.

narrieta · 2022-02-17T18:31:10Z

azurelinuxagent/common/protocol/goal_state.py

@@ -219,6 +219,7 @@ def save_to_history(etag, text):
                pass
            except VmSettingsError as exception:
                save_to_history(exception.etag, exception.vm_settings_text)
+                raise


this error needs to be propagated to the caller, I missed that in a previous commit

narrieta · 2022-02-17T18:32:14Z

azurelinuxagent/common/protocol/hostplugin.py

@@ -480,8 +480,9 @@ def format_message(msg):
                logger.info(message)
                add_event(op=WALAEventOperation.HostPlugin, message=message, is_success=True)

-            # Don't support HostGAPlugin versions older than 115
-            if vm_settings.host_ga_plugin_version < FlexibleVersion("1.0.8.115"):
+            # Don't support HostGAPlugin versions older than 123


123 contains all the fixes we need on the HostGAPlugin before releasing to Prod. Many VMs in the canary region are still on 117 so using that for now

codecov · 2022-02-17T18:33:21Z

Codecov Report

Merging #2514 (2a367ed) into develop (231b22f) will increase coverage by 0.11%.
The diff coverage is 91.89%.

@@             Coverage Diff             @@
##           develop    #2514      +/-   ##
===========================================
+ Coverage    71.77%   71.88%   +0.11%     
===========================================
  Files          101      101              
  Lines        15116    15147      +31     
  Branches      2398     2401       +3     
===========================================
+ Hits         10850    10889      +39     
- Misses        3780     3781       +1     
+ Partials       486      477       -9

Impacted Files	Coverage Δ
azurelinuxagent/common/protocol/hostplugin.py	`88.71% <0.00%> (ø)`
...inuxagent/common/protocol/extensions_goal_state.py	`73.07% <80.00%> (+0.73%)`	⬆️
azurelinuxagent/common/agent_supported_feature.py	`100.00% <100.00%> (ø)`
...ol/extensions_goal_state_from_extensions_config.py	`90.97% <100.00%> (-1.00%)`	⬇️
...protocol/extensions_goal_state_from_vm_settings.py	`80.69% <100.00%> (+0.61%)`	⬆️
azurelinuxagent/common/protocol/goal_state.py	`95.35% <100.00%> (+0.01%)`	⬆️
azurelinuxagent/common/protocol/restapi.py	`97.50% <100.00%> (+0.07%)`	⬆️
azurelinuxagent/common/protocol/wire.py	`78.82% <100.00%> (+0.05%)`	⬆️
azurelinuxagent/ga/exthandlers.py	`85.91% <100.00%> (+0.08%)`	⬆️
azurelinuxagent/ga/update.py	`87.69% <100.00%> (-0.05%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 231b22f...2a367ed. Read the comment docs.

narrieta · 2022-02-17T18:33:52Z

tests/data/wire/ext_conf_mock_status_blob.xml

@@ -1,26 +0,0 @@
-<Extensions version="1.0.0.0" goalStateIncarnation="9"><GuestAgentExtension xmlns:i="http://www.w3.org/2001/XMLSchema-instance">


see comments in tests/protocol/mockwiredata.py

narrieta · 2022-02-17T18:34:12Z

tests/ga/extension_emulator.py

@@ -111,9 +113,11 @@ def generate_put_handler(*emulators):
    def mock_put_handler(url, *args, **_):

        if HttpRequestPredicates.is_host_plugin_status_request(url):
-            return
+            status_blob = WireProtocolData.get_status_blob_from_hostgaplugin_put_status_request(args[0])


see comments in tests/protocol/mockwiredata.py

narrieta · 2022-02-17T18:35:51Z

tests/ga/test_report_status.py

+from tests.protocol import mockwiredata
+from tests.protocol.HttpRequestPredicates import HttpRequestPredicates
+
+


I moved ReportStatusTestCase to its own file.

test_report_status_should_log_errors_only_once_per_goal_state is basically the same as in the original file. The other tests are new or full rewrites of the older tests.

narrieta · 2022-02-17T18:37:41Z

tests/ga/test_report_status.py

+
+                            yield update_handler
+
+    def test_update_handler_should_report_status_when_fetch_goal_state_fails(self):


full rewrite of older tests

narrieta · 2022-02-17T18:39:37Z

tests/ga/test_update.py

-            raise HttpError()
-
-    @patch("azurelinuxagent.common.conf.get_enable_fast_track", return_value=True)
-    def test_update_handler_should_report_status_even_on_failed_goal_state_fetch(self, _):


This test and the one below have been problematic for several of the code iterations I've been working on.

I just went ahead and rewrote them as test_update_handler_should_report_status_when_fetch_goal_state_fails

narrieta · 2022-02-17T18:39:57Z

tests/ga/test_update.py

@@ -2868,171 +2868,6 @@ def test_it_should_process_goal_state_only_on_new_goal_state(self):
            self.assertEqual(2, remote_access_handler.run.call_count, "remote_access_handler.run() should have been called on a new goal state")


-class ReportStatusTestCase(AgentTestCase):


This class was moved to its own file

narrieta · 2022-02-17T18:43:10Z

tests/protocol/mockwiredata.py

@@ -305,17 +302,18 @@ def mock_http_put(self, url, data, **_):

        if url.endswith('/vmAgentLog'):
            self.call_counts['/vmAgentLog'] += 1
-        elif url.endswith('/StatusBlob'):


"/StatusBlob" is a fake url that was used in the test file that I removed. It's usage was making the put status on the hostgaplugin fail, and forcing the request to be retried as a direct call to storage.

I removed this and added support in the mock protocol for direct and hostgaplugin PUT status requests

narrieta · 2022-02-17T18:44:24Z

tests/protocol/mockwiredata.py

@@ -327,6 +325,12 @@ def mock_gen_trans_cert(self, trans_prv_file, trans_cert_file):
        with open(trans_cert_file, 'w+') as cert_file:
            cert_file.write(self.trans_cert)

+    @staticmethod


when the status is PUT via the hostgaplugin, it is wrapped in an enclosing object as property "content" and base64-encoded.

this function unwraps it and decodes it

narrieta · 2022-02-17T18:45:51Z

tests/protocol/test_extensions_goal_state.py

@@ -1,27 +0,0 @@
-# Copyright (c) Microsoft Corporation. All rights reserved.


These 2 tests were moved to test_extensions_goal_state_from_extensions_config/from_vm_settings.py, along with the other parse tests. They are now named test_it_should_default_to_block_blob_when_the_status_blob_type_is_not_valid

narrieta · 2022-02-17T18:47:55Z

tests/protocol/test_extensions_goal_state_from_vm_settings.py

-
-    @patch("azurelinuxagent.common.conf.get_enable_fast_track", return_value=True)
-    def test_extension_goal_state_should_parse_requested_version_properly(self, _):
+    def test_it_should_parse_vm_settings(self, _):


I modified these tests to go thru update_goal_state instead of create_from_* to make them more consistend with other goal state tests

narrieta · 2022-02-17T18:49:01Z

tests/ga/test_report_status.py

+                    remove_timestamps(second_status)
+
+                    self.assertEqual(first_status, second_status)
+


existing test moved from the previous file; almost identical to the original

narrieta · 2022-02-17T18:49:19Z

tests/ga/test_report_status.py

+                        update_handler._try_update_goal_state(exthandlers_handler.protocol)
+                        update_handler._report_status(exthandlers_handler)
+                        self.assertEqual(2, logger_warn.call_count, "UpdateHandler._report_status() should continue reporting errors after a new goal state")
+


The rest are new tests

kevinclark19a · 2022-02-17T21:31:20Z

tests/protocol/test_extensions_goal_state_from_extensions_config.py

+        data_file["ext_conf"] = "hostgaplugin/ext_conf-invalid_blob_type.xml"
+        with mock_wire_protocol(data_file) as protocol:
+            extensions_goal_state = protocol.get_goal_state().extensions_goal_state
+            self.assertEqual("BlockBlob", extensions_goal_state.status_upload_blob_type, 'Expected BlockBob for an invalid statusBlobType')


nit typo: BlockBob -> BlockBlob in the error message

thanks, fixed

kevinclark19a · 2022-02-17T21:32:26Z

tests/protocol/test_extensions_goal_state_from_extensions_config.py

@@ -45,3 +53,8 @@ def test_it_should_parse_empty_depends_on_as_dependency_level_0(self):

            self.assertEqual(0, extensions[0].settings[0].dependencyLevel, "Incorrect dependencyLevel}")


another typo, if you want to fix it in this PR.

what's the typo? thanks

I see it. fixed

kevinclark19a · 2022-02-17T21:36:50Z

tests/protocol/test_extensions_goal_state_from_vm_settings.py

+            self.assertEqual(2, len(extensions_goal_state.agent_manifests), "Incorrect number of agent manifests. Got: {0}".format(extensions_goal_state.agent_manifests))
+            self.assertEqual("Prod", extensions_goal_state.agent_manifests[0].family, "Incorrect agent family.")
+            self.assertEqual(2, len(extensions_goal_state.agent_manifests[0].uris), "Incorrect number of uris. Got: {0}".format(extensions_goal_state.agent_manifests[0].uris))
+            self.assertEqual("https://zrdfepirv2cdm03prdstr01a.blob.core.windows.net/7d89d439b79f4452950452399add2c90/Microsoft.OSTCLinuxAgent_Prod_uscentraleuap_manifest.xml", extensions_goal_state.agent_manifests[0].uris[0], "Incorrect number of uris.")


I think this error message can be better. Instead of, Incorrect number of uris, maybe first URI does not match. Expected: {0}, got {1} or something to that effect.

nagworld9 · 2022-02-17T23:16:35Z

azurelinuxagent/common/protocol/hostplugin.py

-            # Don't support HostGAPlugin versions older than 115
-            if vm_settings.host_ga_plugin_version < FlexibleVersion("1.0.8.115"):
+            # Don't support HostGAPlugin versions older than 123
+            # TODO: update the minimum version to 1.0.8.117 before release


Just to clarify, you mean minimum version to 123 before release?

yes, i'll fix the comment

nagworld9 · 2022-02-17T23:42:44Z

azurelinuxagent/common/protocol/hostplugin.py

-            if vm_settings.host_ga_plugin_version < FlexibleVersion("1.0.8.115"):
+            # Don't support HostGAPlugin versions older than 123
+            # TODO: update the minimum version to 1.0.8.117 before release
+            if vm_settings.host_ga_plugin_version < FlexibleVersion("1.0.8.117"):
                raise_not_supported(reset_state=True)


If not supported, how does this update supports_fast_track flag as false

the flag is maintained by the UpdateHandler and computed like this

supports_fast_track = self._goal_state.extensions_goal_state.source_channel == GoalStateChannel.HostGAPlugin

If the HostGAPlugin does not support vmsettings (or is the wrong version) we fallback to the WireServer

Note also that the HostGAPlugin may support fast track, but the feature may be disabled in waagent.conf. The above condition takes care of that too.

The naming is a little awkward: CRP needs FastTrack to be added to the supportedFeatures, but what the flag really means is that the agent is requesting CRP to start using FastTrack. Of course, if the agent sets this flag, it must support FT. But the agent could support FT and still not request CRP to use it (e.g. the user disabled the feature, or the hostgaplugin is not the correct version).

got you. Thanks for explaining.

I am doing another pass of the code (separate PRs) and will add comments and tests to explain the behavior of the feature, as well as to address the TODOs (the behavior is already described in the spec for fast track on the linux agent, but i should capture that in tests)

narrieta added 3 commits February 15, 2022 14:55

Implement FastTrack handshake with CRP

42aba8a

Added unit tests

dbe5085

Set minimum hostgaplugin version to 117

d192d82

narrieta assigned kevinclark19a Feb 17, 2022

narrieta requested review from larohra, kevinclark19a, ZhidongPeng, dhivyaganesan and nagworld9 as code owners February 17, 2022 18:29

narrieta assigned nagworld9 Feb 17, 2022

narrieta commented Feb 17, 2022

View reviewed changes

kevinclark19a reviewed Feb 17, 2022

View reviewed changes

narrieta and others added 2 commits February 17, 2022 15:03

typos

185bb4b

Merge branch 'develop' into fast-track

c6fa8c4

nagworld9 reviewed Feb 17, 2022

View reviewed changes

fix comment, merge

2a367ed

nagworld9 approved these changes Feb 18, 2022

View reviewed changes

kevinclark19a approved these changes Feb 18, 2022

View reviewed changes

narrieta merged commit acd6301 into Azure:develop Feb 18, 2022

narrieta deleted the fast-track branch February 18, 2022 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FastTrack handshake with CRP #2514

Implement FastTrack handshake with CRP #2514

narrieta commented Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

codecov bot commented Feb 17, 2022 •

edited

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022 •

edited

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022 •

edited

narrieta Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

kevinclark19a Feb 17, 2022 •

edited

narrieta Feb 17, 2022

kevinclark19a Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 17, 2022

kevinclark19a Feb 17, 2022

narrieta Feb 17, 2022

nagworld9 Feb 17, 2022

narrieta Feb 17, 2022

nagworld9 Feb 17, 2022

narrieta Feb 17, 2022

narrieta Feb 18, 2022

nagworld9 Feb 18, 2022

narrieta Feb 18, 2022

		@@ -1,26 +0,0 @@
		<Extensions version="1.0.0.0" goalStateIncarnation="9"><GuestAgentExtension xmlns:i="http://www.w3.org/2001/XMLSchema-instance">

		from tests.protocol import mockwiredata
		from tests.protocol.HttpRequestPredicates import HttpRequestPredicates


		yield update_handler

		def test_update_handler_should_report_status_when_fetch_goal_state_fails(self):

		@@ -2868,171 +2868,6 @@ def test_it_should_process_goal_state_only_on_new_goal_state(self):
		self.assertEqual(2, remote_access_handler.run.call_count, "remote_access_handler.run() should have been called on a new goal state")


		class ReportStatusTestCase(AgentTestCase):

		@@ -1,27 +0,0 @@
		# Copyright (c) Microsoft Corporation. All rights reserved.

		remove_timestamps(second_status)

		self.assertEqual(first_status, second_status)

		@@ -45,3 +53,8 @@ def test_it_should_parse_empty_depends_on_as_dependency_level_0(self):

		self.assertEqual(0, extensions[0].settings[0].dependencyLevel, "Incorrect dependencyLevel}")

Implement FastTrack handshake with CRP #2514

Implement FastTrack handshake with CRP #2514

Conversation

narrieta commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Feb 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Feb 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinclark19a Feb 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2022 •

edited

narrieta Feb 17, 2022 •

edited

narrieta Feb 17, 2022 •

edited

kevinclark19a Feb 17, 2022 •

edited