Fast Track: Ignore Fabric goal states from HostGAPlugin; process most recent goal state from WireServer/HostGAPlugin #2541

narrieta · 2022-03-30T21:26:04Z

The goal states returned by the HostGAPlugin and the WireServer can be out of sync. This presents a problem when the most recent goal state includes an updated tenant certificate (this is always a Fabric goal state). In this case, we need to ensure that we first download the certificate and the process the extensions.

To achieve this, we now ignore Fabric goal states from the HostGAPlugin and we process the most recent goal state from the 2 goal states we receive from WireServer/HostGAPlugin.

This PR also includes some minor cleanup of portions of the Fast Track code. I'll add comments in the PR to point those out.

narrieta · 2022-03-30T21:27:48Z

azurelinuxagent/common/exception.py

@@ -121,16 +121,6 @@ class ExtensionsConfigError(ExtensionsGoalStateError):
    """


-class VmSettingsError(ExtensionsGoalStateError):


I renamed this exception to VmSettingsParseError (since it is only used to report parse errors) and moved it to extensions_goal_state.py (since that is the module that raises it)

narrieta · 2022-03-30T21:28:29Z

azurelinuxagent/common/protocol/extensions_goal_state_from_vm_settings.py

@@ -56,7 +55,8 @@ def __init__(self, etag, json_text, correlation_id):
            self._parse_vm_settings(json_text)
            self._do_common_validations()
        except Exception as e:
-            raise VmSettingsError("Error parsing vmSettings [HGAP: {0}]: {1}".format(self._host_ga_plugin_version, ustr(e)), etag, self.get_redacted_text())
+            message = "Error parsing vmSettings [HGAP: {0} Etag:{1}]: {2}".format(self._host_ga_plugin_version, etag, ustr(e))


added the etag to the error message

narrieta · 2022-03-30T21:29:42Z

azurelinuxagent/common/protocol/extensions_goal_state_from_vm_settings.py

-
-    @staticmethod
-    def redact(text):
-        return re.sub(r'("protectedSettings"\s*:\s*)"[^"]+"', r'\1"*** REDACTED ***"', text)


The redact() method was used in a previous iteration of the code, but now there is no need for it, so I folded it into get_redacted_text()

narrieta · 2022-03-30T21:30:26Z

azurelinuxagent/common/protocol/extensions_goal_state_from_vm_settings.py

@@ -165,7 +161,7 @@ def _parse_simple_attributes(self, vm_settings):
        #         "correlationId": "9a47a2a2-e740-4bfc-b11b-4f2f7cfe7d2e",
        #         "inSvdSeqNo": 1,
        #         "extensionsLastModifiedTickCount": 637726657706205217,
-        #         "extensionGoalStatesSource": "Fabric",
+        #         "extensionGoalStatesSource": "FastTrack",


Updated the vmSettings test data and sample code to FastTrack (since now we ignore Fabric)

narrieta · 2022-03-30T21:32:36Z

azurelinuxagent/common/protocol/goal_state.py

-            self._history = GoalStateHistory(timestamp, incarnation)
-
-            self._initialize_basic_properties(xml_doc)
-            self._fetch_extended_goal_state(xml_text, xml_doc)


These changes are part of the new fix. I merged _initialize_basic_properties and _fetch_extended_goal_state into a single method (_fetch_full_wire_server_goal_state) and invoked that method from update().

I also removed code from init() that is already done by update()

narrieta · 2022-03-30T21:46:13Z

tests/ga/test_report_status.py

@@ -139,12 +139,11 @@ def http_get_handler(url, *_, **__):
            self._test_supported_features_includes_fast_track(protocol, False)

    def _test_supported_features_includes_fast_track(self, protocol, expected):
-        with patch("azurelinuxagent.common.conf.get_enable_fast_track", return_value=True):


Removed all the patches that were setting EnableFastTrack to True. I made FastTrack the default several weeks ago, but was keeping these patches around in case we needed a release with FastTrack disable. No need to keep them around anymore.

narrieta · 2022-03-30T21:47:06Z

tests/protocol/mockwiredata.py

-        '''
-        Sets the ETag for the mock response
-        '''
+    def set_etag(self, etag, timestamp=None):


I need new test helpers to change the source (fast track vs fabric) and timestamp of the goal states in the mock data

narrieta · 2022-03-30T21:47:41Z

tests/protocol/test_extensions_goal_state_from_vm_settings.py

@@ -19,7 +18,7 @@ def assert_property(name, value):

            assert_property("activity_id", "a33f6f53-43d6-4625-b322-1a39651a00c9")
            assert_property("correlation_id", "9a47a2a2-e740-4bfc-b11b-4f2f7cfe7d2e")
-            assert_property("created_on_timestamp", "2021-11-16T13:22:50.620522Z")
+            assert_property("created_on_timestamp", "2021-11-16T13:22:50.620529Z")


updated the timestamp on a few data files to make them the most recent goal state

narrieta · 2022-03-30T21:48:19Z

tests/protocol/test_goal_state.py


            _ = GoalState(protocol.client)

            self._assert_directory_contents(
-                self._find_history_subdirectory("999"),
+                self._find_history_subdirectory("999-888"),


new suffix for history directories (incarnation-etag)

narrieta · 2022-03-30T21:48:49Z

tests/protocol/test_goal_state.py

+            self.assertEqual(expected, actual, "The vmSettings were not saved correctly")
+


Test cases for the certificate fix

codecov · 2022-03-31T03:46:46Z

Codecov Report

Merging #2541 (416765a) into develop (75fcab8) will increase coverage by 0.07%.
The diff coverage is 96.33%.

❗ Current head 416765a differs from pull request most recent head 06d977c. Consider uploading reports for the commit 06d977c to get more accurate results

@@             Coverage Diff             @@
##           develop    #2541      +/-   ##
===========================================
+ Coverage    71.99%   72.07%   +0.07%     
===========================================
  Files          102      102              
  Lines        15202    15228      +26     
  Branches      2406     2416      +10     
===========================================
+ Hits         10945    10975      +30     
+ Misses        3776     3773       -3     
+ Partials       481      480       -1

Impacted Files	Coverage Δ
azurelinuxagent/common/exception.py	`95.95% <ø> (-0.20%)`	⬇️
azurelinuxagent/common/protocol/hostplugin.py	`88.85% <83.33%> (-0.45%)`	⬇️
azurelinuxagent/common/utils/timeutil.py	`88.88% <87.50%> (-11.12%)`	⬇️
azurelinuxagent/common/protocol/goal_state.py	`95.30% <98.61%> (-1.52%)`	⬇️
...inuxagent/common/protocol/extensions_goal_state.py	`72.93% <100.00%> (+1.27%)`	⬆️
...protocol/extensions_goal_state_from_vm_settings.py	`83.33% <100.00%> (-0.19%)`	⬇️
azurelinuxagent/common/utils/archive.py	`88.32% <100.00%> (+0.35%)`	⬆️
azurelinuxagent/ga/update.py	`88.56% <100.00%> (+0.16%)`	⬆️
azurelinuxagent/common/event.py	`86.03% <0.00%> (+0.67%)`	⬆️
... and 1 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 75fcab8...06d977c. Read the comment docs.

narrieta · 2022-03-31T18:06:15Z

azurelinuxagent/ga/update.py

@@ -659,17 +661,9 @@ def __get_vmagent_update_status(self, protocol, goal_state_changed):
    def _report_status(self, exthandlers_handler):
        vm_agent_update_status = self.__get_vmagent_update_status(exthandlers_handler.protocol, self._processing_new_extensions_goal_state())
        # report_ext_handlers_status does its own error handling and returns None if an error occurred
-        #


The first goal state is always Fabric, and now we ignore Fabric goal states from the HostGAPlugin, so

self._goal_state.extensions_goal_state.channel == GoalStateChannel.HostGAPlugin
would never be True.

Now we do this in _try_update_goal_state(), using check_vm_settings_support()

azurelinuxagent/common/protocol/goal_state.py

tests/protocol/test_goal_state.py

larohra · 2022-04-01T21:15:02Z

azurelinuxagent/common/protocol/goal_state.py

+        if not goal_state_updated and not vm_settings_updated:
+            return
+
+        # Start a new history subdirectory and capture the updated goal state


wont this get confusing?

larohra · 2022-04-01T21:39:53Z

azurelinuxagent/common/protocol/goal_state.py

+            except VmSettingsParseError as exception:
+                # ensure we save the vmSettings if there were parsing errors, but save them only once per ETag
+                if not GoalStateHistory.tag_exists(exception.etag):
+                    GoalStateHistory(timeutil.create_timestamp(), exception.etag).save_vm_settings(exception.vm_settings_text)


If by some magic the parsing error goes away in the next iteration for the same etag, will we overwrite the same history directory? Would we want to keep the bad file for debugging purposes?

kevinclark19a · 2022-04-04T23:58:58Z

tests/protocol/test_goal_state.py

+            vm_settings, _ = protocol.client.get_host_plugin().fetch_vm_settings()
+            if vm_settings.etag != etag:
+                raise Exception("The HostGAPlugin is no in sync. Expected ETag {0}. Got {1}".format(etag, vm_settings.etag))


small typo, no in sync to not in sync.

kevinclark19a · 2022-04-04T23:59:13Z

tests/protocol/test_goal_state.py

+            goal_state = GoalState(protocol.client)
+            if goal_state.incarnation != incarnation:
+                raise Exception("The WireServer is no in sync. Expected incarnation {0}. Got {1}".format(incarnation, goal_state.incarnation))


same here as above

narrieta added 7 commits March 26, 2022 08:40

fix certificate

48b78ef

.

fdac74f

Fix unit tests

5f03dbe

save parsing errors

0c1e622

.

9d7065a

Add unit tests

5cbfd31

Output message only once per goal state

367ff56

narrieta requested review from larohra, kevinclark19a, ZhidongPeng, dhivyaganesan and nagworld9 as code owners March 30, 2022 21:26

narrieta assigned kevinclark19a and nagworld9 Mar 30, 2022

narrieta commented Mar 30, 2022

View reviewed changes

narrieta and others added 3 commits March 30, 2022 14:56

Remove placeholder

99d9958

Merge branch 'develop' into certificate

95f0686

pylint warnings and py2.6 compat

736b103

narrieta added 2 commits March 31, 2022 09:14

enable fast track

98da058

pylint

9b664bf

narrieta commented Mar 31, 2022

View reviewed changes

ut fixes

416765a

nagworld9 reviewed Apr 1, 2022

View reviewed changes

azurelinuxagent/common/protocol/goal_state.py Show resolved Hide resolved

azurelinuxagent/common/protocol/goal_state.py Show resolved Hide resolved

update history regex

06d977c

nagworld9 reviewed Apr 1, 2022

View reviewed changes

azurelinuxagent/common/protocol/goal_state.py Show resolved Hide resolved

tests/protocol/test_goal_state.py Show resolved Hide resolved

larohra approved these changes Apr 1, 2022

View reviewed changes

nagworld9 approved these changes Apr 4, 2022

View reviewed changes

kevinclark19a reviewed Apr 4, 2022

View reviewed changes

kevinclark19a approved these changes Apr 5, 2022

View reviewed changes

narrieta merged commit f91bf78 into Azure:develop Apr 5, 2022

narrieta deleted the certificate branch April 5, 2022 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast Track: Ignore Fabric goal states from HostGAPlugin; process most recent goal state from WireServer/HostGAPlugin #2541

Fast Track: Ignore Fabric goal states from HostGAPlugin; process most recent goal state from WireServer/HostGAPlugin #2541

narrieta commented Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

narrieta Mar 30, 2022

codecov bot commented Mar 31, 2022 •

edited

narrieta Mar 31, 2022

larohra Apr 1, 2022

larohra Apr 1, 2022

kevinclark19a Apr 4, 2022

kevinclark19a Apr 4, 2022

		@@ -121,16 +121,6 @@ class ExtensionsConfigError(ExtensionsGoalStateError):
		"""


		class VmSettingsError(ExtensionsGoalStateError):

		self.assertEqual(expected, actual, "The vmSettings were not saved correctly")

Fast Track: Ignore Fabric goal states from HostGAPlugin; process most recent goal state from WireServer/HostGAPlugin #2541

Fast Track: Ignore Fabric goal states from HostGAPlugin; process most recent goal state from WireServer/HostGAPlugin #2541

Conversation

narrieta commented Mar 30, 2022

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 Mar 31, 2022 • edited

Codecov Report

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 Mar 31, 2022 •

edited