Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move fetch_vm_settings() to HostPluginProtocol #2486

Merged
merged 5 commits into from Jan 26, 2022

Conversation

narrieta
Copy link
Member

Some code cleanup. Moved the logic and data structures for the vmSettings request from the WireClient to the HostPluginProtocol.

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #2486 (9a353b1) into fast-track (ea00436) will decrease coverage by 0.00%.
The diff coverage is 93.43%.

Impacted file tree graph

@@              Coverage Diff               @@
##           fast-track    #2486      +/-   ##
==============================================
- Coverage       71.78%   71.78%   -0.01%     
==============================================
  Files             101      101              
  Lines           14987    14990       +3     
  Branches         2388     2389       +1     
==============================================
+ Hits            10758    10760       +2     
  Misses           3742     3742              
- Partials          487      488       +1     
Impacted Files Coverage Δ
azurelinuxagent/common/protocol/hostplugin.py 88.85% <92.50%> (+2.02%) ⬆️
azurelinuxagent/common/protocol/wire.py 79.16% <100.00%> (-1.77%) ⬇️

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 ea00436...9a353b1. Read the comment docs.

@@ -383,3 +390,174 @@ def _base64_encode(self, data):
if PY_VERSION_MAJOR > 2:
return s.decode('utf-8')
return s

Copy link
Member Author

@narrieta narrieta Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from the WireClient, the only difference in the implementation is the use of ResourceGoneError to report 410s (added the exception here because the retry logic needs to be implemented by the caller)

@@ -862,6 +862,125 @@ def test_should_report(self):
self.assertEqual(False, actual)


class TestHostPluginVmSettings(HttpRequestPredicates, AgentTestCase):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move from test_wire. The implementation is almost identical to the original, but instead of testing via WireClient.update_goal_state() now we test via HostPluginProtocol.fetch_vm_settings()

Copy link
Contributor

@larohra larohra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments else LGTM

azurelinuxagent/common/protocol/hostplugin.py Show resolved Hide resolved
azurelinuxagent/common/protocol/hostplugin.py Outdated Show resolved Hide resolved

#
# Then we fetch the vmSettings from the HostGAPlugin; the response will include the goal state for extensions.
#
vm_settings_goal_state, vm_settings_goal_state_updated = (None, False)
vm_settings, vm_settings_updated = (None, False)

if conf.get_enable_fast_track():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should consider renaming this flag to get_enable_vm_settings or something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag controls the Fast Track feature. At this point it only controls the call to vmSettings, but soon it will control other stuff, too, like the handshake with CRP

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, how are we going to handle it if the customer wants to opt-out of fast-track, will we just avoid using vmSettings altogether? In short we're considering vmSettings tantamount to fast-track?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if fast track is disabled we will simply use extensionsConfig. That is already implemented in this PR. Yes, vmSettings is fundamental for FastTrack, no fast-track => do not use vmsettings, no vmsettings => do not use fast track.

@narrieta narrieta merged commit 951908d into Azure:fast-track Jan 26, 2022
@narrieta narrieta deleted the host-ga-plugin branch January 26, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants