Separate goal state from protocol #1777

narrieta · 2020-02-11T20:38:47Z

if an error occurs while _update_from_goal_state is populating the goal state the retry loop may end up producing a goal state that is only partially populated.

This PR addresses that issue and starts to refactor the goal state out of the protocol. I will do further refactoring to improve that separation once we remove the metadata protocol.

The code refactored in this PR is heavily exercised by the current tests. I will add tests specific to the goal state class in my next round of refactoring.

I did manual verification using the debugger on a live VM, and automation reported no errors.

This change is

codecov · 2020-02-11T20:41:09Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@d19c41c). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             develop    #1777   +/-   ##
==========================================
  Coverage           ?   68.95%           
==========================================
  Files              ?       81           
  Lines              ?    11322           
  Branches           ?     1597           
==========================================
  Hits               ?     7807           
  Misses             ?     3187           
  Partials           ?      328

Impacted Files	Coverage Δ
azurelinuxagent/common/version.py	`71.73% <ø> (ø)`
azurelinuxagent/pa/provision/default.py	`73.97% <100%> (ø)`
azurelinuxagent/ga/update.py	`88.22% <100%> (ø)`
azurelinuxagent/common/protocol/util.py	`79.76% <100%> (ø)`

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 d19c41c...7f2e6a6. Read the comment docs.

narrieta · 2020-02-11T20:41:26Z

azurelinuxagent/common/protocol/__init__.py

-from azurelinuxagent.common.protocol.util import get_protocol_util, \
-                                                 OVF_FILE_NAME, \
-                                                 TAG_FILE_NAME
-


This import was creating a dependency on protocol.util to code that imported anything under protocol and was leading to a couple of circular dependencies. I remove the statement and updated the code to always import get_procotol_util from protocol.util instead of protocol.

narrieta · 2020-02-11T20:42:09Z

azurelinuxagent/common/protocol/goal_state.py

@@ -0,0 +1,411 @@
+# Microsoft Azure Linux Agent


Most of the code here comes from wire.py, but please do a careful review. Thanks!

narrieta · 2020-02-11T20:44:47Z

azurelinuxagent/common/protocol/goal_state.py

+        else:
+            xml_text = wire_client.fetch_config(uri, wire_client.get_header_for_cert())
+            self.remote_access = RemoteAccess(xml_text)
+


I'm planning on moving the fetch_* methods below to protocol, and remove the current interface that exposes the goal state from the protocol class. I.e. code would do "goal_state = protocol.fetch_goal_state()" and then use "goal_state" directly instead of using "protocol" to retrieve the properties of the goal state.

Sounds like a good idea, thanks for doing this!

narrieta · 2020-02-11T20:46:26Z

azurelinuxagent/common/protocol/goal_state.py

+
+
+class GoalState(object):
+    def __init__(self, wire_client, full_goal_state=False, base_incarnation=None):


Now the constructor of GoalState has a dependency on the protocol, which makes unit testing a little harder. To compensate for that I created mock_wire_protocol.py in the tests directory). Many tests cleanup nicely using mock_wire_protocol; I only updated the tests affected by this PR but will cleanup other tests as I touch the corresponding code.

narrieta · 2020-02-11T20:48:15Z

azurelinuxagent/common/protocol/wire.py

 from azurelinuxagent.common.version import AGENT_NAME, CURRENT_VERSION

 VERSION_INFO_URI = "http://{0}/?comp=versions"
-GOAL_STATE_URI = "http://{0}/machine/?comp=goalstate"


These variables and methods were moved to goal_state.py

narrieta · 2020-02-11T20:49:14Z

azurelinuxagent/common/protocol/wire.py

+                    new_goal_state = GoalState.fetch_full_goal_state_if_incarnation_different_than(self, self._goal_state.incarnation)
+
+                if new_goal_state is not None:
+                    self._goal_state = new_goal_state


this fixes the original issue: the entire goal state is updated or no updates are done

narrieta · 2020-02-11T20:50:55Z

azurelinuxagent/common/protocol/wire.py

-        self._ext_conf = new_ext_conf
+                if goal_state_property is not None and goal_state_property.xml_text is not None:
+                    self.save_cache(file_path, goal_state_property.xml_text)
+                else:  # remove the file from the previous goal state if it exists


i forgot to remove this else (removing the older files is not needed, since self.goal_state_flusher.flush() above moves the files to the history folder

Thanks for clarifying that! This did confuse me a bit

narrieta · 2020-02-11T20:52:37Z

tests/data/wire/ext_conf.xml

@@ -3,19 +3,19 @@
    <GAFamily>
      <Name>Prod</Name>
      <Uris>
-          <Uri>http://manifest_of_ga.xml</Uri>
+          <Uri>http://mock-goal-state/manifest_of_ga.xml</Uri>


see mock_wire_protocol.py for the motivation of these updates in the test data

Thanks for creating that file, really helpful!

larohra

Left some minor comments

larohra · 2020-02-13T01:27:39Z

azurelinuxagent/common/protocol/goal_state.py

+        else:
+            xml_text = wire_client.fetch_config(uri, wire_client.get_header_for_cert())
+            self.remote_access = RemoteAccess(xml_text)
+


Sounds like a good idea, thanks for doing this!

larohra · 2020-02-13T02:25:01Z

azurelinuxagent/common/protocol/wire.py

+            save_if_not_none(self._goal_state.hosting_env, HOSTING_ENV_FILE_NAME)
+            save_if_not_none(self._goal_state.shared_conf, SHARED_CONF_FILE_NAME)
+            save_if_not_none(self._goal_state.ext_conf, EXT_CONF_FILE_NAME.format(self._goal_state.incarnation))
+            save_if_not_none(self._goal_state.remote_access, REMOTE_ACCESS_FILE_NAME.format(self._goal_state.incarnation))


Just curious, why is "Certificates.xml" handled differently in the goal_state.py file whereas all these other files are handled here?

Yes, I forgot to add a comment here.

Certificates doesn't hold a reference to xml_text. I did not add one since I'm guessing the original intention was not to hold the data on memory for too long.

larohra · 2020-02-13T02:25:17Z

azurelinuxagent/common/protocol/wire.py

-        self._ext_conf = new_ext_conf
+                if goal_state_property is not None and goal_state_property.xml_text is not None:
+                    self.save_cache(file_path, goal_state_property.xml_text)
+                else:  # remove the file from the previous goal state if it exists


Thanks for clarifying that! This did confuse me a bit

larohra · 2020-02-13T02:26:23Z

azurelinuxagent/common/protocol/wire.py

@@ -867,29 +790,29 @@ def get_goal_state(self):
        return self._goal_state

    def get_hosting_env(self):
-        if self._hosting_env is None:
+        if self._goal_state is None:


shouldn't this be self._goal_state.hosting_env rather than just self._goal_state?

The intention of this check was to ensure that update_goal_state was called before retrieving any of the properties of the goal state, wasn't it?

Ohh sorry I missed the raise in the next line. When I introduced this code we werent raising error rather just logging a warning which is what I thought was still happening here. Thanks for clarifying!

azurelinuxagent/common/protocol/wire.py

larohra · 2020-02-13T02:27:51Z

azurelinuxagent/common/protocol/wire.py


    def get_certs(self):
        # This property can be None in the GoalState, so not returning a ProtocolError here
-        return self._certs
+        return self._goal_state.certs


This will throw an error if self._goal_state is None, we should handle that case

Thanks, added

larohra · 2020-02-13T02:32:41Z

tests/data/wire/ext_conf.xml

@@ -3,19 +3,19 @@
    <GAFamily>
      <Name>Prod</Name>
      <Uris>
-          <Uri>http://manifest_of_ga.xml</Uri>
+          <Uri>http://mock-goal-state/manifest_of_ga.xml</Uri>


Thanks for creating that file, really helpful!

narrieta · 2020-02-13T17:06:33Z

tests/protocol/mock_wire_protocol.py

+        wire_server_endpoint = 'http://{0}'.format(KNOWN_WIRESERVER_IP)
+
+        def mock_http_get(url, *args, **kwargs):
+            if not (url.startswith(wire_server_endpoint) or url.startswith('http://mock-goal-state/') or url.startswith('https://mock-goal-state/')):


this should be case insensitive

larohra

LGTM

pgombar · 2020-02-19T00:22:43Z

tests/protocol/test_hostplugin.py


-        # assert direct route is called
-        self.assertEqual(1, patch_upload.call_count, "Direct channel was not used")
+            # assert host plugin route is called


Should be "host plugin route is NOT called"?

narrieta added 4 commits January 31, 2020 16:23

goal state

d813091

Test updates

d55c1a9

Merge branch 'develop' into goal-state

402f339

Separate goal state from protocol

01a11fb

narrieta requested review from vrdmr, pgombar and larohra February 11, 2020 20:38

narrieta commented Feb 11, 2020

View reviewed changes

no need to remove previuos files

1cfe951

larohra reviewed Feb 13, 2020

View reviewed changes

narrieta commented Feb 13, 2020

View reviewed changes

narrieta added 2 commits February 13, 2020 09:27

Case insentive comparison; review feedback

d19c41c

Merge from develop

7f2e6a6

larohra approved these changes Feb 13, 2020

View reviewed changes

narrieta merged commit ce8e736 into Azure:develop Feb 13, 2020

narrieta deleted the goal-state branch February 13, 2020 22:24

pgombar reviewed Feb 19, 2020

View reviewed changes

narrieta mentioned this pull request Mar 6, 2020

Enable WireClient tests #1800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate goal state from protocol #1777

Separate goal state from protocol #1777

narrieta commented Feb 11, 2020 •

edited by vrdmr

Loading

codecov bot commented Feb 11, 2020 •

edited

Loading

narrieta Feb 11, 2020

narrieta Feb 11, 2020

narrieta Feb 11, 2020

larohra Feb 13, 2020

narrieta Feb 11, 2020 •

edited

Loading

narrieta Feb 11, 2020

narrieta Feb 11, 2020

narrieta Feb 11, 2020

larohra Feb 13, 2020

narrieta Feb 11, 2020

larohra Feb 13, 2020

larohra left a comment

larohra Feb 13, 2020

larohra Feb 13, 2020

narrieta Feb 13, 2020

larohra Feb 13, 2020

larohra Feb 13, 2020

narrieta Feb 13, 2020

larohra Feb 13, 2020

larohra Feb 13, 2020

narrieta Feb 13, 2020

larohra Feb 13, 2020

narrieta Feb 13, 2020

larohra left a comment

pgombar Feb 19, 2020



		class GoalState(object):
		def __init__(self, wire_client, full_goal_state=False, base_incarnation=None):

Separate goal state from protocol #1777

Separate goal state from protocol #1777

Conversation

narrieta commented Feb 11, 2020 • edited by vrdmr Loading

codecov bot commented Feb 11, 2020 • edited Loading

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

narrieta Feb 11, 2020 • edited Loading

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

larohra left a 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

larohra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta commented Feb 11, 2020 •

edited by vrdmr

Loading

codecov bot commented Feb 11, 2020 •

edited

Loading

narrieta Feb 11, 2020 •

edited

Loading